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

[Langer, Christoph]

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