Re: RFR: JDK-8238225: Issues reported after replacing symlink at Contents/MacOS/libjli.dylib with binary
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 Sent: Mittwoch, 5. Februar 2020 10:54 To: Erik Joelsson ; core-libs-dev ; build-dev ; Langer, Christoph 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
RE: RFR: JDK-8238225: Issues reported after replacing symlink at Contents/MacOS/libjli.dylib with binary
Hi Erik, > 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. Looks good. Given there are other even longer lines, maybe you would want to join the comment lines 523 and 524? > > 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. Yep, it wouldn't always be verbatim correct, e.g. if jdk/jdk advances to jdk16, it'll be wrong. However maybe you could still strip the "open" from the parameter "TEST=open/test/jdk/tools/launcher/JliLaunchTest.java" in line 66? I guess that would be most correct for OpenJDK... But anyway, the new webrev looks good. No need for further update/discussion from my end. The sooner it's in the merrier I am Cheers Christoph
Re: RFR: JDK-8238225: Issues reported after replacing symlink at Contents/MacOS/libjli.dylib with binary
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 Sent: Mittwoch, 5. Februar 2020 10:54 To: Erik Joelsson ; core-libs-dev ; build-dev ; Langer, Christoph 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
RE: RFR: JDK-8238225: Issues reported after replacing symlink at Contents/MacOS/libjli.dylib with binary
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 > Sent: Mittwoch, 5. Februar 2020 10:54 > To: Erik Joelsson ; core-libs-dev d...@openjdk.java.net>; build-dev ; Langer, > Christoph > 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 > >>
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
RE: RFR: JDK-8238225: Issues reported after replacing symlink at Contents/MacOS/libjli.dylib with binary
Hi Erik,
> Does anyone have an opinion on this?
Yep, first of all, thanks for doing this work. Sorry for not replying earlier
but FOSDEM/openjdkcw has kept us a bit busy. We're in the process of testing
your change in the reported cases where behavior is broken and shall supply you
with results soon.
There's one nit in the coding I've spotted:
Line 532: if (0 == strncmp(realPathToSelf + indexOfLastPathComponent,
altLastPathComponent, sizeOfLastPathComponent - 1)) {
I'm not sure if you want to subtract 1 from sizeOfLastPathComponent here again,
as I believe this was done in line 526 already and sizeOfLastPathComponent
should already reflect the strlen of the string you want to compare to.
Best regards
Christoph
Re: RFR: JDK-8238225: Issues reported after replacing symlink at Contents/MacOS/libjli.dylib with binary
On 04/02/2020 17:45, Erik Joelsson wrote: Does anyone have an opinion on this? I think this looks okay, would be good if Henry could look at it too as we've been bitten by changes in this area, I think with Eclipse Tools that are locating in unusual ways (we think Info.plist). I was happy to see that that JliLaunchTest was found and updated. -Alan
Re: RFR: JDK-8238225: Issues reported after replacing symlink at Contents/MacOS/libjli.dylib with binary
Does anyone have an opinion on this? /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
