RFR: 8351073: [macos] jpackage produces invalid Java runtime DMG bundles [v2]

Thu Jul 10 21:48:52 UTC 2025

On Tue, 8 Jul 2025 00:15:29 GMT, Alexey Semenyuk <asemenyuk at openjdk.org> wrote:

>> Alexander Matveev has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision:
>> 
>>  - Merge remote-tracking branch 'upstream/master' into JDK-8351073-2
>>  - 8351073: [macos] jpackage produces invalid Java runtime DMG bundles
>
> src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/MacApplicationBuilder.java line 184:
> 
>> 182:     }
>> 183: 
>> 184:     public String validatedBundleIdentifier() throws ConfigException {
> 
> This method is private on purpose. It should not be used outside of the MacApplicationBuilder class. If you need to get the valid bundle identifier, create MacApplication instance and call `MacApplication.bundleIdentifier()` on it.

I need it inside `createMacApplication()` before `MacApplication` instance is created.

> src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/MacBaseInstallerBundler.java line 80:
> 
>> 78:             if (!isRuntimeImageJDKBundle(runtimeImage)
>> 79:                     && !isRuntimeImageJDKImage(runtimeImage)) {
>> 80:                 throw new ConfigException(
> 
> 1. Can be simplified:
> 
> throw new ConfigException(
>                     I18N.format("message.runtime-image-invalid", runtimeImage),
>                     I18N.getString("message.runtime-image-invalid.advice"));
> 
> 
> 2. This validation should be a part of `MacPackageBuilder.create()`.

1 and 2 fixed.

> src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/MacBaseInstallerBundler.java line 120:
> 
>> 118:             return IOUtils.exists(jli);
>> 119:         } catch (IOException | NoSuchElementException ex) {
>> 120:             Log.verbose(ex);
> 
> So if jpackage doesn't know if the given path is a JDK image or a JDK bundle, it will proceed with packaging anyway. This doesn't look right. In general, most, if not all, of the existing constructions like:
> 
> } catch (Exception ex) {
>     Log.verbose(ex);
>     ...
> }
> 
> are wrong. Let's not add new ones.

Fixed.

> src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/MacPackagingPipeline.java line 313:
> 
>> 311:     }
>> 312: 
>> 313:     private static void writeRuntimeBundleInfoPlist(MacApplication app, BuildEnv env, Path runtimeRootDirectory) throws IOException {
> 
> This function is almost a duplicate of writeRuntimeInfoPlist(). They can be merged together with branching:
> 
> if (app.asApplicationLayout().isPresent()) {
>     // This is application bundle
> } else {
>     // This is runtime bundle
> }

Fixed

> src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/model/MacPackage.java line 38:
> 
>> 36:     default AppImageLayout appImageLayout() {
>> 37:         if (isRuntimeInstaller()) {
>> 38:             return RuntimeLayout.DEFAULT;
> 
> Why is this change? It looks wrong. The layout of the output bundle should have the "Contents" folder. This is how it was before the change.

`RUNTIME_PACKAGE_LAYOUT` points to "Contents/Home". Bundle is "Contents/Home", "Contents/MacOS" and "Contents/Info.plist". So we need root of bundle and not "Home". Maybe I misunderstanding something.

> src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/model/MacPackage.java line 76:
> 
>> 74:             return !Files.exists(p);
>> 75:         }
>> 76:     }
> 
> Default interface methods should operate on values returned by other interface methods. These new functions involve filesystem operations. They probably don't belong to the interface.

Moved to `MacPackagingPipeline`.

> src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/resources/RuntimeBundle-Info.plist.template line 1:
> 
>> 1: <?xml version="1.0" encoding="UTF-8"?>
> 
> What is the origin of this file?

Based on JDK Info plist template we using. You can find original file here https://github.com/openjdk/jdk/blob/master/make/data/bundle/JDK-Info.plist.template.

> src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/resources/RuntimeBundle-Info.plist.template line 6:
> 
>> 4: <dict>
>> 5:         <key>CFBundleDevelopmentRegion</key>
>> 6:         <string>English</string>
> 
> I'm surprised there is no standard way to override this default value. Is it not important?

https://developer.apple.com/documentation/bundleresources/information-property-list/cfbundledevelopmentregion?language=objc

By default it will be `en-US`. Not sure why JDK Info.plist has `English`, since it is not a documented value. Also all jpackage Info plist template files also have `English`. I think we need to file a separate bug and change it to `en-US` or remove it. I think remove it is better, since no need to set it to default value.

Not sure if it is important for `runtime bundles`, but for `application bundles` it might be important in case if application does not have English localization. For example application is only in German language, then this value should be set to German language ID. I think it might make sense to file a bug to investigate if we want to provide CLI option to specify value for `CFBundleDevelopmentRegion` similar to `--mac-app-category`. Any suggestions?

> src/jdk.jpackage/share/classes/jdk/jpackage/internal/model/Application.java line 101:
> 
>> 99:      */
>> 100:     Optional<Path> runtimeImage();
>> 101: 
> 
> What is "the source directory of this application"?
> Why is the method that returns a path to the source directory called `runtimeImage()`?

Copy-paste. Forgot to update description. I fixed it. `runtimeImageDir` is a value of `--runtime-image` in case of runtime installer.

> test/jdk/tools/jpackage/helpers/jdk/jpackage/test/JPackageCommand.java line 974:
> 
>> 972:             // on macOS. On macOS runtime image will be signed ad-hoc or with
>> 973:             // real certificate when creating runtime installers.
>> 974:             return !(cmd.isRuntime() && TKit.isOSX());
> 
> I guess the directory referenced from the `--runtime-image` option will not be signed if it is a JDK image, not a JDK bundle. So, this switch turns off verification in cases when it should be done.
> 
> My interpretation of the PR description is that if the `-runtime-image` option references a JDK bundle, its contents will be copied, and the copy will be signed optionally. This makes this switch redundant.

Yes, it is no longer needed. I removed it.

> test/jdk/tools/jpackage/macosx/SigningRuntimeImagePackageTest.java line 65:
> 
>> 63:  * always allowed access to this keychain for user which runs test.
>> 64:  * note:
>> 65:  * "jpackage.openjdk.java.net" can be over-ridden by systerm property
> 
> systerm -> system

Fixed.

> test/jdk/tools/jpackage/macosx/SigningRuntimeImagePackageTest.java line 134:
> 
>> 132:     }
>> 133: 
>> 134:     private static Path getRuntimeImagePath(boolean useJDKBundle,
> 
> The name of the function is misleading. It doesn't get the runtime image. It creates it.
> 
> It returns a path to a JDK bundle or a JDK image depending on its arguments. So the "RuntimeImagePath" part of its name is also misleading.

Improved.

> test/jdk/tools/jpackage/macosx/SigningRuntimeImagePackageTest.java line 180:
> 
>> 178:             ex.execute();
>> 179: 
>> 180:             JPackageCommand dummyCMD = new JPackageCommand();
> 
> Why do you need a `dummyCMD`? You can configure a normal JPackageCommand command:
> 
> var cmd = new JPackageCommand().useToolProvider(true).dumpOutput(true).addArguments(...);

Fixed.

> test/jdk/tools/jpackage/macosx/SigningRuntimeImagePackageTest.java line 188:
> 
>> 186: 
>> 187:             MacHelper.withExplodedDmg(dummyCMD, dmgImage -> {
>> 188:                 if (dmgImage.endsWith(dummyCMD.name() + ".jdk")) {
> 
> Can this test ever return `false`?

Yes. `withExplodedDmg` is called for all content in DMG which will include link to `JavaVirtualMachines`. We have same checks for `.app` as well for exact same reason.

> test/jdk/tools/jpackage/macosx/SigningRuntimeImagePackageTest.java line 189:
> 
>> 187:             MacHelper.withExplodedDmg(dummyCMD, dmgImage -> {
>> 188:                 if (dmgImage.endsWith(dummyCMD.name() + ".jdk")) {
>> 189:                     Executor.of("cp", "-R")
> 
> Why noit use `FileUtils.copyRecursive()`?

All our other code uses `cp` command to copy mounted DMG. I tried `FileUtils.copyRecursive()`, but test failed due to signing. I think `FileUtils.copyRecursive()` breaks file permissions or something. Will keep `cp` for consistency.

> test/jdk/tools/jpackage/macosx/SigningRuntimeImagePackageTest.java line 222:
> 
>> 220:                             SigningBase.CertIndex JDKBundleCert,
>> 221:                             SigningBase.CertIndex signCert) throws Exception {
>> 222:         final int JDKBundleCertIndex = JDKBundleCert.value();
> 
> `JDKBundleCertIndex` -> `jdkBundleCertIndex`?

Fixed.

> test/jdk/tools/jpackage/macosx/SigningRuntimeImagePackageTest.java line 231:
> 
>> 229: 
>> 230:         new PackageTest()
>> 231:                 .forTypes(PackageType.MAC)
> 
> `.forTypes(PackageType.MAC)` is redundant. The test will only be executed on macOS

Fixed.

> test/jdk/tools/jpackage/share/ErrorTest.java line 98:
> 
>> 96:             // Missing "Contents/MacOS/libjli.dylib"
>> 97:             try {
>> 98:                 final Path runtimePath = TKit.createTempDirectory("invalidJDKBundle");
> 
> The name "invalid-jdk-bundle" fits better in dash-case naming scheme for path names in jpackage tests.

Fixed.

> test/jdk/tools/jpackage/share/ErrorTest.java line 637:
> 
>> 635:                         .addArgs("--runtime-image", Token.INVALID_MAC_JDK_BUNDLE.token())
>> 636:                         .error("message.runtime-image-invalid", JPackageCommand.cannedArgument(cmd -> {
>> 637:                             return Path.of(cmd.getArgumentValue("--runtime-image")).toAbsolutePath();
> 
> Why do we need an absolute path in error messages? Wouldn't it make more sense to have the value of `--runtime-image` parameter as is in error messages?

Not sure why. Fixed.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/26173#discussion_r2193668918
PR Review Comment: https://git.openjdk.org/jdk/pull/26173#discussion_r2193679404
PR Review Comment: https://git.openjdk.org/jdk/pull/26173#discussion_r2193704037
PR Review Comment: https://git.openjdk.org/jdk/pull/26173#discussion_r2193710393
PR Review Comment: https://git.openjdk.org/jdk/pull/26173#discussion_r2193718285
PR Review Comment: https://git.openjdk.org/jdk/pull/26173#discussion_r2196225026
PR Review Comment: https://git.openjdk.org/jdk/pull/26173#discussion_r2196230170
PR Review Comment: https://git.openjdk.org/jdk/pull/26173#discussion_r2196251812
PR Review Comment: https://git.openjdk.org/jdk/pull/26173#discussion_r2196314256
PR Review Comment: https://git.openjdk.org/jdk/pull/26173#discussion_r2196395258
PR Review Comment: https://git.openjdk.org/jdk/pull/26173#discussion_r2196396676
PR Review Comment: https://git.openjdk.org/jdk/pull/26173#discussion_r2196401655
PR Review Comment: https://git.openjdk.org/jdk/pull/26173#discussion_r2196404744
PR Review Comment: https://git.openjdk.org/jdk/pull/26173#discussion_r2196406257
PR Review Comment: https://git.openjdk.org/jdk/pull/26173#discussion_r2196408886
PR Review Comment: https://git.openjdk.org/jdk/pull/26173#discussion_r2196410205
PR Review Comment: https://git.openjdk.org/jdk/pull/26173#discussion_r2196411953
PR Review Comment: https://git.openjdk.org/jdk/pull/26173#discussion_r2196413148
PR Review Comment: https://git.openjdk.org/jdk/pull/26173#discussion_r2198799393