-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Register the correct native library for Snappy, during native image generation #16069
Conversation
I take that back. Looks like this is indeed enabled in our CI runs https://github.com/quarkusio/quarkus/blob/main/.github/native-tests.json#L68 (I see |
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.
Just made a comment. It looks good to me.
@@ -199,7 +200,7 @@ private void handleSnappy(BuildProducer<ReflectiveClassBuildItem> reflectiveClas | |||
String path = root + dir + "/" + snappyNativeLibraryName; | |||
nativeLibs.produce(new NativeImageResourceBuildItem(path)); | |||
} else { // otherwise the native lib of the platform this build runs on | |||
String dir = getOs() + "/" + getArch(); | |||
String dir = OSInfo.getNativeLibFolderPathForCurrentOS(); |
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.
Is getOS and getArch still used?
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.
Good you asked :) I hadn't noticed that with this change, these methods will now be unused. I've updated this PR to remove them altogether.
…tive image generation
Fixes #16042
The commit here fixes the issue where the Snappy native library that was being registered as a native image resource was using the wrong resource path (it was using
org/xerial/snappy/native/Linux/amd64/libsnappyjava.so
instead oforg/xerial/snappy/native/Linux/x86_64/libsnappyjava.so
). The commit here uses theOSInfo
API from the snappy library to get hold of the correct path (just like we do in theio.quarkus.kafka.client.runtime.KafkaRecorder#loadSnappy()
) and thus letting that API deal with the details.P.S: This
integration-tests/kafka-client
fails even with the latest released version of GraalVM, so this isn't related to any GraalVM dev version. It looks like this native-image tests isn't enabled in our CI, which is why this wasn't caught earlier. Perhaps we should enable these tests in our CI (as a separate PR)?