8303884: jlink --add-options plugin does not allow GNU style options to be provided

[YaSuenag]

We cannot pass GNU style options like --enable-preview to jlink --add-option. It is hard to use for complex application.

We have workaround for this issue (see JBS), but I think it is better to fix on JDK side.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Warning

 ⚠️ Found leading lowercase letter in issue title for 8303884: jlink --add-options plugin does not allow GNU style options to be provided

Issue

• JDK-8303884: jlink --add-options plugin does not allow GNU style options to be provided (Bug - P3)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19987/head:pull/19987
$ git checkout pull/19987

Update a local copy of the PR:
$ git checkout pull/19987
$ git pull https://git.openjdk.org/jdk.git pull/19987/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19987

View PR using the GUI difftool:
$ git pr show -t 19987

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19987.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back ysuenaga! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@YaSuenag The following label will be automatically applied to this pull request:

• core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 00: Full (ea8fc556)

[jaikiran]

Hello @YaSuenag, I haven't had a chance to build your change locally and try it myself, but I suspect this change isn't enough to address the issue. Does this change allow for:

jlink ... --add-options --add-exports java.base/jdk.internal.misc=ALL-UNNAMED

to work correctly and have the --add-exports java.base/jdk.internal.misc=ALL-UNNAMED be considered as the option value for --add-options.

Additionally, do the current tests pass with your proposed change?

[YaSuenag]

Hello @YaSuenag, I haven't had a chance to build your change locally and try it myself, but I suspect this change isn't enough to address the issue. Does this change allow for:

jlink ... --add-options --add-exports java.base/jdk.internal.misc=ALL-UNNAMED

It would not work.
--add-exports java.base/jdk.internal.misc=ALL-UNNAMED should be an one option for --add-options, so it should be quoted.

"--add-exports java.base/jdk.internal.misc=ALL-UNNAMED" would NOT be worked, but --add-exports=java.base/jdk.internal.misc=ALL-UNNAMED would be worked because option parsar is different between java and jlink'ed app. In java, --add-options would be parsed in src/java.base/share/native/libjli/java.c, but in jlink'ed app, it would be parsed in src/hotspot/hotspot/share/runtime/arguments.cpp because command line options are pick-upped from vmoptions in modules file. Then only --add-exports= would be handled in HotSpot - so you cannot specify --add-exports [module] for jlink'ed app. You have to specify --add-exports=[module].

Additionally, do the current tests pass with your proposed change?

I tested this patch with test/jdk/tools/jlink jtreg tests, and all of tests were passed on my Linux x64 of course.

[AlanBateman]

We cannot pass GNU style options like --enable-preview to jlink --add-option. It is hard to use for complex application.

JDK-8303884 was created to track a much larger re-examination of the jlink option parameter. I think the issue will need more thought before deciding whether to put in a short term fix as proposed here (the main concern is the side effect on plugins).

[YaSuenag]

So how should we proceed this? This problem is critical for some modularized applications as I said before.

I agree that we need to consider the approach for this, but it is worth to provide the fix even if it is short-term, I think. I believe we can ensure not to break current behavior with jtreg tests.

[jaikiran]

Hello @YaSuenag, like Alan noted, I believe the options parsing for jlink will need a bigger change. The current proposed change has the potential to have unexpected side effects where an option that wasn't expected to be passed to a plugin for parsing, might now end up being passed to the plugin. The fact that the current tests aren't catching that issue is also a sign that we might need additional test coverage in this area.

This problem is critical for some modularized applications as I said before.

Do you have an example jlink command line where you are running into problems with the --add-options? As far as I know, there are workarounds to get --add-options to behave correctly and be able to pass along the expected values in the generated image. Depending on your example, we might be able to suggest a workaround until the options parsing is reworked.

[YaSuenag]

@jaikiran
This is the case what I want to do.
https://github.com/YaSuenag/perfreader/blob/4844ac54b375d73b4ee981a90cd3b3b3e0245ab6/pom.xml#L71-L76

<argument>--add-options</argument>
<!--
    add-option does not accept argument which stats with '\-\-',
    so HeapDumpOnOutOfMemoryError is added to the top of args to avoid it.
-->
<argument>-XX:+HeapDumpOnOutOfMemoryError --add-exports=jdk.internal.jvmstat/sun.jvmstat.monitor=perfreader --add-exports=jdk.internal.jvmstat/sun.jvmstat.perfdata.monitor=perfreader</argument>

We cannot pass --add-exports straightly as you know, but we can avoid the issue to add the option which starts with - to the top of args (it is similar with the workaround which is shown in JDK-8303884). So I've added -XX:+HeapDumpOnOutOfMemoryError, but it is ugly a bit I think.

[bridgekeeper]

@YaSuenag This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@YaSuenag This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

[slowhog]

The proposed fix is correct in my opinion, but there is a chance that user mistake could lead to missing of desired arguments.
#22526 keeps the current behavior to error on ambiguous situation but allow user to explicitly express argument value with = or quote with multiple values as originally designed,