RFR: 8352480: Don't follow symlinks in additional content for app images [v3]

[Alexey Semenyuk]

On Wed, 7 May 2025 02:07:03 GMT, Alexander Matveev <almatvee at openjdk.org> wrote:

>> - Symbolic links will not be followed for `--app-content` on all platforms.
>> - Added test to cover this case.
>> - `MacHelper` updated to use "cp -R" instead of "cp -r" when unpacking DMG, since "cp -r" on macOS is not documented option and when used `cp` will follow symbolic links which breaks test. "cp -R" does not follow symbolic links.
>
> Alexander Matveev has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8352480: [macos] Don't follow symlinks in additional content for app images [v5]

test/jdk/tools/jpackage/share/AppContentTest.java line 116:

> 114:     private static Path getAppContentPath(JPackageCommand cmd, Path name) {
> 115:         Path contentDir = cmd.appLayout().contentDirectory();
> 116:         // Links are always created in "Resources"

> Links are always created in "Resources"

All additional content on macOS is created in the "Resources" directory. On Linux there is no such requirement.

The return value of `getAppContentPath()` is not supposed to depend on the additional content type; the new "name" parameter doesn't make sense to me.

test/jdk/tools/jpackage/share/AppContentTest.java line 147:

> 145: 
> 146:         private static Path copyAppContentPath(Path appContentPath) throws IOException {
> 147:             Path appContentArg = TKit.createTempDirectory("app-content").resolve("Resources");

This is a redundant change

test/jdk/tools/jpackage/share/AppContentTest.java line 149:

> 147:             Path appContentArg = TKit.createTempDirectory("app-content").resolve("Resources");
> 148:             var srcPath = TKit.TEST_SRC_ROOT.resolve(appContentPath);
> 149:             Path dstPath = appContentArg.resolve(srcPath.getFileName());

This is a redundant change

test/jdk/tools/jpackage/share/AppContentTest.java line 164:

> 162:         private static List<Path> initAppContentPaths(List<Path> appContentPaths) {
> 163:             boolean copy = (copyInResources || appContentPaths.stream()
> 164:                             .anyMatch(s -> s.toString().contains("Link")));

`s.toString().contains("Link")` is a lousy way to detect if the path is a symlink. "s" can be an absolute path (to the local OpenJDK repo) that may have a "Link" substring in one of path components, like "/home/BLink/my-projects/open/test/jdk/tools/jpackage/non-existant". The test will fail, and we will have a very hard time figuring out the cause. I'd fix it by narrowing the scope from the full path to the filename; create a dedicated function for detecting if the given path is supposed to be a symlink instead of repeating `.toString().contains("Link")`:

private static boolean isSymlinkPath(Path v) {
    return v.getFileName().toString().contains("Link");
}

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/24974#discussion_r2077510440
PR Review Comment: https://git.openjdk.org/jdk/pull/24974#discussion_r2077512476
PR Review Comment: https://git.openjdk.org/jdk/pull/24974#discussion_r2077512103
PR Review Comment: https://git.openjdk.org/jdk/pull/24974#discussion_r2077542387