-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8351073: [macos] jpackage produces invalid Java runtime DMG bundles #26173
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,13 +27,17 @@ | |
|
||
import static jdk.jpackage.internal.StandardBundlerParam.PREDEFINED_APP_IMAGE; | ||
import static jdk.jpackage.internal.StandardBundlerParam.PREDEFINED_APP_IMAGE_FILE; | ||
import static jdk.jpackage.internal.StandardBundlerParam.PREDEFINED_RUNTIME_IMAGE; | ||
import static jdk.jpackage.internal.StandardBundlerParam.SIGN_BUNDLE; | ||
|
||
import java.io.IOException; | ||
import java.nio.file.Files; | ||
import java.nio.file.Path; | ||
import java.text.MessageFormat; | ||
import java.util.Map; | ||
import java.util.NoSuchElementException; | ||
import java.util.Optional; | ||
import java.util.stream.Stream; | ||
import jdk.jpackage.internal.model.ConfigException; | ||
|
||
public abstract class MacBaseInstallerBundler extends AbstractBundler { | ||
|
@@ -63,6 +67,23 @@ protected void validateAppImageAndBundeler( | |
"warning.unsigned.app.image"), getID())); | ||
} | ||
} | ||
} else if (StandardBundlerParam.isRuntimeInstaller(params)) { | ||
// Call appImageBundler.validate(params); to validate signing | ||
// requirements. | ||
appImageBundler.validate(params); | ||
|
||
Path runtimeImage = PREDEFINED_RUNTIME_IMAGE.fetchFrom(params); | ||
|
||
// Make sure we have valid runtime image. | ||
if (!isRuntimeImageJDKBundle(runtimeImage) | ||
&& !isRuntimeImageJDKImage(runtimeImage)) { | ||
throw new ConfigException( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
MessageFormat.format(I18N.getString( | ||
"message.runtime-image-invalid"), | ||
runtimeImage.toString()), | ||
I18N.getString( | ||
"message.runtime-image-invalid.advice")); | ||
} | ||
} else { | ||
appImageBundler.validate(params); | ||
} | ||
|
@@ -73,5 +94,33 @@ public String getBundleType() { | |
return "INSTALLER"; | ||
} | ||
|
||
// JDK bundle: "Contents/Home", "Contents/MacOS/libjli.dylib" | ||
// and "Contents/Info.plist" | ||
private static boolean isRuntimeImageJDKBundle(Path runtimeImage) { | ||
Path path1 = runtimeImage.resolve("Contents/Home"); | ||
Path path2 = runtimeImage.resolve("Contents/MacOS/libjli.dylib"); | ||
Path path3 = runtimeImage.resolve("Contents/Info.plist"); | ||
return IOUtils.exists(path1) | ||
&& path1.toFile().list() != null | ||
&& path1.toFile().list().length > 0 | ||
&& IOUtils.exists(path2) | ||
&& IOUtils.exists(path3); | ||
} | ||
|
||
// JDK image: "lib/*/libjli.dylib" | ||
static boolean isRuntimeImageJDKImage(Path runtimeImage) { | ||
final Path jliName = Path.of("libjli.dylib"); | ||
try (Stream<Path> walk = Files.walk(runtimeImage.resolve("lib"))) { | ||
final Path jli = walk | ||
.filter(file -> file.getFileName().equals(jliName)) | ||
.findFirst() | ||
.get(); | ||
return IOUtils.exists(jli); | ||
} catch (IOException | NoSuchElementException ex) { | ||
Log.verbose(ex); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
are wrong. Let's not add new ones. |
||
return false; | ||
} | ||
} | ||
|
||
private final Bundler appImageBundler; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,6 +36,7 @@ | |
import static jdk.jpackage.internal.util.XmlUtils.toXmlConsumer; | ||
import static jdk.jpackage.internal.util.function.ThrowingBiConsumer.toBiConsumer; | ||
import static jdk.jpackage.internal.util.function.ThrowingSupplier.toSupplier; | ||
import static jdk.jpackage.internal.model.MacPackage.RUNTIME_PACKAGE_LAYOUT; | ||
|
||
import java.io.IOException; | ||
import java.io.StringWriter; | ||
|
@@ -71,6 +72,7 @@ | |
import jdk.jpackage.internal.model.MacPackage; | ||
import jdk.jpackage.internal.model.Package; | ||
import jdk.jpackage.internal.model.PackageType; | ||
import jdk.jpackage.internal.model.RuntimeLayout; | ||
import jdk.jpackage.internal.model.PackagerException; | ||
import jdk.jpackage.internal.util.PathUtils; | ||
import jdk.jpackage.internal.util.function.ThrowingConsumer; | ||
|
@@ -91,12 +93,22 @@ enum MacBuildApplicationTaskID implements TaskID { | |
enum MacCopyAppImageTaskID implements TaskID { | ||
COPY_PACKAGE_FILE, | ||
COPY_RUNTIME_INFO_PLIST, | ||
COPY_RUNTIME_JLILIB, | ||
REPLACE_APP_IMAGE_FILE, | ||
COPY_SIGN | ||
COPY_SIGN, | ||
SIGN_RUNTIME_BUNDLE | ||
} | ||
|
||
static AppImageLayout packagingLayout(Package pkg) { | ||
return pkg.appImageLayout().resolveAt(pkg.relativeInstallDir().getFileName()); | ||
if (pkg.isRuntimeInstaller()) { | ||
if (((MacPackage)pkg).isRuntimeImageJDKImage()) { | ||
return RUNTIME_PACKAGE_LAYOUT.resolveAt(pkg.relativeInstallDir().getFileName()); | ||
} else { | ||
return RuntimeLayout.DEFAULT.resolveAt(pkg.relativeInstallDir().getFileName()); | ||
} | ||
} else { | ||
return pkg.appImageLayout().resolveAt(pkg.relativeInstallDir().getFileName()); | ||
} | ||
} | ||
|
||
static PackagingPipeline.Builder build(Optional<Package> pkg) { | ||
|
@@ -118,7 +130,7 @@ static PackagingPipeline.Builder build(Optional<Package> pkg) { | |
.applicationAction(MacPackagingPipeline::writeApplicationRuntimeInfoPlist) | ||
.addDependent(BuildApplicationTaskID.CONTENT).add() | ||
.task(MacBuildApplicationTaskID.COPY_JLILIB) | ||
.applicationAction(MacPackagingPipeline::copyJliLib) | ||
.applicationAction(MacPackagingPipeline::copyApplicationRuntimeJliLib) | ||
.addDependency(BuildApplicationTaskID.RUNTIME) | ||
.addDependent(BuildApplicationTaskID.CONTENT).add() | ||
.task(MacBuildApplicationTaskID.APP_ICON) | ||
|
@@ -140,6 +152,12 @@ static PackagingPipeline.Builder build(Optional<Package> pkg) { | |
.task(MacCopyAppImageTaskID.COPY_RUNTIME_INFO_PLIST) | ||
.addDependencies(CopyAppImageTaskID.COPY) | ||
.addDependents(PrimaryTaskID.COPY_APP_IMAGE).add() | ||
.task(MacCopyAppImageTaskID.COPY_RUNTIME_JLILIB) | ||
.addDependencies(CopyAppImageTaskID.COPY) | ||
.addDependents(PrimaryTaskID.COPY_APP_IMAGE).add() | ||
.task(MacCopyAppImageTaskID.SIGN_RUNTIME_BUNDLE) | ||
.addDependencies(CopyAppImageTaskID.COPY) | ||
.addDependents(PrimaryTaskID.COPY_APP_IMAGE).add() | ||
.task(MacBuildApplicationTaskID.FA_ICONS) | ||
.applicationAction(MacPackagingPipeline::writeFileAssociationIcons) | ||
.addDependent(BuildApplicationTaskID.CONTENT).add() | ||
|
@@ -148,13 +166,13 @@ static PackagingPipeline.Builder build(Optional<Package> pkg) { | |
.addDependent(BuildApplicationTaskID.CONTENT).add(); | ||
|
||
builder.task(MacBuildApplicationTaskID.SIGN) | ||
.appImageAction(MacPackagingPipeline::sign) | ||
.appImageAction(MacPackagingPipeline::signApplicationBundle) | ||
.addDependencies(builder.taskGraphSnapshot().getAllTailsOf(PrimaryTaskID.BUILD_APPLICATION_IMAGE)) | ||
.addDependent(PrimaryTaskID.BUILD_APPLICATION_IMAGE) | ||
.add(); | ||
|
||
builder.task(MacCopyAppImageTaskID.COPY_SIGN) | ||
.appImageAction(MacPackagingPipeline::sign) | ||
.appImageAction(MacPackagingPipeline::signApplicationBundle) | ||
.addDependencies(builder.taskGraphSnapshot().getAllTailsOf(PrimaryTaskID.COPY_APP_IMAGE)) | ||
.addDependent(PrimaryTaskID.COPY_APP_IMAGE) | ||
.add(); | ||
|
@@ -179,9 +197,20 @@ static PackagingPipeline.Builder build(Optional<Package> pkg) { | |
// don't create ".package" file and don't sign it. | ||
disabledTasks.add(MacCopyAppImageTaskID.COPY_PACKAGE_FILE); | ||
disabledTasks.add(MacCopyAppImageTaskID.COPY_SIGN); | ||
// if (p.isRuntimeInstaller()) { | ||
// builder.task(MacCopyAppImageTaskID.COPY_RUNTIME_INFO_PLIST).packageAction(MacPackagingPipeline::writeRuntimeRuntimeInfoPlist).add(); | ||
// } | ||
if (((MacPackage)p).isRuntimeImageJDKImage()) { | ||
builder.task(MacCopyAppImageTaskID.COPY_RUNTIME_INFO_PLIST) | ||
.packageAction(MacPackagingPipeline::writeRuntimeRuntimeInfoPlist) | ||
.add(); | ||
builder.task(MacCopyAppImageTaskID.COPY_RUNTIME_JLILIB) | ||
.packageAction(MacPackagingPipeline::copyRuntimeRuntimeJliLib) | ||
.add(); | ||
} | ||
|
||
if (((MacPackage)p).isRuntimeJDKBundleNeedSigning()) { | ||
builder.task(MacCopyAppImageTaskID.SIGN_RUNTIME_BUNDLE) | ||
.packageAction(MacPackagingPipeline::signRuntimeBundle) | ||
.add(); | ||
} | ||
} | ||
|
||
for (final var taskId : disabledTasks) { | ||
|
@@ -211,14 +240,24 @@ private static void copyAppImage(MacPackage pkg, AppImageDesc srcAppImage, | |
PackagingPipeline.copyAppImage(srcAppImage, dstAppImage, !pkg.predefinedAppImageSigned().orElse(false)); | ||
} | ||
|
||
private static void copyJliLib( | ||
private static void copyRuntimeRuntimeJliLib(PackageBuildEnv<MacPackage, AppImageLayout> env) throws IOException { | ||
copyJliLib(env.resolvedLayout().rootDirectory(), | ||
env.resolvedLayout().rootDirectory().resolve("Contents/Home")); | ||
} | ||
|
||
private static void copyApplicationRuntimeJliLib( | ||
AppImageBuildEnv<MacApplication, MacApplicationLayout> env) throws IOException { | ||
copyJliLib(env.resolvedLayout().runtimeRootDirectory(), | ||
env.resolvedLayout().runtimeDirectory()); | ||
} | ||
|
||
private static void copyJliLib(Path runtimeRootDirectory, Path runtimeLibRoot) throws IOException { | ||
|
||
final var runtimeMacOSDir = env.resolvedLayout().runtimeRootDirectory().resolve("Contents/MacOS"); | ||
final var runtimeMacOSDir = runtimeRootDirectory.resolve("Contents/MacOS"); | ||
|
||
final var jliName = Path.of("libjli.dylib"); | ||
|
||
try (var walk = Files.walk(env.resolvedLayout().runtimeDirectory().resolve("lib"))) { | ||
try (var walk = Files.walk(runtimeLibRoot.resolve("lib"))) { | ||
final var jli = walk | ||
.filter(file -> file.getFileName().equals(jliName)) | ||
.findFirst() | ||
|
@@ -248,7 +287,7 @@ private static void writePkgInfoFile( | |
} | ||
|
||
private static void writeRuntimeRuntimeInfoPlist(PackageBuildEnv<MacPackage, AppImageLayout> env) throws IOException { | ||
writeRuntimeInfoPlist(env.pkg().app(), env.env(), env.resolvedLayout().rootDirectory()); | ||
writeRuntimeBundleInfoPlist(env.pkg().app(), env.env(), env.resolvedLayout().rootDirectory()); | ||
} | ||
|
||
private static void writeApplicationRuntimeInfoPlist( | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function is almost a duplicate of writeRuntimeInfoPlist(). They can be merged together with branching:
|
||
|
||
Map<String, String> data = new HashMap<>(); | ||
data.put("CF_BUNDLE_IDENTIFIER", app.bundleIdentifier()); | ||
data.put("CF_BUNDLE_NAME", app.bundleName()); | ||
data.put("CF_BUNDLE_VERSION", app.version()); | ||
data.put("CF_BUNDLE_SHORT_VERSION_STRING", app.shortVersion().toString()); | ||
data.put("CF_BUNDLE_VENDOR", app.vendor()); | ||
|
||
env.createResource("RuntimeBundle-Info.plist.template") | ||
.setPublicName("RuntimeBundle-Info.plist") | ||
.setCategory(I18N.getString("resource.runtime-bundle-info-plist")) | ||
.setSubstitutionData(data) | ||
.saveToFile(runtimeRootDirectory.resolve("Contents/Info.plist")); | ||
} | ||
|
||
private static void writeAppInfoPlist( | ||
AppImageBuildEnv<MacApplication, MacApplicationLayout> env) throws IOException { | ||
|
||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand why individual signing functions are needed for the runtime and application bundles. They look identical to me. |
||
private static void signRuntimeBundle(PackageBuildEnv<MacPackage, AppImageLayout> env) throws IOException { | ||
sign(env.pkg().app(), env.env(), env.resolvedLayout().rootDirectory()); | ||
} | ||
|
||
final var app = env.app(); | ||
private static void signApplicationBundle(AppImageBuildEnv<MacApplication, MacApplicationLayout> env) throws IOException { | ||
sign(env.app(), env.env(), env.resolvedLayout().rootDirectory()); | ||
} | ||
|
||
private static void sign(final MacApplication app, | ||
final BuildEnv env, final Path appImageDir) throws IOException { | ||
|
||
final var codesignConfigBuilder = CodesignConfig.build(); | ||
app.signingConfig().ifPresent(codesignConfigBuilder::from); | ||
|
@@ -319,17 +381,16 @@ private static void sign(AppImageBuildEnv<MacApplication, MacApplicationLayout> | |
final var entitlementsDefaultResource = app.signingConfig().map( | ||
AppImageSigningConfig::entitlementsResourceName).orElseThrow(); | ||
|
||
final var entitlementsFile = env.env().configDir().resolve(app.name() + ".entitlements"); | ||
final var entitlementsFile = env.configDir().resolve(app.name() + ".entitlements"); | ||
|
||
env.env().createResource(entitlementsDefaultResource) | ||
env.createResource(entitlementsDefaultResource) | ||
.setCategory(I18N.getString("resource.entitlements")) | ||
.saveToFile(entitlementsFile); | ||
|
||
codesignConfigBuilder.entitlements(entitlementsFile); | ||
} | ||
|
||
final Runnable signAction = () -> { | ||
final var appImageDir = env.resolvedLayout().rootDirectory(); | ||
AppImageSigner.createSigner(app, codesignConfigBuilder.create()).accept(appImageDir); | ||
}; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,7 @@ | |
package jdk.jpackage.internal.model; | ||
|
||
import java.nio.file.Path; | ||
import java.nio.file.Files; | ||
import jdk.jpackage.internal.util.CompositeProxy; | ||
|
||
public interface MacPackage extends Package, MacPackageMixin { | ||
|
@@ -34,7 +35,7 @@ public interface MacPackage extends Package, MacPackageMixin { | |
@Override | ||
default AppImageLayout appImageLayout() { | ||
if (isRuntimeInstaller()) { | ||
return RUNTIME_PACKAGE_LAYOUT; | ||
return RuntimeLayout.DEFAULT; | ||
Comment on lines
-37
to
+38
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} else { | ||
return Package.super.appImageLayout(); | ||
} | ||
|
@@ -44,6 +45,36 @@ default Path installDir() { | |
return Path.of("/").resolve(relativeInstallDir()); | ||
} | ||
|
||
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); | ||
} | ||
} | ||
Comment on lines
+48
to
+76
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
public static MacPackage create(Package pkg, MacPackageMixin mixin) { | ||
return CompositeProxy.create(MacPackage.class, pkg, mixin); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.