Skip to content
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

Support systems with both glibc and Musl #661

Merged
merged 8 commits into from
Aug 11, 2023
Merged

Support systems with both glibc and Musl #661

merged 8 commits into from
Aug 11, 2023

Conversation

graebm
Copy link
Contributor

@graebm graebm commented Aug 10, 2023

Issue #: #659

Customer has a system with both glibc and Musl on it. The system is Alpine-based, and uses Musl for most things. But the installed Java is using glibc.

aws-crt-java used to work on this system, back when the shared lib only worked with glibc. But as of v0.23.0 we have different shared libs for glibc and Musl, and need to detect at runtime which to use.

Our current method checks only checks whether the system's built-in libraries use Musl.

Description of changes:

NEW: Let users specify "musl" or "glibc" via system property "aws.crt.libc".

NEW: Otherwise, check whether the current Java executable is using glibc or Musl.

And if those fail, fall back to the previous method of calling ldd --version to see if it mentions "musl".

Testing:

I tested this by hand. It would be a ton of effort to hook up regular CI for this. Seeing as this is such an edge-case, that feels sufficient.

I didn't touch the compile-time CMake logic, only the runtime logic. So you can't test this without using our released JARs (which have shared libs for every platform), or by preparing a JAR by hand with shared libs for both Musl and glibc. Again, since this is such an edge case, this level of effort seems sufficient.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@graebm graebm changed the title Support glibc on Musl Support systems with both glibc and Musl Aug 10, 2023
Copy link
Contributor

@waahm7 waahm7 left a comment

Choose a reason for hiding this comment

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

fix & ship

StringBuilder outputBuilder = new StringBuilder();
while ((line = stdOutput.readLine()) != null) {
outputBuilder.append(line);
// The system might have both musl and glibc on it:
Copy link
Contributor

@waahm7 waahm7 Aug 10, 2023

Choose a reason for hiding this comment

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

We should add a log or print statement of what was detected. This way, we can easily debug in the future if the wrong library gets loaded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, CRT java code logs via our native logger, but we can't use that yet because the C code isn't loaded...

The only place we do System.err.println() is when the CRT has failed to load.

I'm hesitant to print debug-but-not-error info to stderr or stdout. People hate when libraries do that.

I'm just going to 🤷‍♀️ for now

// https://github.com/awslabs/aws-crt-java/issues/659

// First, check which one java is using.
// Run: ldd $JAVA_HOME/bin/java
Copy link
Contributor

Choose a reason for hiding this comment

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

$JAVA_HOME and System property can return different values.

Suggested change
// Run: ldd $JAVA_HOME/bin/java
// Run: ldd System.getProperty("java.home")/bin/java

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comment was a hand-wavey allusion to something you could copy paste to your own terminal
I changed it to be less literal by pretty obvious /path/to/java

// https://github.com/awslabs/aws-crt-java/issues/659

// First, check which one java is using.
// Run: ldd $JAVA_HOME/bin/java
Copy link
Contributor

@waahm7 waahm7 Aug 10, 2023

Choose a reason for hiding this comment

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

Not relevant to this PR, but looking more into how other people are detecting musl vs glibc, this solution will fail if the env doesn't have access to ldd; see facebook/rocksdb#9956. Maybe we should just add an environment override like other people; see dmlc/xgboost#7915, and xerial/sqlite-jdbc#706 (comment). We can add it now, or wait until someone has this issue. Your call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's a solid idea. Now user's can do -Daws.crt.libc=musl or -Daws.crt.libc=glibc if our autodetection fails.

I ended up documenting all our System properties in the README, since none of them were documented, and I needed to document the new one somewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And yeah, there are lots of other ways to detect Musl out there, and every PR and stackoverflow has people commenting "this still doesn't work on my machine". We can revisit other ways in the future. Just run them all one after another. But this seems good enough for now...

README.md Outdated Show resolved Hide resolved
Co-authored-by: Waqar Ahmed Khan <waahm7@gmail.com>
@graebm graebm enabled auto-merge (squash) August 11, 2023 20:23
@graebm graebm merged commit 44c38d2 into main Aug 11, 2023
@graebm graebm deleted the glibc-on-musl branch August 11, 2023 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants