-
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
Use new RuntimeResourceSupport public API with GraalVM >= 22.3 #27728
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
1573ce4
Drop support for "legacy" (pre 21.1) localization feature
zakkak f35f7de
Use io.quarkus.runtime.ReflectionUtil#lookupMethod
zakkak d94509a
Use new RuntimeResourceSupport public API with GraalVM >= 22.3
zakkak 85da3b5
Wrap trueBlock and falseBlock invocations in try-with-resources blocks
zakkak File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@zakkak I thought the public API is
org.graalvm.nativeimage.hosted.RuntimeResourceAccess
?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.
To my understanding they are both public APIs that will be shipped in graal-sdk.
org.graalvm.nativeimage.hosted.RuntimeResourceAccess
is part of theorg.graalvm.nativeimage.hosted
package andorg.graalvm.nativeimage.impl.RuntimeResourceSupport
is part of theorg.graalvm.nativeimage
package which are both included in graal-sdk.The reason I went with
RuntimeResourceSupport
instead ofRuntimeResourceAccess
is that the former doesn't require to specify a module for the resources, and in Quarkus we are not using modules.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. When I see
impl
in an API class it raises a red flag to me. While it's trueRuntimeResourceSupport
doesn't require a module, we should get away withClassLoader.getSystemClassLoader().getUnnamedModule()
and use theRuntimeResourceAccess
. In fact,RuntimeResourceAccess
usesRuntimeResourceSupport
which supports the idea that using the former will be a more robust solution. Also, you don't need to do theImageSingleton
handling yourself and it's in line with what we do in #27690. Your mileage may vary.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.
The tough part is not about getting the unnamed module but getting the named module for JDK classes, e.g.
quarkus/core/deployment/src/main/java/io/quarkus/deployment/steps/ResourceBundleStep.java
Line 15 in 1f87a53
Suggestions are welcome :)
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.
sun.security.util.Resources
is part ofjava.base
so how about?int.class.getModule()
The correct way is to useFoo.class.getModule()
of course, but you might have a chicken/egg problem.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 just an example, there are more such cases and while getting the module might not be that hard what makes it hard is the fact that we need to get the module in the generated native-image
Feature
. So to make use of theRuntimeResourceAccess
we need to:NativeImageResourceBundleBuildItem
to contain information about the modulegizmo
, to programmatically get that module and pass it toRuntimeResourceAccess
in the generated native-imageFeature
.Since
RuntimeResourceSupport
appears to be public API I don't see the need to do the above which will make the corresponding code even more complex.As an alternative I believe that if we really don't want to use
RuntimeResourceSupport
we should suggest the augmentation ofRuntimeResourceAccess
to support registering resources without the module name.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.
Apparently, I was wrong.
org.graalvm.nativeimage.impl
is indeed not supposed to be public API. But we already open it inquarkus/core/deployment/src/main/java/io/quarkus/deployment/steps/NativeImageFeatureStep.java
Lines 130 to 132 in 92fba0a
I will need to prepare a follow up fix for this. Thanks for questioning this and making have another look @jerboaa!
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.
@zakkak Thanks for coming back to this! I was meaning to come back to this discussion, but it dropped on the floor (for me). Yes, we should revisit this. IMHO, there might be latent bugs by not passing in Modules resources belong to.
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.
WIP PR taking care of the above: #28093