On Tue, 8 Jul 2025 00:15:29 GMT, Alexey Semenyuk <asemen...@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
