-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8351073: [macos] jpackage produces invalid Java runtime DMG bundles #25314
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?
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:
Thanks for the follow-up. To the comments on the issue of would it be useful for jpackage to do this. The runtime images that you get with make images doesn?t work for applications. So it can?t be used to test applications. I assume your changes make that possible. How often you might want to test Validation again seemed to me to be important. In trying this out I attempted different ways to use the ?runtime-image parameter. All of them succeeded in creating dmg?s but the images were not valid. Is there some test being added? |
Mailing list message from Michael Hall on core-libs-dev:
I might be misremembering on this. It might be useable as a runtime image to embed in applications with jpackage? |
src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/resources/MacResources.properties
Outdated
Show resolved
Hide resolved
src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/resources/MacResources.properties
Show resolved
Hide resolved
src/jdk.jpackage/share/classes/jdk/jpackage/internal/IOUtils.java
Outdated
Show resolved
Hide resolved
src/jdk.jpackage/share/classes/jdk/jpackage/internal/IOUtils.java
Outdated
Show resolved
Hide resolved
VERSION::fetchFrom, | ||
params -> { | ||
String version = VERSION.fetchFrom(params); | ||
// Special case for MSI product version for runtime | ||
// installer. Runtime version can be single digit | ||
// for example "25", but product version requires 2 or 4 | ||
// components. JDK uses "25.0.0.0" in this case. | ||
if (StandardBundlerParam.isRuntimeInstaller(params) && | ||
!version.contains(".")) { | ||
version = version.concat(".0.0.0"); | ||
} | ||
return version; | ||
}, |
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 guess, this is a workaround for the case when the version comes from JDK's release file. This is the wrong place for this workaround. It should be a part of the code reading version from JDK's release file.
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.
Yes, this is workaround when the version comes from JDK's release file. In this case name will be customeJDK-25.0.0.0 for example if we change it when we read version form file. Other platform do not need it. Only PRODUCT_VERSION in MSI needs to be 2 or 4 components, so I think it is correct location for it.
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.
For runtime packaging, the version can be taken from the command line or the "release" file. In the latter case the version should be valid for the current platform and packaging type. Version adjustments should be isolated in a function that reads a version from the "release" file. The result of this function call should be such that it will be used as-is without any modifications.
Ideally, we should have Optional<String> readVersionFromRuntimeReleaseFile(String packageType, Path pathToReleaseFile)
function that we can unit test and that will return a valid version for the given packaging type ("packageType" parameter). It should return an empty Optional instance if it fails.
RUNTIME_IMAGE(new Builder("--runtime-image").enable(cmd -> { | ||
// External runtime image should be R/O unless it is on macOS. | ||
// On macOS it will be signed ad-hoc or with real certificate. | ||
return !TKit.isOSX(); |
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.
Do I get the comment right, and jpackage will modify the image supplied with --runtime-image
parameter? This is wrong. jpackage must not modify any externally supplied files/directories unless this is app image signing on macos.
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.
Yes, you get the comment right. We do sign runtime image as well (same as app image). We do ad-hoc signing for runtime image if signing is not requested. Ad-hoc signing will modify runtime image. I think you mean case when "--app-image" and "--runtime-image" is specified, then yes we should not modify it, but when we packaging just runtime it will be modify due to signature. I will update test to reflect it.
@@ -334,7 +334,7 @@ static Path getInstallationDirectory(JPackageCommand cmd) { | |||
installLocation = cmd.getArgumentValue("--install-dir", () -> defaultInstallLocation, Path::of); | |||
} | |||
|
|||
return installLocation.resolve(cmd.name() + (cmd.isRuntime() ? "" : ".app")); | |||
return installLocation.resolve(cmd.name() + (cmd.isRuntime() ? ".jdk" : ".app")); |
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.
Why the ".jdk" suffix and not ".app"?
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.
Because JDK bundles has ".jdk" suffix. Why it should be ".app"?
The description is missing some important changes:
I like the idea of reading the package's version from the JDK's release file when bundling a runtime package, but it is out of the scope of the "jpackage produces invalid Java runtime DMG bundles" fix. It should be a separate fix. |
I updated description. |
8351073: [macos] jpackage produces invalid Java runtime DMG bundles [v2]
|
TKit.error(ex.getMessage()); | ||
return null; |
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 is the point of this exception "handling"? What problem does it solve? What is the caller supposed to do with the null
?
@@ -88,6 +90,31 @@ enum Token { | |||
|
|||
return appImageCmd.outputBundle().toString(); | |||
}), | |||
INVALID_JDK_BUNDLE(cmd -> { |
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.
Would INVALID_MAC_JDK_BUNDLE
name be more accurate?
8351073: [macos] jpackage produces invalid Java runtime DMG bundles [v3]
|
@sashamatveev this pull request can not be integrated into git checkout JDK-8351073
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
Fixed jpackage to produce valid Java runtimes based on description below:
Definitions:
jpackage output based on input:
Additional changes:
Progress
Warnings
Issue
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25314/head:pull/25314
$ git checkout pull/25314
Update a local copy of the PR:
$ git checkout pull/25314
$ git pull https://git.openjdk.org/jdk.git pull/25314/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25314
View PR using the GUI difftool:
$ git pr show -t 25314
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25314.diff
Using Webrev
Link to Webrev Comment