ENT-12600: detect-environment: Document and refactor #1737
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
larsewi
changed the title
larsewi
changed the title
|
Thanks for submitting a PR! Maybe @craigcomstock can review this? |
larsewi
force-pushed
the
detect-environment
branch
5 times, most recently
from
eb0edfd to
4ccc0c9
Compare
larsewi
commented
Jun 11, 2025
larsewi
force-pushed
the
detect-environment
branch
2 times, most recently
from
d6ca0df to
2632a22
Compare
Signed-off-by: Lars Erik Wik <[email protected]>
Renamed function `detect_labels` to `detect_cross_target`, because this is really what it does. Furthermore, I added a comment explaining the code, as well as code to inform about the detection of cross target. Ticket: ENT-12600 Signed-off-by: Lars Erik Wik <[email protected]>
Ticket: ENT-12600 Signed-off-by: Lars Erik Wik <[email protected]>
The rest of the file uses two space indentation. Hence, the check to see if the functions script was sourced should also have two space indentation for consitency. Ticket: ENT-12600 Signed-off-by: Lars Erik Wik <[email protected]>
Ticket: ENT-12600 Signed-off-by: Lars Erik Wik <[email protected]>
Put the pattern block terminators on separate lines, in the detect_distribution function, for the sake of consistency. Ticket: ENT-12600 Signed-off-by: Lars Erik Wik <[email protected]>
Removed version substitution of the Debian release code-names (e.g., jessie, buster, etc.) from /etc/debian_version. Looking through the [changelog](https://launchpad.net/debian/+source/base-files/+changelog) for the base-files package in Debian, you can easily see that it has never contained the code-names. At least not since version 5.0.3. Furthermore, we have not updated the version substitution since Debian 10 which is End-of-Life. Ticket: ENT-12600 Signed-off-by: Lars Erik Wik <[email protected]>
There has not been a 1-digit major release of ubuntu for a long time. Hence, there is no point in looking for it when verifying the version number extracted from /etc/lsb-release. Ticket: ENT-12600 Signed-off-by: Lars Erik Wik <[email protected]>
Ticket: ENT-12600 Signed-off-by: Lars Erik Wik <[email protected]>
Ticket: ENT-12600 Signed-off-by: Lars Erik Wik <[email protected]>
On SUSE 12 the /etc/SuSE-release file exists with a deprecation warning. However, /etc/os-release also exists. So we might as well remove the parsing of the former and rely on the latter. Ticket: ENT-12600 Signed-off-by: Lars Erik Wik <[email protected]>
If the former if condition is true, then the latter implicitly cannot be true. Hence, there is no point in evaluating it. Ticket: ENT-12600 Signed-off-by: Lars Erik Wik <[email protected]>
Put the pattern block terminators on separate lines, in the detect_packaging function, for the sake of consistency. Ticket: ENT-12600 Signed-off-by: Lars Erik Wik <[email protected]>
Ticket: ENT-12600 Signed-off-by: Lars Erik Wik <[email protected]>
Put the pattern block terminators on separate lines, in the detect_arch function, for the sake of consistency. Ticket: ENT-12600 Signed-off-by: Lars Erik Wik <[email protected]>
For some reason the version detection for FreeBSD and HP-UX was in the detect_arch function. However, it belongs in the detect_os function. Ticket: ENT-12600 Signed-off-by: Lars Erik Wik <[email protected]>
Ticket: ENT-12600 Signed-off-by: Lars Erik Wik <[email protected]>
AIX 5.3 is no longer supported, so we don't need this environment variable anymore. Ticket: ENT-12600 Signed-off-by: Lars Erik Wik <[email protected]>
larsewi
force-pushed
the
detect-environment
branch
from
2632a22 to
b2be1bc
Compare
Signed-off-by: Lars Erik Wik <[email protected]>
The function "fatal" is not defined in this script. So it may not be available. Ticket: ENT-12600 Signed-off-by: Lars Erik Wik <[email protected]>
Ticket: ENT-12600 Signed-off-by: Lars Erik Wik <[email protected]>
Put the pattern block terminators on separate lines, in the detect_cores function, for the sake of consistency. Ticket: ENT-12600 Signed-off-by: Lars Erik Wik <[email protected]>
Ticket: ENT-12600 Signed-off-by: Lars Erik Wik <[email protected]>
The detect-environment script is quite noicy due to the fact that it prints the environment every time it's sourced. Hence, modified it to print only the environment variables that the script affects. Ticket: ENT-12600 Signed-off-by: Lars Erik Wik <[email protected]>
If we disable -x to decrease the verbosity of the build scripts. Then these log messages will come in handy. Ticket: ENT-12600 Signed-off-by: Lars Erik Wik <[email protected]>
larsewi
force-pushed
the
detect-environment
branch
from
b2be1bc to
d02fbb9
Compare
Signed-off-by: Lars Erik Wik <[email protected]>
| @@ -72,7 +72,7 @@ case "$WITH_SYSTEMD" in | |||
| esac | |||
|
|
|||
| # RHEL 8 requires an SELinux policy | |||
| if [ "x$OS" = "xrhel" ] && [ "${VER%\.*}" -gt "7" ]; then | |||
| if [ "x$OS" = "xrhel" ] && [ "${OS_VERSION%\.*}" -gt "7" ]; then | |||
There was a problem hiding this comment.
we don't need to use this x business. https://www.shellcheck.net/wiki/SC2268
Unless we run on awfully old shells, which we do... so maybe change the letter as shellcheck suggests
If you are targeting especially old shells, you can ignore this warning (or use a different letter).
| esac | ||
|
|
||
| # Windows is build on Debian. Hence we need to determine the architecture |
There was a problem hiding this comment.
Suggested change
| # Windows is build on Debian. Hence we need to determine the architecture | |
| # We need to determine the architecture |
Windows is built on Ubuntu-16, not Debian, and I don't think that is important to this area of code which distribution is involved.
|
|
||
| PATCH=`func_whereis gpatch patch` | ||
| # We use it to submit patches to the dependencies. |
There was a problem hiding this comment.
Suggested change
| # We use it to submit patches to the dependencies. | |
| # We use patch to apply patches to the dependencies. |
| # This function appends the -j/--jobs option to the MAKEFLAGS environment | ||
| # variable based on the number of cores. However, the variable is overwritten in | ||
| # the "install-dependencies" script. I created a ticket fix this (see | ||
| # ENT-13041). |
There was a problem hiding this comment.
no doubt! over-rode to -j1 even! :(
| hpux) | ||
| echo "Detected OS family is hpux" | ||
| NUM_CORES="$(ioscan -k -C processor |grep processor | wc -l)";; | ||
| NUM_CORES="$(ioscan -k -C processor | grep -c processor)" |
There was a problem hiding this comment.
did you check this? I wouldn't bet that grep on hpux is "normal" ;)
There was a problem hiding this comment.
I checked, all good in our hpux environment.
-c Only a count of matching lines is printed.
| @@ -386,7 +426,7 @@ detect_environment | |||
| echo | |||
| echo | |||
| echo "==================== Current environment ========================" | |||
| env | |||
| env | grep -E "^(label|CROSS_TARGET|UNAME|OS|OS_VERSION|DEP_PACKAGING|PACKAGING|ARCH|MAKE|FUSER|PATCH|MAKEFLAGS)=" | |||
There was a problem hiding this comment.
I think this line will definitely get lost in refactorings. Maybe we could instead append to a string near each of these and use that here! That way when someone copy/pastas a section about FUSER it will come with all the bits to make this line work.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Build with exotics and no tests
^ Build failures are unrelated: