Fail early in *nix configuration build script #16717
Conversation
|
Thank you! Makes generally sense to me, but if we explicitly set |
hakre
force-pushed
the
build-fail-early
branch
from
c380a1c to
03f430d
Compare
Yes, +4 more files in the update, but please mind the target branch of this merge request: ext/fileinfo/generate_patch.sh is on a different branch. |
Ah, indeed; and that's probably the least important one (only used maybe once a year). |
Adding one more [1] exit early safeguard in *nix build scripts: Given the initial cd into the build tree fails (the project root), the `buildconf` script exits with non-zero status (failure). Additionally quoting the pathname to cd into and the empty CD_PATH parameter for portability, also for systems that are using a non-portable pathname [2] for the build tree. [1]: php#16717 [2]: https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_271
Adding one more [1] exit early safeguard in *nix build scripts: Given the initial cd into the build tree fails (the project root), the `buildconf` script exits with non-zero status (failure). Additionally quoting the pathname to cd into and the empty CD_PATH parameter for portability, also for systems that are using a non-portable pathname [2] for the build tree. [1]: php#16717 [2]: https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_271
And thinking: Nevertheless this change should be complete in the sense you highlighted at first:
|
|
Thanks @hakre all looks good to me. Except in case of failure there now wouldn't be any info why some script failed. |
Yes, there should be diagnostics for cd(1) "The cd utility shall then perform actions equivalent to the chdir() function called with curpath as the path argument. If these actions fail for any reason, the cd utility shall display an appropriate error message and the remainder of this step shall not be executed." ref. For grep there is no diagnostic message, only the non-zero exit status. This suffices to fail early so the operation is as announced. For the principle of the least astonishment I'll add a message there. Please also take a look at PR #16724 and if possible assign yourself as reviewer there @petk |
|
Argh, adding diagnostics for the grep is a bit in muddy waters. If the If the AC_INIT macro could not be found in Let me know what is wanted, I can implement it but my current assumption is that it is not really helpful at this point. |
|
It's ok. If grep isn't found it is already enough info as this grep in the buildconf should work across all implementations I think. |
hakre
force-pushed
the
build-fail-early
branch
from
03f430d to
6c3ead4
Compare
Adding one more [1] exit early safeguard in *nix build scripts: Given the initial cd into the build tree fails (the project root), the `buildconf` script exits with non-zero status (failure). Additionally quoting the pathname to cd into and the empty CD_PATH parameter for portability, also for systems that are using a non-portable pathname [2] for the build tree. [1]: php#16717 [2]: https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_271
|
I'll push again, two Win32 tests on filenames look flaky: https://github.com/php/php-src/actions/runs/11730392143/job/32678161308#step:6:418 |
hakre
force-pushed
the
build-fail-early
branch
from
6c3ead4 to
0f33884
Compare
Adding two exit early safeguards in the *nix configuration build script: 1) Given the initial cd into the build tree fails (the project root), the `buildconf` script exits with non-zero status (failure). 2) Given the grep command does not exist or `configure.ac` AC_INIT [1] expectations are unmet, the buildconf script exits non-zero. Additionally quoting the pathname to cd into and the empty CD_PATH parameter for portability, also for systems that are using a non-portable pathname [2] for the build tree. The initial CD safeguard has been applied to the `buildconf` and four more scripts: - build/genif.sh - scripts/dev/credits - scripts/dev/genfiles - scripts/dev/makedist Rationale: Cd-ing into the project root should always prematurely exit w/ FAILURE as a required precondition for its invocation has not been met. This should never go unnoticed as it always requires user intervention. Similar and more specifically to the PHP build on *nix systems, the grep command is required early to obtain the `php_extra_version` from configure.ac. Previously, if the grep command is missing (or failing due to not matching the line with the AC_INIT macro [1]), the internal dev parameter would always be zero (0) which can easily result in the situation that the configure script is not being rebuilt. This is cumbersome as the rebuild of a configure script is more likely required with checked-out dev versions under change rather than an already properly set-up build environment on a dedicated build or release system. Missing the fact that either the grep utility is missing or the expectation of having the AC_INIT macro in configure.ac is unmet should never go unnoticed as it always requires user intervention. [1]: https://www.gnu.org/software/autoconf/manual/autoconf-2.67/html_node/Initializing-configure.html [2]: https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_271
hakre
force-pushed
the
build-fail-early
branch
from
0f33884 to
02da729
Compare
|
Thank you! Applied to PHP-8.2 and merged up. |
Adding one more [1] exit early safeguard in *nix build scripts: Given the initial cd into the build tree fails (the project root), the `buildconf` script exits with non-zero status (failure). Additionally quoting the pathname to cd into and the empty CD_PATH parameter for portability, also for systems that are using a non-portable pathname [2] for the build tree. [1]: #16717 [2]: https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_271 Closes GH-16724.
Adding two exit early safeguards in the *nix configuration build script:
the
buildconfscript exits with non-zero status (failure).configure.acAC_INIT 1expectations are unmet, the buildconf script exits non-zero.
Additionally quoting the pathname to cd into and the empty CD_PATH parameter for portability, also for systems that are using a non-portable pathname 2 for the build tree.
Rationale:
CD-ing into the project root should always prematurely exit w/ FAILURE as a required precondition for its invocation has not been met. This should never go unnoticed as it always requires user intervention.
Similar and more specifically to the PHP build on *nix systems, the grep command is required early to obtain the
php_extra_versionfrom configure.ac. Previously, if the grep command is missing (or failing due to not matching the line with the AC_INIT macro 1), the internal dev parameter would always be zero (0) which can easily result in the situation that the configure script is not being rebuilt. This is cumbersome as the rebuild of a configure script is more likely required with checked-out dev versions under change rather than an already properly set-up build environment on a dedicated build or release system. Missing the fact that either the grep utility is missing or the expectation of having the AC_INIT macro in configure.ac is unmet should never go unnoticed as it always requires user intervention.