8351073: [macos] jpackage produces invalid Java runtime DMG bundles #26173
Conversation
|
👋 Welcome back almatvee! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@sashamatveev The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
|
Mailing list message from Michael Hall on core-libs-dev: What is the .jdk extension preceded by? A normal system jdk is like jdk-23.jdk Maybe something like jpkg-23.jdk to indicate the source of the jre image is jpakcage? Also ensuring it doesn?t conflict with a normally distributed one.
-------------- next part -------------- |
|
| return RUNTIME_PACKAGE_LAYOUT; | ||
| return RuntimeLayout.DEFAULT; |
There was a problem hiding this comment.
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.
| /** | ||
| * Gets the source directory of this application if available or an empty | ||
| * {@link Optional} instance. | ||
| * <p> | ||
| * Source directory is a directory with the applications's classes and other | ||
| * resources. | ||
| * | ||
| * @return the source directory of this application | ||
| */ | ||
| Optional<Path> runtimeImage(); | ||
|
|
There was a problem hiding this comment.
What is "the source directory of this application"?
Why is the method that returns a path to the source directory called runtimeImage()?
| // External runtime image should be R/O unless it is runtime installer | ||
| // on macOS. On macOS runtime image will be signed ad-hoc or with | ||
| // real certificate when creating runtime installers. | ||
| return !(cmd.isRuntime() && TKit.isOSX()); |
There was a problem hiding this comment.
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.
| // Make sure we have valid runtime image. | ||
| if (!isRuntimeImageJDKBundle(runtimeImage) | ||
| && !isRuntimeImageJDKImage(runtimeImage)) { | ||
| throw new ConfigException( |
There was a problem hiding this comment.
- Can be simplified:
throw new ConfigException(
I18N.format("message.runtime-image-invalid", runtimeImage),
I18N.getString("message.runtime-image-invalid.advice"));
- This validation should be a part of
MacPackageBuilder.create().
| .get(); | ||
| return IOUtils.exists(jli); | ||
| } catch (IOException | NoSuchElementException ex) { | ||
| Log.verbose(ex); |
There was a problem hiding this comment.
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.
| private String validatedBundleIdentifier() throws ConfigException { | ||
| public String validatedBundleIdentifier() throws ConfigException { |
There was a problem hiding this comment.
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.
| default boolean isRuntimeImageJDKImage() { | ||
| if (isRuntimeInstaller()) { | ||
| Path runtimeImage = app().runtimeImage().orElseThrow(); | ||
| Path p = runtimeImage.resolve("Contents/Home"); | ||
| return !Files.exists(p); | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| // Returns true if signing is requested or JDK bundle is not signed | ||
| // or JDK image is provided. | ||
| default boolean isRuntimeJDKBundleNeedSigning() { | ||
| if (!isRuntimeInstaller()) { | ||
| return false; | ||
| } | ||
|
|
||
| if (app().sign()) { | ||
| return true; | ||
| } | ||
|
|
||
| if (isRuntimeImageJDKImage()) { | ||
| return true; | ||
| } else { | ||
| Path runtimeImage = app().runtimeImage().orElseThrow(); | ||
| Path p = runtimeImage.resolve("Contents/_CodeSignature"); | ||
| return !Files.exists(p); | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| * jpackagerTest keychain with | ||
| * always allowed access to this keychain for user which runs test. | ||
| * note: | ||
| * "jpackage.openjdk.java.net" can be over-ridden by systerm property |
| ); | ||
|
|
||
| MacHelper.withExplodedDmg(dummyCMD, dmgImage -> { | ||
| if (dmgImage.endsWith(dummyCMD.name() + ".jdk")) { |
There was a problem hiding this comment.
Can this test ever return false?
|
|
||
| MacHelper.withExplodedDmg(dummyCMD, dmgImage -> { | ||
| if (dmgImage.endsWith(dummyCMD.name() + ".jdk")) { | ||
| Executor.of("cp", "-R") |
There was a problem hiding this comment.
Why noit use FileUtils.copyRecursive()?
| public static void test(boolean useJDKBundle, | ||
| SigningBase.CertIndex JDKBundleCert, | ||
| SigningBase.CertIndex signCert) throws Exception { | ||
| final int JDKBundleCertIndex = JDKBundleCert.value(); |
There was a problem hiding this comment.
JDKBundleCertIndex -> jdkBundleCertIndex?
| (signCertIndex != SigningBase.CertIndex.INVALID_INDEX.value()); | ||
|
|
||
| new PackageTest() | ||
| .forTypes(PackageType.MAC) |
There was a problem hiding this comment.
.forTypes(PackageType.MAC) is redundant. The test will only be executed on macOS
| } | ||
| } | ||
|
|
||
| private static Path getRuntimeImagePath(boolean useJDKBundle, |
There was a problem hiding this comment.
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.
|
|
||
| ex.execute(); | ||
|
|
||
| JPackageCommand dummyCMD = new JPackageCommand(); |
There was a problem hiding this comment.
Why do you need a dummyCMD? You can configure a normal JPackageCommand command:
var cmd = new JPackageCommand().useToolProvider(true).dumpOutput(true).addArguments(...);
| testSpec().noAppDesc().nativeType() | ||
| .addArgs("--runtime-image", Token.INVALID_MAC_JDK_BUNDLE.token()) | ||
| .error("message.runtime-image-invalid", JPackageCommand.cannedArgument(cmd -> { | ||
| return Path.of(cmd.getArgumentValue("--runtime-image")).toAbsolutePath(); |
There was a problem hiding this comment.
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?
| INVALID_MAC_JDK_BUNDLE(cmd -> { | ||
| // Missing "Contents/MacOS/libjli.dylib" | ||
| try { | ||
| final Path runtimePath = TKit.createTempDirectory("invalidJDKBundle"); |
There was a problem hiding this comment.
The name "invalid-jdk-bundle" fits better in dash-case naming scheme for path names in jpackage tests.
| <plist version="1.0"> | ||
| <dict> | ||
| <key>CFBundleDevelopmentRegion</key> | ||
| <string>English</string> |
There was a problem hiding this comment.
I'm surprised there is no standard way to override this default value. Is it not important?
There was a problem hiding this comment.
What is the origin of this file?
| @@ -308,9 +363,16 @@ private static void writeAppInfoPlist( | |||
| .saveToFile(infoPlistFile); | |||
| } | |||
|
|
|||
| private static void sign(AppImageBuildEnv<MacApplication, MacApplicationLayout> env) throws IOException { | |||
There was a problem hiding this comment.
I don't understand why individual signing functions are needed for the runtime and application bundles. They look identical to me.
| @@ -271,6 +310,22 @@ private static void writeRuntimeInfoPlist(MacApplication app, BuildEnv env, Path | |||
| .saveToFile(runtimeRootDirectory.resolve("Contents/Info.plist")); | |||
| } | |||
|
|
|||
| private static void writeRuntimeBundleInfoPlist(MacApplication app, BuildEnv env, Path runtimeRootDirectory) throws IOException { | |||
There was a problem hiding this comment.
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
}
Re-submission of #25314 after merge conflict was resolved. Old PR will be closed.
All comments are addressed from old PR. Merge conflict was significant, so it is like new fix.
Fixed jpackage to produce valid Java runtimes based on description below:
Definitions:
jpackage output based on input:
Additional changes:
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26173/head:pull/26173$ git checkout pull/26173Update a local copy of the PR:
$ git checkout pull/26173$ git pull https://git.openjdk.org/jdk.git pull/26173/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26173View PR using the GUI difftool:
$ git pr show -t 26173Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26173.diff
Using Webrev
Link to Webrev Comment