-
Notifications
You must be signed in to change notification settings - Fork 1k
Check glibc version at startup and abort if below 2.28 #9560
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
f99eb02 to
a9597b2
Compare
fab-10
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks, just small suggestions
| logger.error("Insufficient glibc version detected."); | ||
| logger.error("Required: {} or higher", minVersion); | ||
| logger.error("Found: {}", glibcVersion); | ||
| logger.error("Please upgrade your system's glibc to version {} or higher.", minVersion); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
collapse everything on a single log line
| return false; | ||
| } | ||
| } | ||
| return true; // Versions are equal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this also happens if there are no . in the version
| return; | ||
| } | ||
|
|
||
| final String minVersion = "2.28"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to introduce a constant for this value
|
@fab-10 Updated. Consolidated the error messages, added the constant, and fixed the version comparison to handle edge cases. |
|
you need to fix the DCO on the last commit |
d009186 to
7483e0f
Compare
fab-10
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, now just update your branch with main
|
there are failures in the CI, have not you tried to build and test locally? |
fab-10
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the failires in the CI, and please use a real email for the DCO
7483e0f to
0914fa6
Compare
Fixes hyperledger#9551 Signed-off-by: Anubhav Singh <anubhavsingh@Anubhavs-MacBook-Air.local> Signed-off-by: Anubhav <anumukul009@gmail.com>
- Consolidate error logging into single line - Add MINIMUM_GLIBC_VERSION constant - Handle version strings without dots in comparison logic Signed-off-by: Anubhav Singh <anubhavsingh@Anubhavs-MacBook-Air.local> Signed-off-by: Anubhav <anumukul009@gmail.com>
0914fa6 to
624b495
Compare
garyschulte
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
|
|
||
| checkGlibcVersion(glibcVersion); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR - I hadn't seen it until just now.
besu-native is not required for non-mainnet uses of besu, and that is the root of the glibc requirement. Bouncycastle provides a pure-java implementation of most of the crypto libraries necessary. We should not make a specific version glibc a hard requirement for startup. This could break alpine linux for example, which uses musl rather than glibc.
Ideally, we would make the glibc check in the besu startup script, that way it can be trivially bypassed for non-standard but otherwise valid configurations. Also, it should be only required for ethereum mainnet configurations that have a hard requirement of the besu-native implementation. see NativeRequirement.java
If we want to keep the loading checks in besu java, perhaps we can move this into the ValidateConfigSubcommand and call it from the startup script to assert loading is successful.
It is going to be a bit tricky to make and keep up-to-date the assertion about glibc version, since it is dependent on the platform building besu-native, which at the time of writing is ubuntu-22.04. But I think moving the startup check into a subcommand and modifying the besu startup script to execute the subcommand would be a great place to add this safety/clarity - it is flexible enough to be useful and bypassable, and is open to extension in the future.
Adds a check for glibc version at Besu startup. If the detected version is below 2.28, startup is aborted with a clear error message instead of failing later with confusing native library load errors.
Related to #9506 where users with older glibc versions experience cryptic failures when native libraries try to load.
Fixes #9551