Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

[Tools] Generate tables include path fix #8674

Merged
merged 1 commit into from
Apr 26, 2023
Merged

[Tools] Generate tables include path fix #8674

merged 1 commit into from
Apr 26, 2023

Conversation

laemtl
Copy link
Contributor

@laemtl laemtl commented Apr 25, 2023 •
edited
Loading

Fix the include paths to execute the tools from anywhere without breaking the script.

@CamilleBeau
Copy link
Contributor

Still getting an error when running from a different directory. I think line 34 needs to be modified as well
lorisadmin@cbeaudoin-dev:/var/www/loris$ php tools/generate_tables_sql.php PHP Warning: filesize(): stat failed for ip_output.txt in /var/www/loris/tools/generate_tables_sql.php on line 34 PHP Fatal error: Uncaught ValueError: fread(): Argument #2 ($length) must be greater than 0 in /var/www/loris/tools/generate_tables_sql.php:34 Stack trace: #0 /var/www/loris/tools/generate_tables_sql.php(34): fread() #1 {main} thrown in /var/www/loris/tools/generate_tables_sql.php on line 34

CamilleBeau
CamilleBeau approved these changes Apr 26, 2023
View reviewed changes
Copy link
Contributor

@CamilleBeau CamilleBeau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everythings working great! Left a comment about the added print statement, but the scripts are working well.

tools/generate_tables_sql_and_testNames.php Outdated
@@ -38,14 +38,15 @@
foreach ($items as $item) {
$paramId = "";
$bits = explode("{@}", trim($item));
print_r($bits);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this meant to be added or was it for debugging?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks :)

@CamilleBeau CamilleBeau added the Passed Manual Tests PR has undergone proper testing by at least one peer label Apr 26, 2023
driusan
driusan reviewed Apr 26, 2023
View reviewed changes
tools/generate_tables_sql.php Outdated
require_once "../php/libraries/Database.class.inc";
require_once "../php/libraries/NDB_Config.class.inc";
require_once "../php/libraries/NDB_BVL_Instrument.class.inc";
require_once __DIR__ . "/../php/libraries/Database.class.inc";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why aren't these all autoloaded?

driusan
driusan reviewed Apr 26, 2023
View reviewed changes
tools/generate_tables_sql_and_testNames.php Outdated
require_once "../php/libraries/Database.class.inc";
require_once "../php/libraries/NDB_Config.class.inc";
require_once "../php/libraries/NDB_BVL_Instrument.class.inc";
require_once __DIR__ . "/../php/libraries/Database.class.inc";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with these

@laemtl laemtl requested a review from driusan April 26, 2023 15:35
@laemtl
Copy link
Contributor Author

laemtl commented Apr 26, 2023

@driusan Ready for second review

@CamilleBeau
Copy link
Contributor

Re-tested and it's working well with last update

@driusan driusan merged commit 2ff7962 into aces:main Apr 26, 2023
sanjay-thiyagarajan pushed a commit to sanjay-thiyagarajan/Loris that referenced this pull request Jun 17, 2023
@laemtl @sanjay-thiyagarajan
[Tools] Generate tables include path fix (aces#8674)
6cf118a 
Fix the include paths to execute the tools from anywhere without breaking the script.
@ridz1208 ridz1208 added this to the 26.0.0 milestone Jun 20, 2023
@maximemulder maximemulder mentioned this pull request Jul 25, 2024
Open
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@CamilleBeau CamilleBeau CamilleBeau approved these changes

@driusan driusan Awaiting requested review from driusan

Assignees

@CamilleBeau CamilleBeau

Labels
Passed Manual Tests PR has undergone proper testing by at least one peer
Projects
None yet
Milestone
26.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@laemtl @CamilleBeau @driusan @ridz1208