On Thu, 20 Aug 2026 22:20:47 GMT, Alexander Matveev <[email protected]> wrote:
> - Added `--app-resources`. > > Specification: > > --app-resources > > A File.pathSeparator separated list of paths > > Additional application files and directories to copy into the > application resources directory. > > This option may be specified multiple times. > > Destination: > Windows: application image root > Linux: application image lib directory > macOS: Contents/Resources > > > --------- > - [x] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). Changes requested by asemenyuk (Reviewer). src/jdk.jpackage/share/classes/jdk/jpackage/internal/resources/HelpResources.properties line 173: > 171: \ Windows: application image root\n\ > 172: \ Linux: application image lib directory\n\ > 173: \ macOS: Contents/Resources The Windows-specific descript should not mention other platfdorms. Lets have separate descriptions for all platforms with tailored wording. src/jdk.jpackage/share/man/jpackage.md line 283: > 281: Windows: application image root > 282: Linux: application image lib directory > 283: macOS: Contents/Resources I'm not sure the indentation is preserved in the output html. Could you check that? test/jdk/tools/jpackage/share/AppContentAndResourcesTest.java line 85: > 83: * --jpt-run=AppContentAndResourcesTest > 84: */ > 85: public class AppContentAndResourcesTest { I don't think we need to rename the class. The next time we add a new similar option and add it to the name it will become unreadable. test/jdk/tools/jpackage/share/AppContentAndResourcesTest.java line 229: > 227: > 228: private enum AppFilesOption { > 229: CONTENT("--app-content", ",", > ApplicationLayout::contentDirectory, true), You can use `TKit.isOSX()` instead of `true` and avoid calling it from `wrapInResourcesOnMac()` test/jdk/tools/jpackage/share/AppContentAndResourcesTest.java line 280: > 278: } > 279: > 280: private record TestCase(AppFilesOption option, TestSpec spec) { I think this new abstraction is redundant. You can just add `AppFilesOption option` field to the existing `TestSpec` and `TestSpec.Builder` classes. test/jdk/tools/jpackage/share/AppContentAndResourcesTest.java line 472: > 470: @FunctionalInterface > 471: private interface ContentFactory { > 472: Content create(JPackageCommand cmd, AppFilesOption option); I think `AppFilesOption option` is excessive. `boolean srcRootMustBeResourcesDir` should be sufficient. test/jdk/tools/jpackage/share/AppImageFillOrderTest.java line 131: > 129: @Parameter("false") > 130: @Parameter("true") > 131: public void testAppResourcesOverrideAppContent(boolean > resourcesFirst) throws IOException { The app image bundling test case looks good. We also need coverage for native packaging. ------------- PR Review: https://git.openjdk.org/jdk/pull/32474#pullrequestreview-4988761650 PR Review Comment: https://git.openjdk.org/jdk/pull/32474#discussion_r3826428655 PR Review Comment: https://git.openjdk.org/jdk/pull/32474#discussion_r3826050310 PR Review Comment: https://git.openjdk.org/jdk/pull/32474#discussion_r3826414285 PR Review Comment: https://git.openjdk.org/jdk/pull/32474#discussion_r3826146667 PR Review Comment: https://git.openjdk.org/jdk/pull/32474#discussion_r3826257823 PR Review Comment: https://git.openjdk.org/jdk/pull/32474#discussion_r3826240574 PR Review Comment: https://git.openjdk.org/jdk/pull/32474#discussion_r3826122512
