On Wed, 7 May 2025 02:07:03 GMT, Alexander Matveev <almat...@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
