-
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
Wrap more options in UnlockExperimentalVMOptions
#35772
Wrap more options in UnlockExperimentalVMOptions
#35772
Conversation
This comment has been minimized.
This comment has been minimized.
CI failures seem unrelated. |
e4d2263
to
67a9f00
Compare
UnlockExperimentalVMOptions
This comment has been minimized.
This comment has been minimized.
67a9f00
to
3197103
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.
We should go through all -H
options being added in any condition and evaluate whether we need to wrap them. 99% will need the wrapping with 23.1+. -H:AdditionalSecurityProviders
seems like we need an upstream issue to evaluate if that one needs a stable API option instead.
core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildStep.java
Show resolved
Hide resolved
@@ -887,7 +887,7 @@ public NativeImageInvokerInfo build() { | |||
+ " will be removed in a future Quarkus version."); | |||
} | |||
if (nativeConfig.enableVmInspection()) { | |||
nativeImageArgs.add("-H:+AllowVMInspection"); | |||
addExperimentalVMOption(nativeImageArgs, "-H:+AllowVMInspection"); |
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.
This is an old option, which should be --enable-monitoring
instead. I think since we have a minimal of 22.3
(which supports it), it would be fine to flip to that internally? Ideally, we'd deprecate the option too (if not already done).
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.
Note, however, that minimum is still 22.2, but we can probably bump it.
quarkus/core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/GraalVM.java
Line 136 in dc6ed7e
public static final Version MINIMUM = VERSION_22_2_0; |
Ideally, we'd deprecate the option too (if not already done).
It's already deprecated.
That's what I tried to do here :)
Yes, if that's needed in production for some users we should. |
OK, sorry. Here is what I see (without this patch). Are
I think it's referenced in some guides as well, so we should open a discussion. That was their ask at the last community meeting. |
That option has no effect in >= 23.1, and Quarkus won't pass it to native-image.
Similarly here, this was used to retrieve build time information in 22.3. Starting with 23.0 this info is included in the build output json file so there is no need to generate extra files. The rest, appear to be referenced by documentation. Not sure if it's a good idea to spam |
Regarding Update: upstream issue oracle/graal#7369 |
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.
OK.
Thank you! |
✔️ The latest workflow run for the pull request has completed successfully. It should be safe to merge provided you have a look at the other checks in the summary. |
Complementary to #35379
Closes #35788