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.

Sign up for GitHub

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

Jump to bottom

[GR-60382] Handle Enable-Native-Access manifest attribute if present #10009

Merged
merged 1 commit into from
Dec 12, 2024
Merged

[GR-60382] Handle Enable-Native-Access manifest attribute if present #10009

merged 1 commit into from
Dec 12, 2024

Conversation

zakkak
Copy link
Collaborator

@zakkak zakkak commented Nov 1, 2024

Closes #10008

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Nov 1, 2024
@zakkak zakkak requested a review from zapster November 1, 2024 11:50
@zapster
Copy link
Member

zapster commented Nov 4, 2024

thanks for the contribution @zakkak. I'll have a look as soon as time allows.

@zapster
Copy link
Member

zapster commented Nov 12, 2024

Sorry for the delay, @zakkak. A lot of side tracking this week. I agree that #10008 is something we need to fix. In the current state, --enable-native-access is only about the image builder itself. At image run time, all native access check are currently disabled (aka all native access is allowed to all modules). This is about to change. I've pushed the current proposal to #10076. It is not yet fully ready, but the basics should be there. I'll need to do further testing, especially with external projects, though. Feedback from your sides is also very welcome.

That said, I believe that #10076 will break this PR, or at least partially defeat its purpose. I suggest we wait until #10076 lands and then revisit this and #10008. Would that work for you?

@zakkak
Copy link
Collaborator Author

zakkak commented Nov 12, 2024

Thank you for the update @zapster .

Would that work for you?

Yes, that's fine with us. As said in quarkusio/quarkus#44257 (comment) this is still a warning.

@zakkak
Copy link
Collaborator Author

zakkak commented Nov 12, 2024

I'll need to do further testing, especially with external projects, though. Feedback from your sides is also very welcome.

Quarkus is not using modules, so I am not sure how much we can test this on our side other than passing --enable-native-access=ALL-UNNAMED. If there is anything specific you would like me to try, let me know.

@zapster
Copy link
Member

zapster commented Nov 12, 2024

Quarkus is not using modules, so I am not sure how much we can test this on our side

That is totally fine. Out of interest, are you already passing --enable-native-access=ALL-UNNAMED to the image builder, or are do you intend to set this via the manifest attribute (#10008)?

@zakkak
Copy link
Collaborator Author

zakkak commented Nov 12, 2024

No, we currently don't do anything about it. For some cases setting it via the manifest attribute would be enough, but we will definitely need to pass --enable-native-access in other cases such as testing, dev-mode, etc.

The Quarkus issue tracking this is quarkusio/quarkus#44257.

@zakkak zakkak force-pushed the 2024-11-01-fix-10008 branch from 1924173 to e099a6c Compare December 5, 2024 14:57
@zakkak
Copy link
Collaborator Author

zakkak commented Dec 5, 2024

Hi @zapster, it appears that this PR is still relevant (after #10076 merge). I rebased it on latest master. Could you have another look or are you already working on a different fix?

@zakkak zakkak force-pushed the 2024-11-01-fix-10008 branch from e099a6c to 8fd44ef Compare December 9, 2024 10:43
@zapster zapster self-assigned this Dec 9, 2024
zapster
zapster approved these changes Dec 9, 2024
View reviewed changes
Copy link
Member

@zapster zapster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update, the change looks good now. I'll integrate it asap, but I'm still busy with post-branch-off tasks, so it could take a bit. However, I'll make sure that this is backported to the GraalVM for 24 release branch.

@zakkak
Copy link
Collaborator Author

zakkak commented Dec 9, 2024

Great, thank you @zapster 🙏

@zapster zapster changed the title Handle Enable-Native-Access manifest attribute if present [GR-60382] Handle Enable-Native-Access manifest attribute if present Dec 10, 2024
olpaw
olpaw requested changes Dec 10, 2024
View reviewed changes
Copy link
Member

@olpaw olpaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small changes requested

zapster
zapster requested changes Dec 10, 2024
View reviewed changes
Copy link
Member

@zapster zapster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there are anyways pending changes, I've added another minor suggestion.

substratevm/src/com.oracle.svm.driver/src/com/oracle/svm/driver/NativeImage.java Outdated
String enableNativeAccess = mainAttributes.getValue("Enable-Native-Access");
if (enableNativeAccess != null) {
if (!"ALL-UNNAMED".equals(enableNativeAccess)) {
NativeImage.showError("illegal value \"" + enableNativeAccess + "\" for Enable-Native-Access manifest attribute. Only ALL-UNNAMED is allowed");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
NativeImage.showError("illegal value \"" + enableNativeAccess + "\" for Enable-Native-Access manifest attribute. Only ALL-UNNAMED is allowed");
throw NativeImage.showError("illegal value \"" + enableNativeAccess + "\" for Enable-Native-Access manifest attribute. Only " + ALL_UNNAMED + " is allowed");

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, what's the point of throw here since showError never actually returns. Looking in other showError usages the use of throw doesn't seem to be consistent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It helps me to see that execution will stop here without knowing the implementation of showError. Of course one can look it up, but I find it more readable. But it is a matter of taste, mostly.

substratevm/src/com.oracle.svm.driver/src/com/oracle/svm/driver/NativeImage.java Outdated Show resolved Hide resolved
@zakkak zakkak force-pushed the 2024-11-01-fix-10008 branch from 8fd44ef to 29067f5 Compare December 11, 2024 13:56
@olpaw olpaw self-requested a review December 11, 2024 14:26
@graalvmbot graalvmbot merged commit d9578a8 into oracle:master Dec 12, 2024
13 checks passed
@zakkak zakkak deleted the 2024-11-01-fix-10008 branch December 13, 2024 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olpaw olpaw olpaw approved these changes

@zapster zapster zapster approved these changes

Assignees

@zapster zapster

Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement. redhat-interest
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Native Image] GraalVM for JDK 24 ignores manifest entry Enable-Native-Access
4 participants
@zakkak @zapster @olpaw @graalvmbot