-
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
Support both GraalVM 19.2.1 and 19.3.1 #6574
Support both GraalVM 19.2.1 and 19.3.1 #6574
Conversation
JNI is currently enabled no matter which GraalVM version is used. Should we try to disable it when possible? I don't know if this can have an impact on the native image size with I'd like to run some tests again before this is merged, I didn't have enough time to test everything I wanted tonight. |
// The following line is required to throw an exception and make sure we load the 19.2.1 class when needed. | ||
workaroundTryBlock.invokeVirtualMethod( | ||
ofMethod(Class.class, "getDeclaredMethod", Method.class, String.class, Class[].class), locClass, | ||
workaroundTryBlock.load("addBundleToCache"), | ||
workaroundTryBlock.marshalAsArray(Class.class, workaroundTryBlock.loadClass(String.class))); |
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.
I tried to find a simpler expression to force the exception throw here, but I didn't find anything that worked. I'm sure this expression could be much shorter...
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.
I think it's just fine. You are just trying to make sure the method exists right? It should be perfectly OK and in any case it should be temporary
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.
Actually, I only need to check if the com.oracle.svm.core.jdk.LocalizationFeature
class is available, I'm not really interested in the method itself. But invoking a simple getName
from LocalizationFeature
wasn't enough to force an exception throw. That's not really a problem since this code is only a copy of the original code a few lines below and it is also temporary, but that's still not pretty :)
Something seems to be wrong with CI:
|
It would be nice to force JNI only for 19.3. We don't need it to be as pretty as the patch you worked on. Something like: We can refine with your other patch once we only support GraalVM 19.3. |
It also means that the default value in NativeConfig needs to be false |
I've seen that, but that's a weird issue, I don't know where it is coming from. There isn't a single |
Yeah it is weird, especially given the fact that the PR built for me locally... |
Thanks for checking! Everything is working fine on my computer too. I'll update the PR with the JNI changes in a couple hours, we'll see if CI is happier then. |
Great, thanks a lot! |
50abff4
to
dc2ff88
Compare
JNI is now disabled by default with GraalVM CI is still unhappy with the PR... @gsmet have you already seen the following kind of error before?
There's absolutely no trace of |
You have to rebase and fix the artifact in the narayana-stm extension. I did it but I can't force push to your branch so you'll have to do it yourself :). CI applies your PR on top of master. |
2621c83
to
d68ff6a
Compare
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.
I added a few comments.
The last commit need to be squashed into the one already changing the artifacts.
core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildStep.java
Outdated
Show resolved
Hide resolved
core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildStep.java
Show resolved
Hide resolved
extensions/jdbc/jdbc-h2/runtime/src/main/java/io/quarkus/jdbc/h2/runtime/graal/Engine.java
Show resolved
Hide resolved
147e8e4
to
2d68b72
Compare
There's a small issue visible in #6594 that needs to be fixed before this PR is merged:
|
As discussed with @gsmet, once this is in we should setup some simple GitHub Actions pipeline to ensure that things work properly for both GraalVM versions |
Let's merge that one and make progress from there. Additional 19.3.1 issues can be fixed later, they are independent from this PR. |
@gwenneg will do, thanks |
Sorry to bother. I tried to use 1.2.0.CR1. I have small maven project which uses quarkus. and I just wanted to compile it with graalvm 19.3 but it did not work I have changed quarkus.platform.version and ran mvn install just to see if build works but failed. with if you dont mind can you tell me how to test new CRs ? or better just point me to an example repo where you test CR1 :) I can copy from there |
The |
thank you very much and here is result Error: Unknown arguments: 100, io.quarkus.vertx.core.runtime.VertxLogDelegateFactory, true, DISABLED, 1, com.oracle.svm.core.genscavenge.CollectionPolicy$BySpaceAndTime, 1, 0, http, quarkus-command-runner-1.0-SNAPSHOT-runner` |
@ozkanpakdil Maybe you can ask on the Quarkus Zulip chat? |
@ozkanpakdil My answer won't be directly related to your issue, but please note that GraalVM If you still can't generate your native image with GraalVM |
@ozkanpakdil: I got it working with a combination of these changes: <quarkus-plugin.version>1.2.0.Final</quarkus-plugin.version>
<quarkus.platform.artifact-id>quarkus-bom</quarkus.platform.artifact-id>
<quarkus.platform.version>1.2.0.CR1</quarkus.platform.version>
Use Set
I'm assuming the 1.2.0.Final release is imminent, perhaps even in progress as we speak, and that's why we're having to cobble a few things together for the moment. |
Fixes #6483