On Wed, 7 May 2025 12:26:54 GMT, Alexey Semenyuk <asemen...@openjdk.org> wrote:
>> 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.
For links we creating link and corresponding text file, so for it we should put
both files into folder and add it to `--app-content app-content-0/Resources`.
Note `app-content-0` is temp folder with random name and if we using it
directly we end up copying `app-content-0` into application bundle. In such
case it will not be possible to figure out location of link without putting it
under known folder name such as "Resources".
"name" is to figure out if it link and if link it should be under "Resources".
> 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
Fixed.
> 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
Fixed.
> 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");
> }
Fixed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24974#discussion_r2080659287
PR Review Comment: https://git.openjdk.org/jdk/pull/24974#discussion_r2080659397
PR Review Comment: https://git.openjdk.org/jdk/pull/24974#discussion_r2080659613
PR Review Comment: https://git.openjdk.org/jdk/pull/24974#discussion_r2080659694
