On Sat, 10 May 2025 19:02:45 GMT, Alexey Semenyuk <asemen...@openjdk.org> wrote:
>> Refactor jpackage to separate the configuration and execution phases. >> At the configuration phase, jpackage parses command-line arguments and >> validates them. >> At the execution phase, jpackage builds a bundle based on data collected at >> the configuration phase. >> >> There was no clear separation between these phases. Both used the same data >> type (`Map<String, Object>`), making it hard to understand and use properly. >> >> This change introduces data model to jpackage (classes in >> "jdk.jpackage.internal.model" package). The output of the configuration >> phase is either an instance of >> [jdk.jpackage.internal.model.Application](https://github.com/openjdk/jdk/pull/19668/files#diff-e4e7717f1978a09ac4806eded5c7f94aa29b2ea56671545dc053cb83eba86919) >> interface for app image bundling or >> [jdk.jpackage.internal.model.Package](https://github.com/openjdk/jdk/pull/19668/files#diff-9908b5648e03bd8a8104f6f6f5aa08e5df78fbc0508823774d3458b22927b721) >> for native package bundling. >> >> The execution phase has been reworked to get configuration properties from >> the new `jdk.jpackage.internal.model.Application` and >> `jdk.jpackage.internal.model.Package` interfaces instead of extracting data >> from `Map<String, Object>` "params". >> >> Additionally, a notion of "packaging pipeline" >> (jdk.jpackage.internal.PackagingPipeline class) was added to configure >> packaging declaratively with more code sharing between bundlers. >> >> jdk.jpackage module javadoc - >> https://alexeysemenyukoracle.github.io/jpackage-javadoc/jdk.jpackage/module-summary.html >> >> **Functional changes** >> jpackage behavior 99% remains the same, i.e., it produces the same bundles >> for the given parameters. This change affects only the implementation. >> Still, there are some changes in jpackage behavior. They are outlined below. >> >> - Minimize copying of the source app image when doing native packaging. >> >> Before this change, native package bundlers made redundant copies of the >> source app image. E.g., msi and linux package bundlers copied the external >> app image (the one specified with `--app-image` parameter); linux package >> bundlers always transformed the source app image if the installation >> directory was in the "/usr" tree (`--install-dir /usr`). This change >> eliminates all redundant app image copy/transformations. >> >> - PKG bundler: change "preinstall" and "postinstall" scripts in app bundles. >> >> post- and pre- install PKG scripts for SimplePackageTest package before and >> after the change: >> <table> >> <thead> >> <tr> >> <th>Scr... > > Alexey Semenyuk has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 512 commits: > > - Add ConfigFilesStasher that allows to save contents of jpackage build > directories in external directory; Add clean_stashed_files.sh > - Merge branch 'master' into JDK-8333664 > - Remove redundant StandardBundlerParam.createResource() > - Adapt JDK-8352480 fix > - Merge branch 'master' into JDK-8333664 > - Fix javadoc > - Merge branch 'master' into JDK-8333664 > - 8333568: Test that jpackage doesn't modify R/O files/directories > > Reviewed-by: almatvee > - 8356562: SigningAppImageTwoStepsTest test fails > > Reviewed-by: almatvee > - Remove clean_stashed_files.sh > - ... and 502 more: https://git.openjdk.org/jdk/compare/43696030...011eb710 Looks good with minor comments. src/jdk.jpackage/linux/classes/jdk/jpackage/internal/DesktopIntegration.java line 337: > 335: */ > 336: private InstallableFile createDesktopFile(String fileName) { > 337: var srcPath = > pkg.asPackageApplicationLayout().orElseThrow().resolveAt(env.appImageDir()).destktopIntegrationDirectory().resolve(fileName); `destktop` -> `desktop` src/jdk.jpackage/linux/classes/jdk/jpackage/internal/DesktopIntegration.java line 477: > 475: return Optional.of(fa) > 476: .flatMap(FileAssociation::icon) > 477: .map(DesktopIntegration::getSquareSizeOfImage) `getSquareSizeOfImage` will return 0, if exception is thrown for invalid image. Should it return -1 or should we only accept icons if `getIconSize() > 0`? src/jdk.jpackage/share/classes/jdk/jpackage/internal/AppImageDesc.java line 33: > 31: import jdk.jpackage.internal.model.ApplicationLayout; > 32: > 33: record AppImageDesc(AppImageLayout appImageLyout, Path path) { `appImageLyout` -> `appImageLayout` src/jdk.jpackage/share/classes/jdk/jpackage/internal/BuildEnvBuilder.java line 51: > 49: } else if (!Files.isDirectory(root)) { > 50: throw exceptionBuilder.create(); > 51: } else { Maybe rewrite to: if (!Files.isDirectory(root)) { throw exceptionBuilder.create(); } else if (Files.exists(root)) { ... } src/jdk.jpackage/share/classes/jdk/jpackage/internal/PackageBuilder.java line 65: > 63: installDirName = Optional.empty(); > 64: } > 65: case MAC_DMG,MAC_PKG -> { `MAC_DMG,MAC_PKG` -> `MAC_DMG, MAC_PKG` src/jdk.jpackage/share/classes/jdk/jpackage/internal/model/package-info.java line 34: > 32: * All methods of all interfaces and classes in this package return > non-null values unless stated otherwise. > 33: */ > 34: package jdk.jpackage.internal.model; Missing new line. src/jdk.jpackage/share/classes/jdk/jpackage/internal/util/CompositeProxy.java line 200: > 198: * static final CompositeProxyTunnel INSTANCE = new > CompositeProxyTunnel(); > 199: * } > 200: * } Extra `}` src/jdk.jpackage/windows/classes/jdk/jpackage/internal/WixAppImageFragmentBuilder.java line 940: > 938: var launchers = > Optional.ofNullable(pkg.app().launchers()).map( > 939: List::stream).orElseGet(Stream::of); > 940: return launchers.map(launcher -> { Extra space. ------------- PR Review: https://git.openjdk.org/jdk/pull/19668#pullrequestreview-2835056108 PR Review Comment: https://git.openjdk.org/jdk/pull/19668#discussion_r2085771680 PR Review Comment: https://git.openjdk.org/jdk/pull/19668#discussion_r2085785398 PR Review Comment: https://git.openjdk.org/jdk/pull/19668#discussion_r2089916849 PR Review Comment: https://git.openjdk.org/jdk/pull/19668#discussion_r2089955028 PR Review Comment: https://git.openjdk.org/jdk/pull/19668#discussion_r2090064291 PR Review Comment: https://git.openjdk.org/jdk/pull/19668#discussion_r2090100387 PR Review Comment: https://git.openjdk.org/jdk/pull/19668#discussion_r2092111270 PR Review Comment: https://git.openjdk.org/jdk/pull/19668#discussion_r2092154401
