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

[Magnus Ihse Bursie]

On 2020-02-05 16:32, Erik Joelsson wrote:
> Hello,
>
> New webrev: http://cr.openjdk.java.net/~erikj/8238225/webrev.02/
Looks good.

/Magnus
>
> 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
>>>>>