RFR: 8356047: [macos] jpackage produces confusing post- and pre- installation PKG scripts [v3]
Alexey Semenyuk
asemenyuk at openjdk.org
Fri Sep 26 12:21:32 UTC 2025
On Fri, 26 Sep 2025 00:51:57 GMT, Alexander Matveev <almatvee at openjdk.org> wrote:
>> - Removed pre- and post- installation PKG scripts.
>> - This code is not needed and PKG should create destination folder and set correct permissions.
>> - If for some reason it is not happens, permissions issues should be fixed when jpackage prepares application bundle. PKG should keep all permissions unchanged when packaging and installing bundle.
>> - Users will have ability to provide pre- and post- installation PKG scripts if needed.
>> - `INSTALL_LOCATION` and `APP_LOCATION` substitution is removed, since `$1` argument in scripts is same as `INSTALL_LOCATION`.
>> - I think code in these scripts are some legacy leftovers.
>> - Added test to test that pre- and post-scripts are no longer exist and can be added via `--resource-dir`.
>
> 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 four additional commits since the last revision:
>
> - Merge remote-tracking branch 'upstream/master' into JDK-8356047
> - 8356047: [macos] jpackage produces confusing post- and pre- installation PKG scripts [v2]
> - Merge remote-tracking branch 'upstream/master' into JDK-8356047
> - 8356047: [macos] jpackage produces confusing post- and pre- installation PKG scripts
Changes requested by asemenyuk (Reviewer).
A few issues with the coverage:
- Missing running custom scripts.
- Missing test case when only a custom install script is provided.
- Missing test case when only a custom uninstall script is provided.
src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/MacPkgPackager.java line 358:
> 356: data.put("INSTALL_LOCATION", Path.of("/").resolve(pkg.relativeInstallDir()).toString());
> 357: data.put("APP_LOCATION", appLocation.toString());
> 358:
I'd keep the context for custom scripts unchanged. If somebody relies on these variables in their custom scripts, they will stop working after this change.
src/jdk.jpackage/share/classes/jdk/jpackage/internal/OverridableResource.java line 237:
> 235: Log.verbose(I18N.format("message.no-default-resource",
> 236: publicName, getPrintableCategory(), publicName));
> 237: }
Why would we write this log message if there is no consumer for the resource?
src/jdk.jpackage/unix/classes/jdk/jpackage/internal/PackageScripts.java line 81:
> 79: static class ResourceConfig {
> 80:
> 81: ResourceConfig(String defaultName, String categoryId, boolean noDefault) {
Don't you think `String defaultName, String categoryId, boolean noDefault` signature looks odd? You have a parameter specifying the default name and another parameter that specifies if the default name is set or not.
Wouldn't:
ResourceConfig(Optional<String> defaultName, String categoryId)
be less confusing?
For backward compatibility keep the old ctor, but redefine it:
ResourceConfig(String defaultName, String categoryId) {
this(Optional.of(defaultName), categoryId);
}
test/jdk/tools/jpackage/macosx/PkgScriptsTest.java line 28:
> 26: * @summary jpackage with --type pkg --resource-dir Scripts
> 27: * @library /test/jdk/tools/jpackage/helpers
> 28: * @key jpackagePlatformPackage
I guess, we don't want SQE to use artifacts from this test in manual testing. That said, we need
jtreg `@requires (jpackage.test.SQETest == null)` condition.
-------------
PR Review: https://git.openjdk.org/jdk/pull/25510#pullrequestreview-2876261203
PR Comment: https://git.openjdk.org/jdk/pull/25510#issuecomment-3338414361
PR Review Comment: https://git.openjdk.org/jdk/pull/25510#discussion_r2112642131
PR Review Comment: https://git.openjdk.org/jdk/pull/25510#discussion_r2382202155
PR Review Comment: https://git.openjdk.org/jdk/pull/25510#discussion_r2382214753
PR Review Comment: https://git.openjdk.org/jdk/pull/25510#discussion_r2382226244
More information about the core-libs-dev
mailing list