On Wed, 12 Nov 2025 23:11:18 GMT, Alexey Semenyuk <[email protected]> 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
