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

[Langer, Christoph]

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.

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)

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