RFR: JDK-8238225: Issues reported after replacing symlink at Contents/MacOS/libjli.dylib with binary

[Erik Joelsson]

Hello,

New webrev: http://cr.openjdk.java.net/~erikj/8238225/webrev.02/

On 2020-02-05 02:17, Langer, Christoph wrote:
> Hi,
>
> we've tested the patch and all reported failure scenarios we're aware of are fixed with that, so basically, ship it 😊
>
> As for the code review part:
> The patch I've tested had removed the "-1" in line 532, so that seems to be correct. As Magnus wrote, the pattern seems to be copied from the lines above. So I think in line 518, subtracting -1 from "sizeOfLastPathComponent" seems to be incorrect as well. So it could be corrected in the same fix, I guess.
Yes, I did indeed just copy the pattern, but it also seems you are right 
about the -1, so I fixed it in both locations. I also figured reusing 
the variables wasn't very nice, so add the "Alt" prefix in all of them.
> And there's one other minor thing: I tried to execute JliLaunchTest for the bundle scenario and had to make some adaptions to the example call to "make test-only ..." in line 66 of test/jdk/tools/launcher/JliLaunchTest.java. I guess the example could be more generic if it were changed to:
>
> 66                 // $ make test-only TEST=test/jdk/tools/launcher/JliLaunchTest.java \
> 67                 //     JDK_IMAGE_DIR=$PWD/images/jdk-bundle/jdk-15.jdk/Contents/Home
>
> (e.g. use relative paths inside the build directory)

Right, the name of the output dir may change. I didn't intend the 
example to be verbatim correct for everyone (hence the "something like" 
wording) but I see your point. Changed it.

/Erik

> Thanks
> Christoph
>
>> -----Original Message-----
>> From: Magnus Ihse Bursie <magnus.ihse.bursie at oracle.com>
>> Sent: Mittwoch, 5. Februar 2020 10:54
>> To: Erik Joelsson <erik.joelsson at oracle.com>; core-libs-dev <core-libs-
>> dev at openjdk.java.net>; build-dev <build-dev at openjdk.java.net>; Langer,
>> Christoph <christoph.langer at sap.com>
>> Subject: Re: RFR: JDK-8238225: Issues reported after replacing symlink at
>> Contents/MacOS/libjli.dylib with binary
>>
>> On 2020-02-04 18:45, Erik Joelsson wrote:
>>> Does anyone have an opinion on this?
>> The solution seems sound to me. I'm having a hard time figuring out if
>> the string manipulations in java_md_macosx.m are correct; as Christoph
>> is saying, they might not be. I realize you have copied an existing
>> pattern, and is likely subject to constraints, but that does not make it
>> easier to read.
>>
>> /Magnus
>>> /Erik
>>>
>>> On 2020-01-31 07:31, Erik Joelsson wrote:
>>>> In JDK-8235687 the MacOS bundle distribution of the JDK was changed
>>>> to conform to Apple requirements by changing
>>>> Contents/MacOS/libjli.dylib from a symlink into
>>>> ../Home/lib/libjli.dylib to a copy of that file. The problem with
>>>> having a symlink there is that Contents/MacOS/libjli.dylib is the
>>>> declared CFBundleExecutable of the bundle and that executable may not
>>>> be a symlink. The history around why that particular library was put
>>>> there seems lost in ancient history. All we know is that it was there
>>>> when Apple donated the Mac port and according to this bug report,
>>>> there are users out there relying on it.
>>>>
>>>> When changing Contents/MacOS/libjli.dylib to a copy, loading that
>>>> dylib and using it to launch a JVM no longer works. This patch fixes
>>>> that by making libjli.dylib aware of potentially being located there
>>>> and if so, finding the JDK home dir in ../Home.
>>>>
>>>> I've also expanded the existing test for launching a JVM through
>>>> libjli.dylib directly to also test this location when possible. In
>>>> local testing, this will not be covered unless the user explicitly
>>>> specifies that the JDK under test should be the bundle image on the
>>>> command line like this:
>>>>
>>>> $ make test-only TEST=open/test/jdk/tools/launcher/JliLaunchTest.java
>>>> JDK_IMAGE_DIR=$PWD/build/macosx-x64/images/jdk-bundle/jdk-
>> 15.jdk/Contents/Home
>>>> But, at least in Oracle's distributed testing, the JDK on MacOS is
>>>> distributed in the bundle layout, so there this functionality will be
>>>> tested.
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8238225
>>>>
>>>> Webrev: http://cr.openjdk.java.net/~erikj/8238225/webrev.01/
>>>>
>>>> /Erik
>>>>