RFR: 8333664: Decouple command line parsing and package building in jpackage [v4]

Fri May 16 01:53:01 UTC 2025

On Sat, 10 May 2025 19:02:45 GMT, Alexey Semenyuk <asemenyuk at 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