8333727: Use JOpt in jpackage to parse command line

[alexeysemenyukoracle]

Use "JOpt Simple" from jdk.internal.joptsimple package to parse jpackage command line.

All command-line parsing code is placed in a new "jdk.jpackage.internal.cli" package with 92% unit test coverage.

Error reporting improved

1. In case of multiple command-line errors, all are reported, unlike previously, only the first one was reported.

Command line (Windows):

jpackage --linux-shortcut --mac-package-name foo -p m1 --linux-menu-group grp -p m2 --app-image dir

Old error output:

Error: Option [--linux-shortcut] is not valid on this platform

New error output:

Error: Option [--linux-shortcut] is not valid on this platform
Error: Option [--mac-package-name] is not valid on this platform
Error: Option [-p] is not valid with type [exe]
Error: Option [--linux-menu-group] is not valid on this platform

2. Fix misleading error messages.

Command line (Windows):

jpackage --input no --main-jar no.jar

Old error output:

jdk.jpackage.internal.model.ConfigException: The configured main jar does not exist no.jar in the input directory

New error output:

The value "no" provided for parameter --input is not a directory

Help output fixed

Options in the original help output were out of order. On macOS, options were placed in wrong sections. There were trailing whitespaces.

The old help output is captured in the cd7bca2 commit.

The reordered and filtered old help output is captured in the 10dc379 commit.

Help output in this PR is captured in the 58c2d94 commit. Use it to see the diff between the new and old filtered and reordered help output.

Functional changes

Old tool provider implementation jdk.jpackage.internal.JPackageToolProvider had lousy thread-safety protection that didn't work when multiple instances of jpackage tool provider are created and invoked asynchronously. This patch fixes this issue. It is safe to invoke the same jpackage tool provider instance asynchronously, and also safe to invoke multiple instances of jpackage tool provider.

Like other JDK tools, jpackage will print help output if the -? option is specified on the command line.

In addition to --option-name <value> variant to set option value, supports --option-name=<value>, -option-name=<value>, -option-name <value> variants - courtesy of jdk.internal.joptsimple package.

Misc

All "params"-specific code removed.

Replaced the checked PackagerException exception with the new unchecked JPackageException exception. Converted ConfigException to an unchecked exception.

The format of the .jpackage.xml file has changed:

• The name of the main launcher is stored as the @name attribute of the /jpackage-state/main-launcher element.
• Child elements of the /jpackage-state element with the values of the main launcher properties are stored as child elements of the /jpackage-state/main-launcher element.
• "launcher-as-service" property of an additional launcher is stored as a text value of the /jpackage-state/add-launcher/service element instead of the value of the /jpackage-state/add-launcher/@service attribute.

Changes to the format of the .jpackage.xml file are to simplify handling of properties of additional and the main launcher.

Autogenerated summary of all recognized options (jdk/test/jdk/tools/jpackage/junit/share/jdk.jpackage/jdk/jpackage/internal/cli/jpackage-options.md).

Fixes JDK-8371384 as a side effect.

Testing

The same testing as in the #19668 PR. Additionally, verified no unexpected diffs in the test logs after running all jpackage tests.

---

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
• Change requires CSR request JDK-8371447 to be approved

Issues

• JDK-8333727: Use JOpt in jpackage to parse command line (Enhancement - P3)
• JDK-8371384: libapplauncher.so is copied to a wrong location in two step packaging when --install-dir=/usr (Bug - P4)
• JDK-8371447: Use JOpt in jpackage to parse command line (CSR)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28163

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back asemenyuk! 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]

@alexeysemenyukoracle 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.

[openjdk]

@alexeysemenyukoracle Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[mlbridge]

Webrevs

• 06: Full - Incremental (862285d6)
• 05: Full - Incremental (f7de2646)
• 04: Full - Incremental (0448168a)
• 03: Full - Incremental (da39bcc1)
• 02: Full - Incremental (096e4051)
• 01: Full - Incremental (fde4f4b9)
• 00: Full (2d975def)

[alexeysemenyukoracle]

/issue add 8371384

[openjdk]

@alexeysemenyukoracle
Adding additional issue to issue list: 8371384: libapplauncher.so is copied to a wrong location in two step packaging when --install-dir=/usr.

[alexeysemenyukoracle]

@sashamatveev PTAL

[openjdk]

@alexeysemenyukoracle Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[openjdk]

@alexeysemenyukoracle Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[openjdk]

@alexeysemenyukoracle Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[sashamatveev]

Looks good with some comments.

[sashamatveev]

Old tool provider implementation jdk.jpackage.internal.JPackageToolProvider had lousy thread-safety protection that didn't work when multiple instances of jpackage tool provider are created and invoked asynchronously. This patch fixes this issue. It is safe to invoke the same jpackage tool provider instance asynchronously, and also safe to invoke multiple instances of jpackage tool provider.

Can you explain more on this one? I did not figure out how synchronization issue was fixed and what was issue with original implementation.

[alexeysemenyukoracle]

There were a few synchronization issues with jpackage. The major one was the use of static member fields throughout the codebase. Andy attempted to fix it with JDK-8223322, but due to other synchronization issues, the fix for JDK-8223322 #1829 was never integrated. Instead, he applied a smaller-scope fix JDK-8259238 that fixed the use of static member fields. However, the "synchronized" keyword in JPackageToolProvider.java was still in place. I don't know what the intent of this "synchronized" is. It doesn't apply to a global object but to the instance of the "jdk.jpackage.internal.JPackageToolProvider" class. Looks like the assumption was that the system would create only a single instance of a specific tool provider. There is no such guarantee, though.

If multiple instances of the "jdk.jpackage.internal.JPackageToolProvider" class are created and used asynchronously, the "synchronized" keyword doesn't do what it is supposed to.

The new tool provider implementation in the "jdk.jpackage.internal.cli.Main" class doesn't use "synchronized" keyword. Command-line parsing code doesn't use global variables; it is thread-safe by design, rather than having thread safety as a patch.

[alexeysemenyukoracle]

Pushed commits addressing all findings in the review.
Additionally:

• Fix minor code formatting issue in src/jdk.jpackage/share/classes/jdk/jpackage/internal/cli/Validator.java
• Remove unreferenced keys from share\classes\jdk\jpackage\internal\resources\MainResources.properties