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.bur...@oracle.com> > Sent: Mittwoch, 5. Februar 2020 10:54 > To: Erik Joelsson <erik.joels...@oracle.com>; core-libs-dev <core-libs- > d...@openjdk.java.net>; build-dev <build-...@openjdk.java.net>; Langer, > Christoph <christoph.lan...@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 > >>
