RFR: 8333727: Use JOpt in jpackage to parse command line [v6]
Alexander Matveev
almatvee at openjdk.org
Fri Nov 14 03:21:15 UTC 2025
On Wed, 12 Nov 2025 23:11:18 GMT, Alexey Semenyuk <asemenyuk at openjdk.org> wrote:
>> Use "JOpt Simple" from [jdk.internal.joptsimple](https://github.com/openjdk/jdk/tree/master/src/jdk.internal.opt/share/classes/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 cd7bca2bb665556f314170c81129ef53de91f135 commit.
>>
>> The reordered and filtered old help output is captured in the 10dc3792e6896cfa4bbe8693ee33e4c5df45d952 commit.
>>
>> Help output in this PR is captured in the 58c2d944e2e14b1cf35786162ad2a5f9a8ccfee6 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](https://github.com/openjdk/jdk/blob/5fccabff15ae8bcc3d03156fa331bbc0fefb0cbe/src/jdk.jpackage/share/classes/jdk/jpackage/internal/JPackageToolProvider.java#L48) 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 JD...
>
> Alexey Semenyuk has updated the pull request incrementally with one additional commit since the last revision:
>
> MainResources.properties: remove unreferenced L10N key
Looks good with some comments.
src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/MacBundlingEnvironment.java line 49:
> 47: .bundler(CREATE_MAC_APP_IMAGE, MacBundlingEnvironment::createAppImage)
> 48: .bundler(CREATE_MAC_DMG, LazyLoad::dmgSysEnv, MacBundlingEnvironment::createDmdPackage)
> 49: .bundler(CREATE_MAC_PKG, MacBundlingEnvironment::createPkgPackage));
Do you know why `CREATE_MAC_PKG` does not need/use environment like `CREATE_MAC_DMG`?
src/jdk.jpackage/macosx/classes/module-info.java.extra line 27:
> 25:
> 26: provides jdk.jpackage.internal.cli.CliBundlingEnvironment with
> 27: jdk.jpackage.internal.MacBundlingEnvironment;
Missing new line at the end of the file.
src/jdk.jpackage/share/classes/jdk/jpackage/internal/cli/HelpFormatter.java line 46:
> 44: this.optionGroups = Objects.requireNonNull(optionGroups);
> 45: this.formatter = Objects.requireNonNull(formatter);
> 46:
Extra new line.
src/jdk.jpackage/share/classes/jdk/jpackage/internal/cli/JOptSimpleOptionsBuilder.java line 184:
> 182: try {
> 183: initializer.run();
> 184: throw new AssertionError();
Can you put a comment on why we need `throw new AssertionError()`? I assume that `initializer.run()` never exits.
src/jdk.jpackage/share/classes/jdk/jpackage/internal/cli/OptionSpec.java line 126:
> 124: * <p>
> 125: * If the option has three names "a", "b", and "c", the stream will have three
> 126: * option spec objects each with a single name. The firt will have name "a", the
`firt` -> `first`
src/jdk.jpackage/share/classes/jdk/jpackage/internal/cli/StandardAppImageFileOption.java line 227:
> 225: }).map(option -> {
> 226: var spec = context.mapOptionSpec(option.spec());
> 227: var strValue = Optional.ofNullable(properties.get(spec.name().name()));
`name().name()` why we need name of the name and what it means?
src/jdk.jpackage/share/classes/jdk/jpackage/internal/cli/StandardOption.java line 328:
> 326: public static final OptionValue<List<Path>> MAC_DMG_CONTENT = pathOption("mac-dmg-content")
> 327: .valuePattern("additional content path")
> 328: //.toArray(pathSeparator())
Remove if not needed.
src/jdk.jpackage/share/classes/jdk/jpackage/internal/cli/StandardOption.java line 335:
> 333:
> 334: public static final OptionValue<Boolean> MAC_APP_STORE = booleanOption("mac-app-store")
> 335: //.scope(MAC_SIGNING) // TODO: --mac-app-store should be applicable to app image signing operation because it redefines signing key
I do not like any TODO comments. They usually never got fixed. Bug needs to be filed if follow up is required.
src/jdk.jpackage/share/classes/jdk/jpackage/internal/cli/StandardOption.java line 492:
> 490: context.asFileSource().ifPresent(propertyFile -> {
> 491: b.converterExceptionFactory(forMessageWithOptionValueAndName(propertyFile));
> 492: b.converterExceptionFormatString("error.properties-paramater-not-path");
`paramater` -> `parameter`. Below as well.
src/jdk.jpackage/share/classes/jdk/jpackage/internal/cli/StandardOption.java line 544:
> 542: }))
> 543: .converterExceptionFactory(ERROR_WITH_VALUE_AND_OPTION_NAME)
> 544: // .converterExceptionFormatString("error.paramater-not-launcher-shortcut-dir")
Remove if not needed.
src/jdk.jpackage/share/classes/jdk/jpackage/internal/cli/StandardOption.java line 644:
> 642: final var theCause = cause.orElseThrow();
> 643: if (theCause instanceof AddLauncherSyntaxException) {
> 644: // return ERROR_WITH_VALUE_AND_OPTION_NAME.create(optionName,
Remove if not needed.
src/jdk.jpackage/share/classes/jdk/jpackage/internal/cli/StandardValidator.java line 48:
> 46: static final Predicate<Path> IS_DIRECTORY = Files::isDirectory;
> 47:
> 48: static final Predicate<Path> IS_EXISTENT_NOT_DIRECTORY = path -> {
`IS_EXISTENT_NOT_DIRECTORY` will be same as `IS_REGULAR_FILE`? And just do `return Files. isRegularFile(path)`.
test/jdk/tools/jpackage/junit/share/jdk.jpackage/jdk/jpackage/internal/cli/StandardOptionTest.java line 403:
> 401: }
> 402:
> 403: // Builder expectMalformedError(String v) {
Remove if not needed.
test/jdk/tools/jpackage/junit/share/jdk.jpackage/jdk/jpackage/internal/cli/StandardOptionTest.java line 544:
> 542: buildLauncherShortcutTest().optionValue("false"),
> 543: buildLauncherShortcutTest().optionValue(""),
> 544: // buildLauncherShortcutTest().optionValue("").propertyFile(true),
Remove and line 547.
-------------
PR Review: https://git.openjdk.org/jdk/pull/28163#pullrequestreview-3455666852
PR Review Comment: https://git.openjdk.org/jdk/pull/28163#discussion_r2520495679
PR Review Comment: https://git.openjdk.org/jdk/pull/28163#discussion_r2520589934
PR Review Comment: https://git.openjdk.org/jdk/pull/28163#discussion_r2521119607
PR Review Comment: https://git.openjdk.org/jdk/pull/28163#discussion_r2521215259
PR Review Comment: https://git.openjdk.org/jdk/pull/28163#discussion_r2521320407
PR Review Comment: https://git.openjdk.org/jdk/pull/28163#discussion_r2521373482
PR Review Comment: https://git.openjdk.org/jdk/pull/28163#discussion_r2525413799
PR Review Comment: https://git.openjdk.org/jdk/pull/28163#discussion_r2525418201
PR Review Comment: https://git.openjdk.org/jdk/pull/28163#discussion_r2525490887
PR Review Comment: https://git.openjdk.org/jdk/pull/28163#discussion_r2521406263
PR Review Comment: https://git.openjdk.org/jdk/pull/28163#discussion_r2521408337
PR Review Comment: https://git.openjdk.org/jdk/pull/28163#discussion_r2525506624
PR Review Comment: https://git.openjdk.org/jdk/pull/28163#discussion_r2525610819
PR Review Comment: https://git.openjdk.org/jdk/pull/28163#discussion_r2525612889
More information about the core-libs-dev
mailing list