Skip to content
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 4 commits into from Sep 6, 2022

Conversation

zakkak
Copy link
Contributor

@zakkak zakkak commented Sep 5, 2022

@quarkus-bot quarkus-bot bot added the area/core label Sep 5, 2022
The minimum supported GraalVM version is 21.3
Reduces the complexity of the NativeImageFeatureStep by invoking the
equivalent helper method.
Drops dependency in GraalVM internal API
@zakkak zakkak force-pushed the legalization-feature-updates branch from ae5f452 to 85da3b5 Compare September 5, 2022 13:49
@zakkak zakkak requested review from gsmet and geoand and removed request for gsmet September 5, 2022 13:50
@zakkak zakkak added this to the 2.13 - main milestone Sep 5, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Sep 5, 2022

Failing Jobs - Building 85da3b5

Status Name Step Failures Logs Raw logs
Gradle Tests - JDK 11 Set up JDK 11 ⚠️ Check → Logs Raw logs
✔️ Gradle Tests - JDK 11 Windows

@zakkak zakkak merged commit d84e728 into quarkusio:main Sep 6, 2022
@zakkak zakkak deleted the legalization-feature-updates branch September 6, 2022 06:17
static final String LOCALIZATION_FEATURE = "com.oracle.svm.core.jdk.localization.LocalizationFeature";
static final String RUNTIME_RESOURCE_SUPPORT = "org.graalvm.nativeimage.impl.RuntimeResourceSupport";
Copy link
Contributor

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?

Copy link
Contributor Author

@zakkak zakkak Sep 9, 2022

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 the org.graalvm.nativeimage.hosted package and org.graalvm.nativeimage.impl.RuntimeResourceSupport is part of the org.graalvm.nativeimage package which are both included in graal-sdk.

The reason I went with RuntimeResourceSupport instead of RuntimeResourceAccess is that the former doesn't require to specify a module for the resources, and in Quarkus we are not using modules.

Copy link
Contributor

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 true RuntimeResourceSupport doesn't require a module, we should get away with ClassLoader.getSystemClassLoader().getUnnamedModule() and use the RuntimeResourceAccess. In fact, RuntimeResourceAccess uses RuntimeResourceSupport which supports the idea that using the former will be a more robust solution. Also, you don't need to do the ImageSingleton handling yourself and it's in line with what we do in #27690. Your mileage may vary.

Copy link
Contributor Author

@zakkak zakkak Sep 9, 2022

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.

return new NativeImageResourceBundleBuildItem("sun.security.util.Resources");

Suggestions are welcome :)

Copy link
Contributor

@jerboaa jerboaa Sep 9, 2022

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 of java.base so how about? int.class.getModule() The correct way is to use Foo.class.getModule() of course, but you might have a chicken/egg problem.

Copy link
Contributor Author

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 the RuntimeResourceAccess we need to:

  1. Extend NativeImageResourceBundleBuildItem to contain information about the module
  2. Generate the necessary code, using gizmo, to programmatically get that module and pass it to RuntimeResourceAccess in the generated native-image Feature.

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 of RuntimeResourceAccess to support registering resources without the module name.

Copy link
Contributor Author

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 in

// required in order to access org.graalvm.nativeimage.impl.RuntimeSerializationSupport and org.graalvm.nativeimage.impl.ConfigurationCondition
features.produce(
new JPMSExportBuildItem("org.graalvm.sdk", "org.graalvm.nativeimage.impl", GraalVM.Version.VERSION_22_1_0));
which led me to thinking it's accessible by default.

I will need to prepare a follow up fix for this. Thanks for questioning this and making have another look @jerboaa!

Copy link
Contributor

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.

Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants