-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix TestFlight builds on the Mac App Store #1812
Conversation
@AlexeyTsvetkov would you be able to review this, please? |
```xml | ||
<?xml version="1.0" encoding="UTF-8"?> | ||
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd"> | ||
<plist version="1.0"> | ||
<dict> | ||
<key>com.apple.security.app-sandbox</key> | ||
<true/> | ||
<key>com.apple.security.cs.allow-jit</key> | ||
<true/> | ||
<key>com.apple.security.cs.allow-unsigned-executable-memory</key> | ||
<true/> | ||
<key>com.apple.security.cs.disable-library-validation</key> | ||
<true/> | ||
<key>com.apple.security.cs.allow-dyld-environment-variables</key> | ||
<true/> | ||
<key>com.apple.security.cs.debugger</key> | ||
<true/> | ||
<key>com.apple.security.device.audio-input</key> | ||
<true/> | ||
</dict> | ||
</plist> | ||
``` |
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.
Maybe some of these entitlements could be left out, because they are already added to the app level entitlements file. I am not sure. To be safe I just added all of the ones used by default here https://github.com/openjdk/jdk/blob/master/src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/resources/sandbox.plist.
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.
Alternatively, if these runtime entitlements do not have to be changed by the user, maybe there is no need to let the user customise them and just include them in compose.
```xml | ||
<?xml version="1.0" encoding="UTF-8"?> | ||
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd"> | ||
<plist version="1.0"> | ||
<dict> | ||
<key>com.apple.security.app-sandbox</key> | ||
<true/> | ||
<key>com.apple.security.cs.allow-jit</key> | ||
<true/> | ||
<key>com.apple.security.cs.allow-unsigned-executable-memory</key> | ||
<true/> | ||
<key>com.apple.security.cs.disable-library-validation</key> | ||
<true/> | ||
<key>com.apple.security.cs.allow-dyld-environment-variables</key> | ||
<true/> | ||
<key>com.apple.security.cs.debugger</key> | ||
<true/> | ||
<key>com.apple.security.device.audio-input</key> | ||
<true/> | ||
<key>com.apple.application-identifier</key> | ||
<string>TEAMID.APPID</string> | ||
<key>com.apple.developer.team-identifier</key> | ||
<string>TEAMID</string> | ||
<!-- Add additional entitlements here, for example for network or hardware access. --> | ||
</dict> | ||
</plist> | ||
``` |
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.
Some of these entitlements are from https://github.com/openjdk/jdk/blob/master/src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/resources/sandbox.plist.
The com.apple.application-identifier
and com.apple.developer.team-identifier
entitlements are needed for TestFlight.
private fun resignRuntimeAndAppDir( | ||
appDir: File, | ||
runtimeDir: File | ||
) { | ||
// Sign all libs and executables in runtime | ||
runtimeDir.walk().forEach { file -> | ||
val path = file.toPath() | ||
if (path.isRegularFile() && (path.isExecutable() || path.toString().isDylibPath)) { | ||
if (path.isSymbolicLink()) { | ||
// Ignore symbolic links | ||
} else { | ||
// Resign file | ||
macSigner.unsign(file) | ||
macSigner.sign(file, runtimeEntitlementsFile) | ||
} | ||
} | ||
} | ||
|
||
// Resign runtime directory | ||
macSigner.unsign(runtimeDir) | ||
macSigner.sign(runtimeDir, runtimeEntitlementsFile, forceEntitlements = true) | ||
|
||
// Resign app directory (contents other than runtime were already signed by jpackage) | ||
macSigner.unsign(appDir) | ||
macSigner.sign(appDir, entitlementsFile, forceEntitlements = true) | ||
} | ||
} |
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.
Logic in this function is similar to the JDK sources here: https://github.com/openjdk/jdk/blob/380378c551b4243ef72d868571f725b390e12124/src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/MacAppImageBuilder.java#L651-L822
if (runtimeProvisioningProfile == null && | ||
// When resigning the runtime, an app entitlements file is also needed. | ||
(runtimeEntitlementsFile == null || entitlementsFile == null) | ||
) { | ||
return |
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.
What to do if no entitlements file is provided? Should compose bundle a default file like jpackage or is there a way to read the default file from jpackage?
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.
I think it's ok to simply require in docs for these files to be set.
Another possibility is to add a warning/error, when mac app store publication is required (macAppStore
is true
), but these files are not provided.
@@ -149,6 +154,11 @@ abstract class AbstractJPackageTask @Inject constructor( | |||
@get:PathSensitive(PathSensitivity.ABSOLUTE) | |||
val macProvisioningProfile: RegularFileProperty = objects.fileProperty() |
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 file is currently added using --app-content
which requires JDK 18. But now that almost the complete app is already resigned manually after jpackage, I guess this could be done manually as well. Then JDK 18 would no longer be required. What do you think? At least then users don't have to use a JDK version that is still in preview.
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.
If you could try and it works for you, the PR is welcome :)
On the other hand JDK 18 should be released in a couple of months
if (runtimeProvisioningProfile == null && | ||
// When resigning the runtime, an app entitlements file is also needed. | ||
(runtimeEntitlementsFile == null || entitlementsFile == null) | ||
) { | ||
return |
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.
I think it's ok to simply require in docs for these files to be set.
Another possibility is to add a warning/error, when mac app store publication is required (macAppStore
is true
), but these files are not provided.
@@ -149,6 +154,11 @@ abstract class AbstractJPackageTask @Inject constructor( | |||
@get:PathSensitive(PathSensitivity.ABSOLUTE) | |||
val macProvisioningProfile: RegularFileProperty = objects.fileProperty() |
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.
If you could try and it works for you, the PR is welcome :)
On the other hand JDK 18 should be released in a couple of months
@Thomas-Vos thanks for the contribution! Cool work |
@AlexeyTsvetkov Great, thanks a lot for the review. I noticed a minor issue in the documentation which I fixed in #1820. |
This PR fixes TestFlight builds, see #1599 and #1613 for details.
Two new properties are added:
macOS.runtimeEntitlementsFile
- entitlements file for the JVM runtimemacOS.runtimeProvisioningProfile
- provisioning profile for the JVM runtimeBecause jpackage does not support the properties above, the runtime (and app dir) needs to be resigned manually after the files are added.
I did update the documentation, but I could see that this is a bit hard to understand. So maybe that needs to be made more clear with some help.
Please let me know what you think about this. My app is available on TestFlight with this PR!