On Wed, 28 May 2025 07:11:36 GMT, Alexander Matveev <almat...@openjdk.org> wrote:
> - `--install-dir` option in DMG packaging is no longer ignored. > - Defaults are still the same: `/Applications` and > `/Library/Java/JavaVirtualMachines`. > - If the installation directory doesn't exist, jpackage will try to create > and delete it right after the DMG package is created. > - If jpackage was unable to create installation directory error will be > thrown or if installation directory points to invalid location like file. > - It will be user responsibility to make sure installation directory exist on > target machine, since DMG cannot create directories during drag and drop. > - Target directory in case of non-default installation dir will display full > path. See image below for example. > >  Changes requested by asemenyuk (Reviewer). src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/MacDmgPackageBuilder.java line 99: > 97: } > 98: } catch (Exception ex) { > 99: Log.verbose(ex); What is the point of this log statement? The error will be logged at the top level anyway src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/MacDmgPackageBuilder.java line 101: > 99: Log.verbose(ex); > 100: throw new RuntimeException(ex); > 101: } This code should be in a separate function covered with unit tests. src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/model/MacDmgPackageMixin.java line 41: > 39: * @return Root of created install dir > 40: */ > 41: Optional<Path> installDirDeleteRoot(); Why is this information a part of the model? Can't dmg packager figure it out on its own? src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/resources/MacResources.properties line 84: > 82: message.setfile.dmg=Setting custom icon on DMG file skipped because > 'SetFile' utility was not found. Installing Xcode with Command Line Tools > should resolve this issue. > 83: message.install-dir-invalid=Error: "--install-dir" value {0} is ivalid > for DMG packaging. Make sure it is valid path to existing or non-existing > directory. > 84: message.install-dir-create=Error: Unable to create install dir {0}. Make > sure you have sufficient permissions or create it manually. Exception: {1} Are these error messages DMG-specific? If "yes", let's give them less vague names, like `message.dmg-install-dir-invalid` test/jdk/tools/jpackage/share/InstallDirTest.java line 121: > 119: new PackageTest() > 120: .setExpectedExitCode(1) > 121: .excludeTypes(PackageType.MAC_PKG) `.forTypes(PackageType.MAC_DMG)` would be more appropriate as the test applies to DMG and not to "all packages except of PKG". If we add another bundle type, `.excludedTypes(Package Type.MAC_PKG)` will become wrong. ------------- PR Review: https://git.openjdk.org/jdk/pull/25481#pullrequestreview-2876618289 PR Review Comment: https://git.openjdk.org/jdk/pull/25481#discussion_r2112872869 PR Review Comment: https://git.openjdk.org/jdk/pull/25481#discussion_r2112872246 PR Review Comment: https://git.openjdk.org/jdk/pull/25481#discussion_r2112875748 PR Review Comment: https://git.openjdk.org/jdk/pull/25481#discussion_r2112880614 PR Review Comment: https://git.openjdk.org/jdk/pull/25481#discussion_r2112879267
