-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Move to GraalVM 19 #2875
Move to GraalVM 19 #2875
Conversation
|
core/creator/src/main/java/io/quarkus/creator/phase/nativeimage/NativeImagePhase.java
Show resolved
Hide resolved
@@ -29,7 +34,17 @@ | |||
|
|||
public class SubstrateAutoFeatureStep { | |||
|
|||
private static final String GRAAL_AUTOFEATURE = "io.quarkus/runner/AutoFeature"; | |||
private static final String GRAAL_AUTOFEATURE = "io/quarkus/runner/AutoFeature"; |
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.
Oh WOW, we're replacing a partly binary class name. Can we introduce a convension for constant class names that lets us know if it's a class name, or a class binary name ('.' or '/')?
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.
TBH it's a complete mess. We have APIs that accept either one, APIs that require one or the other, and APIs that accept one and produce the other. With no documentation of course.
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 know, I'm just saying we should once and for all decide on a suffix/terminology for these constants. I've the same problem in my extensions.
farray); | ||
} | ||
} | ||
CatchBlockCreator cc = tc.addCatch(Throwable.class); | ||
//cc.invokeVirtualMethod(ofMethod(Throwable.class, "printStackTrace", void.class), cc.getCaughtException()); | ||
cc.invokeVirtualMethod(ofMethod(Throwable.class, "printStackTrace", void.class), cc.getCaughtException()); |
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 this meant to stay, or a debug thing that should stay commented out ?
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 don't know really. It does produce an exception from Jackson. But I think that might be a legitimate problem.
The native image failures are expected as CI isn't running 19.0.2 yet. |
This is waiting for @cescoffier to create a docker image. |
@dmlloyd there is a conflict now, can you rebase? |
Rebased. FYI I have no idea how to do anything with the Azure pipelines; we might have to ping @n1hility for that. |
OK, I think we can take care of it. My current understanding is that changing the image would be a code update. Not yet sure if we have to update the pipelines. I'll check that on Monday once @cescoffier has prepared the image. |
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.
@dmlloyd I added a few comments/questions. Could you take care of them?
Thanks!
core/creator/src/main/java/io/quarkus/creator/phase/nativeimage/NativeImagePhase.java
Show resolved
Hide resolved
core/creator/src/main/java/io/quarkus/creator/phase/nativeimage/NativeImagePhase.java
Outdated
Show resolved
Hide resolved
All fixed @gsmet |
I remember asking here #1602 (comment) about the quarkus/core/creator/src/main/java/io/quarkus/creator/phase/nativeimage/NativeImagePhase.java Lines 515 to 528 in 688ee16
. Is this the right moment to deal with the comment? If not, should we update the code in question to take into consideration 19.0.2 ?
|
Let's address it in a follow-up PR. |
Fixes #2792, fixes #2412, fixes #2563, fixes #2438, fixes #2428, fixes #2410, fixes #2876.