-
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
Upgrade GraalVM to 19.3.0 #5358
Upgrade GraalVM to 19.3.0 #5358
Conversation
How does this relate to #5353 ? |
This PR could be considered as a follow-up of #5353. |
ce0f04a
to
b7435ba
Compare
I'm changing this PR a bit since GraalVM @cescoffier Could you please create the
Edit: Actually, I'm not sure we need to wait for the JDK 11 native tests to merge this PR. I'll probably add them in a separate PR. |
b7435ba
to
1fcbd22
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.
Yeah, don't rush on adding the native image JDK 11 tests. I think we need to define our strategy regarding JDK 11 support and I would probably make it a separate PR once this one is in.
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
506a6a0
to
487ba9d
Compare
81ef322
to
5d6b945
Compare
PR promoted and ready to be reviewed/merged as soon as:
|
Starting working on the docker image, I keep you posted. |
5d6b945
to
80b1b9f
Compare
397db6b
to
eaffb98
Compare
Most native builds failed with an error I never saw before:
|
I needed to remove some of the substitutions to get the keycloak tests working. Basically what was happening is that static init methods were registering providers, that were then causing problems for native image. I just moved all this to runtime, which seems to have fixed it (at least in my local testing). BTW @gwenneg I noticed that you mentioned you needed to recompile graal to figure out what was causing the problem. Not sure if you are aware of this but if you build with '-Dquarkus.native.debug-build-process=true' then you can just attach a debugger and set a breakpoint to see what is going on without needing a custom graal build. I figured this issue out with a breakpoint at java.security.Security#insertProviderAt to see where the providers were being registered. |
Does that work with the community edition of GraalVM? I thought the enterprise edition was needed for debugging. I'm still new to GraalVM so there are probably many things (like this one) I don't know yet :) Thanks for the tip! |
Yea, native-image is just a standard java program (as in the actual build process), you can just attach a debugger as normal and debug the build process itself. This does not let you actually debug the resulting program though. |
93a4422
to
763797a
Compare
core/deployment/src/main/java/io/quarkus/deployment/ThreadLocalRandomProcessor.java
Show resolved
Hide resolved
The Quarkus doc needs to be updated, I'll do it today. |
Thanks @stuartwdouglas for your help with the NPE issue! |
Remove some problematic substitutions and make bouncycastle classes initialized at runtime to work around security provider bug
763797a
to
85d8746
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.
Awesome team work, thanks.
I think we need some documentation work:
- document how to use GraalVM JDK 11 tech preview if possible, either with Docker or standard install
- see if the SSL documentation needs some updates. From my understanding, it might not be needed anymore to include the SunEC library. We need to properly check that.
@gwenneg do you think you will have the time to do it soon?
core/deployment/src/main/java/io/quarkus/deployment/ThreadLocalRandomProcessor.java
Show resolved
Hide resolved
@@ -132,7 +132,7 @@ | |||
/** | |||
* The docker image to use to do the image build | |||
*/ | |||
@ConfigItem(defaultValue = "quay.io/quarkus/ubi-quarkus-native-image:19.2.1") | |||
@ConfigItem(defaultValue = "quay.io/quarkus/ubi-quarkus-native-image:19.3.0-java8") |
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.
If we plan to support JDK 11, we need to document how to do it.
And we should at least try to run the tests manually once to check it's OK.
@gwenneg I let you the honor to push the Merge button, considering how much effort you put into that one. Have fun :). |
I'm running tests with both JDK to check everything's OK now. Please don't merge this PR until I'm done. Edit: @gsmet: Your comment wasn't visible when I wrote mine, yes I'll push the button :) |
The JDK 11 native tests are still running locally, I'll merge the PR tomorrow morning once I've checked the results. I'll also take care of the documentation tomorrow night with a dedicated PR. |
You rock, my friend!
…On Wed, Dec 4, 2019 at 7:33 PM Gwenneg Lepage ***@***.***> wrote:
The JDK 11 native tests are still running locally, I'll merge the PR
tomorrow morning once I've checked the results.
I'll also take care of the documentation tomorrow night with a dedicated
PR.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5358?email_source=notifications&email_token=AAAD4T7AUPY2YJMEUJKMASTQXBD6NA5CNFSM4JLLNSU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEF7BJ6Y#issuecomment-561911035>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAD4TYV3ARF557EKG22LT3QXBD6NANCNFSM4JLLNSUQ>
.
|
Comment removed, wrong tests conditions. |
I'm merging this PR as it feels we're ready for GraalVM 19.3.0 with JDK 8. There are still some issues I need to investigate with JDK 11 though, I will continue this work from #5682. Thanks a lot everyone for your help with this PR! |
Thanks for your awesome work on this @gwenneg! |
Great stuff @gwenneg !!! |
Awesome work @gwenneg!! |
This PR is related to #4218
It is incomplete and not meant to be merged for now. The build will most probably fail until the GraalVM 19.3.0 dependency is available and integrated into Quarkus.