Skip to content

chore: set Java header at native image runtime #2069

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

Merged
merged 8 commits into from
May 12, 2022
Merged

Conversation

mpeddada1
Copy link
Contributor

This contributes towards removing the use of the SVM dependency for native image compilation. This code makes use of the org.graalvm.nativeimage.imagecode system property instead of Substitutions to set the Java version at image runtime. For reference: https://www.graalvm.org/sdk/javadoc/index.html?constant-values.html

See reproducer: https://github.com/mpeddada1/avoid-substitutions

@mpeddada1 mpeddada1 requested a review from a team as a code owner May 10, 2022 02:32
@product-auto-label product-auto-label bot added the size: s Pull request size is small. label May 10, 2022
@mpeddada1 mpeddada1 requested review from suztomo and meltsufin May 10, 2022 14:14
@mpeddada1 mpeddada1 changed the title chore: set Java header during native image runtime chore: set Java header at native image runtime May 10, 2022
@@ -164,6 +165,16 @@ static class ApiClientVersion {
}

public String toString() {
// When running the application as a native image, append `-graalvm` to the
// version.
String imageCode = System.getProperty("org.graalvm.nativeimage.imagecode");
Copy link
Member

Choose a reason for hiding this comment

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

Please use constants instead of string literals here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a valid point! However, this might come down to the question of whether it is worth having dependency on graal-sdk in this repo? If we use the latest version (22.1.0) then we'll see the following incompatibility:

bad class file: /home/runner/.m2/repository/org/graalvm/sdk/graal-sdk/22.1.0/graal-sdk-22.1.0.jar(org/graalvm/nativeimage/ImageInfo.class)
Error:      class file has wrong version 55.0, should be 52.0
Error:      Please remove or make sure it appears in the correct subdirectory of the classpath.
Error:  -> [Help 1]

In which case, we may need to do something similar to googleapis/gax-java#1671. This could still pose a slight risk since we still target Java 7. But open to hearing about what you think!

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with either. @suztomo what do you think? Is it worth it to bring the whole setup for compiling with 11 and testing on 7 in this case?

@mpeddada1 mpeddada1 merged commit ea078f8 into main May 12, 2022
@mpeddada1 mpeddada1 deleted the graalvm-version branch May 12, 2022 17:01
blakeli0 pushed a commit that referenced this pull request Dec 16, 2022
* chore: set Java header during native image runtime
blakeli0 pushed a commit that referenced this pull request Dec 16, 2022
* chore: set Java header during native image runtime
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: s Pull request size is small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants