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