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

[zakkak]

Closes #10008

[zapster]

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

[zapster]

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]

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]

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]

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]

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]

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?

[zapster]

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]

Great, thank you @zapster 🙏

[olpaw]

Small changes requested

[zapster]

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

[zapster]

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");

[zakkak]

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.

[zapster]

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.