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

slowhog · 2024-12-03T19:59:56Z

Improving option value handling to support passing argument value starts with "--".

Before the fix, in following example, --add-modules will be considered as another option for JLink instead of argument value for --add-options.
--add-options --add-modules=jdk.incubator.concurrent
--add-options=--add-modules=jdk.incubator.concurrent

will cause JLink to report
Error: no value given for --add-options
as --add-modules is considered another option for JLink.

After the fix, by using = will ensure the value is properly handled as argument value
--add-options=--add-modules=jdk.incubator.concurrent

Progress

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

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)

Reviewers

Yasumasa Suenaga (@YaSuenag - Reviewer)
Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22526

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

Using diff file

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

Using Webrev

Link to Webrev Comment

…to be provided

bridgekeeper · 2024-12-03T20:01:05Z

👋 Welcome back henryjen! 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 · 2024-12-03T20:01:28Z

@slowhog This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: ysuenaga, alanb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 131 new commits pushed to the master branch:

0ee6ba9: 8347596: Update HSS/LMS public key encoding
ec2aaaa: 8326236: assert(ce != nullptr) failed in Continuation::continuation_bottom_sender
02d2493: 8347613: Remove leftover doPrivileged call in Currency test: CheckDataVersion.java
10d08db: 8346142: [perf] scalability issue for the specjvm2008::xml.validation workload
9b1bed0: 8290043: serviceability/attach/ConcAttachTest.java failed "guarantee(!CheckJNICalls) failed: Attached JNI thread exited without being detached"
2de71d0: 8347129: cpuset cgroups controller is required for no good reason
4c30933: 8346971: [ubsan] psCardTable.cpp:131:24: runtime error: large index is out of bounds
06ff4c1: 8347146: Convert IncludeLocalesPluginTest to use JUnit
db76f47: 8347720: [BACKOUT] Portable implementation of FORBID_C_FUNCTION and ALLOW_C_FUNCTION
e6902cf: 8323740: java.lang.ExceptionInInitializerError when trying to load XML classes in wrong order
... and 121 more: https://git.openjdk.org/jdk/compare/ddb58819640dc8f1930d243d6eb07ce88ef79b22...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-12-03T20:02:50Z

@slowhog 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 · 2024-12-03T20:06:21Z

Webrevs

YaSuenag

Looks good.
Thank you for working on this issue!

AlanBateman · 2024-12-05T11:02:24Z

/reviewers 2

openjdk · 2024-12-05T11:03:35Z

@AlanBateman
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/TaskHelper.java

AlanBateman

I was initially skeptical that this would be a small change, I had expected it would require significant re-working of the CLI code. Happy to be proved wrong.

mlchung · 2024-12-03T22:23:11Z

test/jdk/tools/jlink/TaskHelperTest.java

@@ -0,0 +1,148 @@
+/*
+ * Copyright (c) 2024, 2024, Oracle and/or its affiliates. All rights reserved.


Hi @mlchung, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user mlchung" for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

I agree to the OpenJDK Terms of Use for all comments I make in a project in the OpenJDK GitHub organization.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

mlchung · 2024-12-03T22:43:17Z

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/TaskHelper.java

+                        }
+                        if (potentiallyGnuOption && param.length() >= 2 &&
+                            param.charAt(0) == '-' && param.charAt(1) == '-' &&
+                            !param.contains(" ")) {


Hi @mlchung, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user mlchung" for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

I agree to the OpenJDK Terms of Use for all comments I make in a project in the OpenJDK GitHub organization.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

--style argument still could be a real jlink argument, better check would be to see if that's a valid jlink option rather than just "--".
The other alternative would be like what proposed by @YaSuenag, simply take the next token as value.

Hi @mlchung, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user mlchung" for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

I agree to the OpenJDK Terms of Use for all comments I make in a project in the OpenJDK GitHub organization.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

I agree it's a little bit awkward, unfortunately we cannot look at the quotation as that had been decoded by shell.

Just take next token as value would be simpler, see the origin PR from @YaSuenag at #19987.

The other alternative would be that, we can check next token against known options of jlink, and treat it as value without match. That would limited the potential ambiguous conflicts but would be even more mysterious to user?

After the update, = must be used if value is prefixed with --.
--add-options=--add-modules=jdk.incubator.concurrent will work and --add-options="--add-modules=jdk.incubator.concurrent --module-path xx" will work, but not -add-options "--add-modules=jdk.incubator.concurrent --module-path xx"`.

mlchung · 2024-12-03T22:45:27Z

test/jdk/tools/jlink/TaskHelperTest.java

+            { "--raw-arg-plugin=--main-no-arg", "--main-no-arg" },
+            { "--raw-arg-plugin", "--main-no-arg --list", "--main-no-arg"},
+            { "--raw-arg-plugin", " --main-no-arg", "--main-no-arg" },
+        };


Hi @mlchung, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user mlchung" for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

I agree to the OpenJDK Terms of Use for all comments I make in a project in the OpenJDK GitHub organization.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

Line 126 validate the argument is processed.

Hi @mlchung, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user mlchung" for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

I agree to the OpenJDK Terms of Use for all comments I make in a project in the OpenJDK GitHub organization.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

bridgekeeper · 2025-01-02T18:09:13Z

@slowhog 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!

mlchung · 2025-01-09T18:04:36Z

Hi @mlchung, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user mlchung" for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

I agree to the OpenJDK Terms of Use for all comments I make in a project in the OpenJDK GitHub organization.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

AlanBateman · 2025-01-10T10:35:33Z

Good, I like this too. I agree with Mandy on documenting this. Also to avoid developer flapping around trying to figure this out then maybe we need a better err.missing.arg resource when potentiallyGnuOption and the param starts with "--". In otherwise, could be just given a better error message?

slowhog · 2025-01-11T02:47:20Z

I prefer to have a better error message which tell users what to do. Would something like following work?

$  jlink --add-modules java.base --add-options --help                                                                   
Error: value for option --add-options starts with "--" should use --add-options=<value> format

$ jlink --add-modules --help --add-options --help                                                                      
Error: value for option --add-modules starts with "--" should use --add-modules=<value> format

AlanBateman

The update looks good, I assume you'll bump the copyright date of the files before integrating.

YaSuenag

It's better, thanks!

slowhog · 2025-01-14T23:08:22Z

/integrate

openjdk · 2025-01-14T23:08:56Z

@slowhog This pull request has not yet been marked as ready for integration.

slowhog · 2025-01-14T23:10:05Z

@YaSuenag Will need you to re-approve the change, thanks.

slowhog · 2025-01-14T23:48:56Z

/integrate

openjdk · 2025-01-14T23:49:17Z

Going to push as commit d6d45c6.
Since your change was applied there have been 131 commits pushed to the master branch:

0ee6ba9: 8347596: Update HSS/LMS public key encoding
ec2aaaa: 8326236: assert(ce != nullptr) failed in Continuation::continuation_bottom_sender
02d2493: 8347613: Remove leftover doPrivileged call in Currency test: CheckDataVersion.java
10d08db: 8346142: [perf] scalability issue for the specjvm2008::xml.validation workload
9b1bed0: 8290043: serviceability/attach/ConcAttachTest.java failed "guarantee(!CheckJNICalls) failed: Attached JNI thread exited without being detached"
2de71d0: 8347129: cpuset cgroups controller is required for no good reason
4c30933: 8346971: [ubsan] psCardTable.cpp:131:24: runtime error: large index is out of bounds
06ff4c1: 8347146: Convert IncludeLocalesPluginTest to use JUnit
db76f47: 8347720: [BACKOUT] Portable implementation of FORBID_C_FUNCTION and ALLOW_C_FUNCTION
e6902cf: 8323740: java.lang.ExceptionInInitializerError when trying to load XML classes in wrong order
... and 121 more: https://git.openjdk.org/jdk/compare/ddb58819640dc8f1930d243d6eb07ce88ef79b22...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-01-14T23:49:23Z

@slowhog Pushed as commit d6d45c6.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

slowhog added 2 commits December 3, 2024 11:55

8303884: jlink --add-options plugin does not allow GNU style options …

fee6e65

…to be provided

add test

70b5f9f

openjdk bot added the rfr Pull request is ready for review label Dec 3, 2024

openjdk bot added the core-libs core-libs-dev@openjdk.org label Dec 3, 2024

slowhog mentioned this pull request Dec 3, 2024

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

Closed

3 tasks

YaSuenag approved these changes Dec 5, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 5, 2024

openjdk bot removed the ready Pull request is ready to be integrated label Dec 5, 2024

AlanBateman reviewed Dec 5, 2024

View reviewed changes

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/TaskHelper.java Outdated Show resolved Hide resolved

AlanBateman reviewed Dec 5, 2024

View reviewed changes

Fix style

7dbc99f

mlchung reviewed Dec 5, 2024

View reviewed changes

slowhog added 2 commits January 7, 2025 15:57

Use opt=value format when value is starting with --

1274931

Merge remote-tracking branch 'openjdk/master' into JDK-8303884

7060e47

Use different error message for ambiguous gnu-style options as value

76ce447

AlanBateman approved these changes Jan 13, 2025

View reviewed changes

YaSuenag approved these changes Jan 14, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 14, 2025

Update copyright year

50a8c5e

openjdk bot removed the ready Pull request is ready to be integrated label Jan 14, 2025

AlanBateman approved these changes Jan 14, 2025

View reviewed changes

YaSuenag approved these changes Jan 14, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 14, 2025

openjdk bot added the integrated Pull request has been integrated label Jan 14, 2025

openjdk bot closed this Jan 14, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 14, 2025

slowhog deleted the JDK-8303884 branch January 15, 2025 00:09

		@@ -0,0 +1,148 @@
		/*
		* Copyright (c) 2024, 2024, Oracle and/or its affiliates. All rights reserved.

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

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

Uh oh!

Conversation

slowhog commented Dec 3, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Warning

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Dec 3, 2024

Uh oh!

openjdk bot commented Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Dec 3, 2024

Uh oh!

mlbridge bot commented Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

YaSuenag left a comment

Choose a reason for hiding this comment

Uh oh!

AlanBateman commented Dec 5, 2024

Uh oh!

openjdk bot commented Dec 5, 2024

Uh oh!

Uh oh!

AlanBateman left a comment

Choose a reason for hiding this comment

Uh oh!

mlchung Dec 3, 2024 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlchung Dec 3, 2024 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slowhog Dec 5, 2024

Choose a reason for hiding this comment

Uh oh!

mlchung Dec 5, 2024 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slowhog Jan 2, 2025

Choose a reason for hiding this comment

Uh oh!

slowhog Jan 8, 2025

Choose a reason for hiding this comment

Uh oh!

mlchung Dec 3, 2024 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slowhog Dec 5, 2024

Choose a reason for hiding this comment

Uh oh!

mlchung Dec 5, 2024 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bridgekeeper bot commented Jan 2, 2025

Uh oh!

mlchung commented Jan 9, 2025 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlanBateman commented Jan 10, 2025

Uh oh!

slowhog commented Jan 11, 2025

Uh oh!

AlanBateman left a comment

Choose a reason for hiding this comment

Uh oh!

YaSuenag left a comment

Choose a reason for hiding this comment

slowhog commented Dec 3, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Dec 3, 2024 •

edited

Loading

mlbridge bot commented Dec 3, 2024 •

edited

Loading

mlchung Dec 3, 2024 •

edited by bridgekeeper bot

Loading

mlchung Dec 3, 2024 •

edited by bridgekeeper bot

Loading

mlchung Dec 5, 2024 •

edited by bridgekeeper bot

Loading

mlchung Dec 3, 2024 •

edited by bridgekeeper bot

Loading

mlchung Dec 5, 2024 •

edited by bridgekeeper bot

Loading

mlchung commented Jan 9, 2025 •

edited by bridgekeeper bot

Loading