Re: RFR: 8164805: Fail to create a MR modular JAR with a versioned entry in base-versioned empty package

[Steve Drach]

 There is a new webrev at 
 http://cr.openjdk.java.net/~sdrach/8164805/webrev.01/ 
>>> 
>>> sun/tools/jar/Main.java
>>> 
>>> Thanks for refactoring and adding the findConcealedPackages method.  What I 
>>> actually meant was to move out this line:
>>>  concealedPackages = findConcealedPackages(rd);
>>> 
>>> to probably before calling addExtendedModuleAttributes(moduleInfos) above 
>>> line 342 and 1101.
>> 
>> I tried that and decided it was non-optimal because we’d have to construct 
>> the ModuleDescriptor from the modInfos twice in succession.  Let's 
>> compromise here, okay?
> 
> OK.  It wasn’t clear to me that you considered that.  It’s fine to leave it 
> for a future clean up.
> 
>> 
>>> 
>>> 2014 .filter(p -> !p.equals("”))
>> 
>> 
>>> 
>>> For a modular JAR, there should be no unnamed package.  I think the jar 
>>> tool should fail if it detects an unnamed package.  Your test does not have 
>>> any unnamed package - how did you find this?
>> 
>> the modularJar/Basic test found a bug.  Then when I was fixing the bug in 
>> toPackageNames I noticed it could return unnamed packages (“”).  And in fact 
>> there are some, a few, classes that aren’t in a package.  Jar tool shouldn’t 
>> fail with unnamed packages.
> 
> I meant for modular JAR.
> 
>> They could even exist in a modular multi-release jar when the module-info 
>> class is only in a versioned directory.  
> 
> module-info.class should be filtered before calling toPackageName.

It is, although not in a stream.  My inspection of usages led me to an unused 
method, findPackages, that I will remove (I did so and all tests passed).

> 
>> I guess it should fail if a class in an unnamed package is in a module, 
>> although I’m not even sure about that.
> 
> 
> Mandy

Re: RFR: 8164805: Fail to create a MR modular JAR with a versioned entry in base-versioned empty package

[Steve Drach]

>> There is a new webrev at 
>> http://cr.openjdk.java.net/~sdrach/8164805/webrev.01/ 
> 
> sun/tools/jar/Main.java
> 
> Thanks for refactoring and adding the findConcealedPackages method.  What I 
> actually meant was to move out this line:
>concealedPackages = findConcealedPackages(rd);
> 
> to probably before calling addExtendedModuleAttributes(moduleInfos) above 
> line 342 and 1101.

I tried that and decided it was non-optimal because we’d have to construct the 
ModuleDescriptor from the modInfos twice in succession.  Let's compromise here, 
okay?

> 
> 2014 .filter(p -> !p.equals("”))


> 
> For a modular JAR, there should be no unnamed package.  I think the jar tool 
> should fail if it detects an unnamed package.  Your test does not have any 
> unnamed package - how did you find this?

the modularJar/Basic test found a bug.  Then when I was fixing the bug in 
toPackageNames I noticed it could return unnamed packages (“”).  And in fact 
there are some, a few, classes that aren’t in a package.  Jar tool shouldn’t 
fail with unnamed packages.  They could even exist in a modular multi-release 
jar when the module-info class is only in a versioned directory.  I guess it 
should fail if a class in an unnamed package is in a module, although I’m not 
even sure about that.

> 
> ConcealedPackage.java test
> 
> Thanks for improving the test.  It’d be good to name the @Test method with a 
> descriptive method name e.g. 
>   test1 -> testUpdateVersionedPublicClass
>   test2 -> testUpdatedVersionedPublicConcealedClass

It’s difficult to come up with names that aren’t sentences.  I figured the 
comments would explain the test adequately.

> 
> 117 @Test // updates a valid multi-release jar with a new public class in
> 118   // versioned section and fails
> 
> Nit: You can consider moving the comment above @Test.

Ok

Re: RFR: 8164805: Fail to create a MR modular JAR with a versioned entry in base-versioned empty package

[Steve Drach]

There is a new webrev at http://cr.openjdk.java.net/~sdrach/8164805/webrev.01/ 
 that addresses the 
issues raised by Mandy.  Comments inline.

>> issue: https://bugs.openjdk.java.net/browse/JDK-8164805
>> webrev: http://cr.openjdk.java.net/~sdrach/8164805/webrev.00/ 
> 
> Issue a warning is good in the case public classes are added in a concealed 
> package.  Some comments:
> 
> Main.java
> 
> addExtendedModuleAttributes does not seem to be the appropriate method to 
> initialize concealedPackages.  It would be better to set concealedPackages in 
> a separate method that will be called for each modular JAR.

Done, although I still don’t see an advantage to this.

> 
> Validator.java
> 160 
> main.error(Main.formatMsg("error.validator.concealed.public.class", 
> entryName));
> 
> This is a warning.  You should add a new warn method to make it clear.
> Similiarly,  “error.validator.concealed.public.class” should be renamed to 
> “warn.validator.concealed.public.class”.  I notice there are several keys 
> with “error.validator” prefix that are meant for warnings.  It’d be good to  
> change them too.  

All done.

> 
> 247 if (idx > -1) {
> 248 String pkgName = internalName.substring(0, idx).replace('/', 
> '.');
> 249 if (main.concealedPackages.contains(pkgName)) {
> 250 return true;
> 251 }
> 252 }
> 
> Alternative impl for the above lines:
> 
> String pkgName = idx != -1 ? internalName.substring(0, idx).replace('/', '.’)
>  : “”;
> return main.concealedPackages.contains(pkgName);

Done.

> 
> jar.properties
> 
> 112 error.validator.concealed.public.class=\
> 113  warning - entry {0} is a public class in a concealed package, 
> placing this \
> 114  jar on the class path will result in incompatible public 
> interfaces
> 
> line 113 needs new line ‘\n’.  You may break it into 3 lines
> since the entry name will take up some characters.

Done.

> 
> ConcealedPackage.java test
> 
>  60 Path destination = userdir.resolve("classes”);
> 
> I suggest to use Paths.get(“classes”) rather than ${user.dir}.  jtreg will 
> clean up the JTwork/scratch directory after the test run.

Doen, although I still clean up after tests.

> 
>  63 // add an empty directory
>  64 
> Files.createDirectory(destination.resolve("p").resolve("internal"));
> 
> I suggest to take this out.  The test verifies if "jar tf mmr.jar” succeeds.

Done.

> 
> The test should test both “cf” and “uf” and verify that the ConcealedPackage 
> attributes in module-info.class is updated correctly (one single 
> module-info.class case in the root entry and two module-info.class - root and 
> versioned).

Done, but perhaps not exactly the suggested way.  The test has been 
considerably beefed up.

> 
>  92 private int jar(String cmd) {
> Nit: this can simply take a varargs and no need to split:
>  jar(String… options)

I left it the way it was.

> 
> 
>  76 private String files(Path start) throws IOException {
> Perhaps return Stream.  That can replace the unnecessary concat in a 
> String and then later split to pass to javac.

Done.

Also fixed a bug I found Main::toPackageName

Re: RFR: 8164805: Fail to create a MR modular JAR with a versioned entry in base-versioned empty package

[Steve Drach]

>> On Oct 19, 2016, at 5:05 PM, Steve Drach  wrote:
>> 
>>>> issue: https://bugs.openjdk.java.net/browse/JDK-8164805
>>>> webrev: http://cr.openjdk.java.net/~sdrach/8164805/webrev.00/ 
>>> 
>>> Issue a warning is good in the case public classes are added in a concealed 
>>> package.  Some comments:
>>> 
>>> Main.java
>>> 
>>> addExtendedModuleAttributes does not seem to be the appropriate method to 
>>> initialize concealedPackages.  It would be better to set concealedPackages 
>>> in a separate method that will be called for each modular JAR.
>> 
>> I made a simple change to existing code, I wasn’t intending to make 
>> significant changes to jar tool.  Of course as time goes on, jar tool 
>> continues to grow into a bigger hair ball.  It would definitely benefit from 
>> major cosmetic surgery.  Perhaps I don’t understand the point you are trying 
>> to make.
> 
> It made it hard for review and future maintainability.   It was not obvious 
> to me when I reviewed the code whether this misses any case.
> 
> Refactoring it is a small change.

Just to be clear, you’d like me to take lines 1988-1995 and put them in a 
separate method that gets called by addExtendedModuleAttributes?


> 
>> 
>>> 
>>> ConcealedPackage.java test
>>> 
>>> 60 Path destination = userdir.resolve("classes”);
>>> 
>>> I suggest to use Paths.get(“classes”) rather than ${user.dir}.
>> 
>> Is there a performance advantage or some other reason for doing this?  The 
>> way I do it seems reasonable.
> 
> I just want to point out that jtreg will do the clean up if you use the 
> scratch directory.
> 
>> 
>>> jtreg will clean up the JTwork/scratch directory after the test run.
>> 
>> That’s what the docs say but I’ve seen problems in the past with windows 
>> machines, so I just got in the habit
>> of doing my own clean up.
>> 
> 
> Up to you.
> 
>>> 
>>> 63 // add an empty directory
>>> 64 
>>> Files.createDirectory(destination.resolve("p").resolve("internal"));
>>> 
>>> I suggest to take this out.  The test verifies if "jar tf mmr.jar” succeeds.
>> 
>> Ok.  Just trying to make it exactly the same as the jar structure in the bug 
>> report.
> 
> I updated the JBS issue to clarify that per the recent discussion.
> 
>> 
>>> 
>>> 92 private int jar(String cmd) {
>>> Nit: this can simply take a varargs and no need to split:
>>> jar(String… options)
>> 
>> I like the String because it’s more readable and I suspect the split isn’t 
>> that costly.
>> 
> 
> I just point out that it’s a kind of silly to concat all args and then split 
> it. 
> 
> Mandy
> 

Re: RFR: 8164805: Fail to create a MR modular JAR with a versioned entry in base-versioned empty package

[Steve Drach]

>  76 private String files(Path start) throws IOException {
> Perhaps return Stream.  That can replace the unnecessary concat in a 
> String and then later split to pass to javac.

Is this better?

private void javac(Path source, Path destination) throws IOException {
Stream prefix = Stream.of("-d", destination.toString());
Stream suffix = Files.isDirectory(source)
? Files.walk(source)
   .map(Path::toString)
   .filter(s -> s.endsWith(".java"))
: Stream.of(source.toString());
String[] args = Stream.concat(prefix, suffix).toArray(String[]::new);
JAVAC_TOOL.run(System.out, System.err, args);
}

Re: RFR: 8164805: Fail to create a MR modular JAR with a versioned entry in base-versioned empty package

[Steve Drach]

>> issue: https://bugs.openjdk.java.net/browse/JDK-8164805
>> webrev: http://cr.openjdk.java.net/~sdrach/8164805/webrev.00/ 
> 
> Issue a warning is good in the case public classes are added in a concealed 
> package.  Some comments:
> 
> Main.java
> 
> addExtendedModuleAttributes does not seem to be the appropriate method to 
> initialize concealedPackages.  It would be better to set concealedPackages in 
> a separate method that will be called for each modular JAR.

I made a simple change to existing code, I wasn’t intending to make significant 
changes to jar tool.  Of course as time goes on, jar tool continues to grow 
into a bigger hair ball.  It would definitely benefit from major cosmetic 
surgery.  Perhaps I don’t understand the point you are trying to make.

> 
> Validator.java
> 160 
> main.error(Main.formatMsg("error.validator.concealed.public.class", 
> entryName));
> 
> This is a warning.  You should add a new warn method to make it clear.

Ok.

> Similiarly,  “error.validator.concealed.public.class” should be renamed to 
> “warn.validator.concealed.public.class”.  I notice there are several keys 
> with “error.validator” prefix that are meant for warnings.  It’d be good to  
> change them too.  
> 
> 247 if (idx > -1) {
> 248 String pkgName = internalName.substring(0, idx).replace('/', 
> '.');
> 249 if (main.concealedPackages.contains(pkgName)) {
> 250 return true;
> 251 }
> 252 }
> 
> Alternative impl for the above lines:
> 
> String pkgName = idx != -1 ? internalName.substring(0, idx).replace('/', '.’)
>  : “”;
> return main.concealedPackages.contains(pkgName);

Ok.

> 
> jar.properties
> 
> 112 error.validator.concealed.public.class=\
> 113  warning - entry {0} is a public class in a concealed package, 
> placing this \
> 114  jar on the class path will result in incompatible public 
> interfaces
> 
> line 113 needs new line ‘\n’.  You may break it into 3 lines
> since the entry name will take up some characters.

Ok.

> 
> ConcealedPackage.java test
> 
>  60 Path destination = userdir.resolve("classes”);
> 
> I suggest to use Paths.get(“classes”) rather than ${user.dir}.

Is there a performance advantage or some other reason for doing this?  The way 
I do it seems reasonable.

>  jtreg will clean up the JTwork/scratch directory after the test run.

That’s what the docs say but I’ve seen problems in the past with windows 
machines, so I just got in the habit
of doing my own clean up.

> 
>  63 // add an empty directory
>  64 
> Files.createDirectory(destination.resolve("p").resolve("internal"));
> 
> I suggest to take this out.  The test verifies if "jar tf mmr.jar” succeeds.

Ok.  Just trying to make it exactly the same as the jar structure in the bug 
report.

> 
> The test should test both “cf” and “uf” and verify that the ConcealedPackage 
> attributes in module-info.class is updated correctly (one single 
> module-info.class case in the root entry and two module-info.class - root and 
> versioned).

Ok.

> 
>  92 private int jar(String cmd) {
> Nit: this can simply take a varargs and no need to split:
>  jar(String… options)

I like the String because it’s more readable and I suspect the split isn’t that 
costly.

> 
> 
>  76 private String files(Path start) throws IOException {
> Perhaps return Stream.  That can replace the unnecessary concat in a 
> String and then later split to pass to javac.

What we probably want is stream.toArray(String[]::new), but then we’d have to 
copy that array into a new array that is 2 elements larger to accommodate the 
destination parameter.  I wonder if that’s any better.  I need to think about 
it.

> 
> Mandy

RFR: 8164805: Fail to create a MR modular JAR with a versioned entry in base-versioned empty package

[Steve Drach]

Hi,

Please review the following changeset.  This fix allows jar tool to add a new 
public class in a versioned directory in a modular multi-release jar file if 
the class is in a concealed package. When this event takes place, a warning 
message is emitted saying that placing this jar on a class path will result in 
an incompatible public interface, the jar file does not meet the single api 
rule of a multi-release jar file.  As before, adding a new public class in a 
versioned directory of a multi-release jar file (not a modular jar file) will 
result in an error.

issue: https://bugs.openjdk.java.net/browse/JDK-8164805 


webrev: http://cr.openjdk.java.net/~sdrach/8164805/webrev.00/ 


Thanks,
Steve

Fwd: RFR: 8167237: Jar tool can not correctly find/process the --release option if it occurs before the file list

[Steve Drach]

Hi again,

Is there anyone willing to review this?  It’s a very simple change.

Thanks
Steve

> Begin forwarded message:
> 
> From: Steve Drach 
> Subject: RFR: 8167237: Jar tool can not correctly find/process the --release 
> option if it occurs before the file list
> Date: October 6, 2016 at 1:50:30 PM PDT
> To: core-libs-dev 
> 
> Hi,
> 
> Please review the changeset that fixes the problem of not “seeing” the jar 
> tool —release  option if it is not preceded by anything other than gnu-style 
> options.  It’s a simple one line change to process —release the same way as 
> -C.
> 
> issue: https://bugs.openjdk.java.net/browse/JDK-8167237 
> <https://bugs.openjdk.java.net/browse/JDK-8167237>webrev: 
> http://cr.openjdk.java.net/~sdrach/8167237/webrev.00/ 
> <http://cr.openjdk.java.net/~sdrach/8167237/webrev.00/>
> 
> Thanks,
> Steve

Re: RFR: 8166460: jdk/internal/util/jar/TestVersionedStream gets Assertion error

[Steve Drach]

As a consequence of some conversations about this test, I’ve removed the part 
that specifically depended on the order of the entries and now just verify that 
the order of the versioned entries is the same relative order as all the 
entries in the jar file.   The updated changeset is 
http://cr.openjdk.java.net/~sdrach/8166460/webrev.03/ 
<http://cr.openjdk.java.net/~sdrach/8166460/webrev.03/>

> On Oct 5, 2016, at 12:27 PM, Steve Drach  wrote:
> 
> Hi,
> 
> Please review the following changeset that creates a replacement for the 
> TestVersionedStream test.  The previous test occasionally failed on Linux 
> hotspot nightly testing due to jar tool sometimes changing the order of the 
> entries.  The new test specifically sets the order of the entries and tests 
> both possible orders.
> 
> issue: https://bugs.openjdk.java.net/browse/JDK-8166460 
> <https://bugs.openjdk.java.net/browse/JDK-8166460>
> webrev: http://cr.openjdk.java.net/~sdrach/8166460/webrev.00/ 
> <http://cr.openjdk.java.net/~sdrach/8166460/webrev.00/>
> 
> Thanks,
> Steve
> 

RFR: 8167237: Jar tool can not correctly find/process the --release option if it occurs before the file list

[Steve Drach]

Hi,

Please review the changeset that fixes the problem of not “seeing” the jar tool 
—release  option if it is not preceded by anything other than gnu-style 
options.  It’s a simple one line change to process —release the same way as -C.

issue: https://bugs.openjdk.java.net/browse/JDK-8167237 
webrev: 
http://cr.openjdk.java.net/~sdrach/8167237/webrev.00/ 


Thanks,
Steve

RFR: 8166460: jdk/internal/util/jar/TestVersionedStream gets Assertion error

[Steve Drach]

Hi,

Please review the following changeset that creates a replacement for the 
TestVersionedStream test.  The previous test occasionally failed on Linux 
hotspot nightly testing due to jar tool sometimes changing the order of the 
entries.  The new test specifically sets the order of the entries and tests 
both possible orders.

issue: https://bugs.openjdk.java.net/browse/JDK-8166460 

webrev: http://cr.openjdk.java.net/~sdrach/8166460/webrev.00/ 


Thanks,
Steve

Re: RFR: 8165944 jar utility doesn't process more than one -C argument

[Steve Drach]

We discovered that the last webrev subtly changed the behavior of jar tool with 
respect to the JDK 8 jar tool, so that was fixed, along with some more 
simplification, and additional test cases were added to demonstrate consistent 
behavior across releases.  Here is the newest webrev.  

http://cr.openjdk.java.net/~sdrach/8165944/webrev.06/ 
<http://cr.openjdk.java.net/~sdrach/8165944/webrev.06/>


> On Sep 27, 2016, at 12:31 PM, Steve Drach  wrote:
> 
> After a discussion with Paul Sandoz, I’ve simplified and, hopefully, thus 
> clarified the changeset.  The new webrev is
> 
> http://cr.openjdk.java.net/~sdrach/8165944/webrev.01/ 
> <http://cr.openjdk.java.net/~sdrach/8165944/webrev.01/>
> 
>> On Sep 26, 2016, at 12:31 PM, Steve Drach > <mailto:steve.dr...@oracle.com>> wrote:
>> 
>> Hi,
>> 
>> Please review these changes to the jar tool to fix a capability regression I 
>> introduced in an earlier revision.  The issue is that this
>> 
>> $ jar -cf test.jar -C test1 . -C test2 .
>> 
>> only puts the files under test1 in the jar and ignores the files under 
>> test2.  The DoubleCs test verified the problem and the solution.
>> 
>> issue: https://bugs.openjdk.java.net/browse/JDK-8165944 
>> <https://bugs.openjdk.java.net/browse/JDK-8165944> 
>> <https://bugs.openjdk.java.net/browse/JDK-8165944 
>> <https://bugs.openjdk.java.net/browse/JDK-8165944>>
>> webrev: http://cr.openjdk.java.net/~sdrach/8165944/webrev.00/ 
>> <http://cr.openjdk.java.net/~sdrach/8165944/webrev.00/> 
>> <http://cr.openjdk.java.net/~sdrach/8165944/webrev.00/ 
>> <http://cr.openjdk.java.net/~sdrach/8165944/webrev.00/>>
>> 
>> Thanks,
>> Steve
>> 
> 

Re: RFR: 8165944 jar utility doesn't process more than one -C argument

[Steve Drach]

After a discussion with Paul Sandoz, I’ve simplified and, hopefully, thus 
clarified the changeset.  The new webrev is

http://cr.openjdk.java.net/~sdrach/8165944/webrev.01/ 
<http://cr.openjdk.java.net/~sdrach/8165944/webrev.01/>

> On Sep 26, 2016, at 12:31 PM, Steve Drach  wrote:
> 
> Hi,
> 
> Please review these changes to the jar tool to fix a capability regression I 
> introduced in an earlier revision.  The issue is that this
> 
> $ jar -cf test.jar -C test1 . -C test2 .
> 
> only puts the files under test1 in the jar and ignores the files under test2. 
>  The DoubleCs test verified the problem and the solution.
> 
> issue: https://bugs.openjdk.java.net/browse/JDK-8165944 
> <https://bugs.openjdk.java.net/browse/JDK-8165944>
> webrev: http://cr.openjdk.java.net/~sdrach/8165944/webrev.00/ 
> <http://cr.openjdk.java.net/~sdrach/8165944/webrev.00/>
> 
> Thanks,
> Steve
> 

RFR: 8165944 jar utility doesn't process more than one -C argument

[Steve Drach]

Hi,

Please review these changes to the jar tool to fix a capability regression I 
introduced in an earlier revision.  The issue is that this

$ jar -cf test.jar -C test1 . -C test2 .

only puts the files under test1 in the jar and ignores the files under test2.  
The DoubleCs test verified the problem and the solution.

issue: https://bugs.openjdk.java.net/browse/JDK-8165944 

webrev: http://cr.openjdk.java.net/~sdrach/8165944/webrev.00/ 


Thanks,
Steve

Re: RFR: 8153654: Update jdeps to be multi-release jar aware

[Steve Drach]

> This looks good.  Thanks for the update.
> 
> Minor comments below and you can make the change before you push (no need for 
> a new webrev).
> 
> MultiReleaseException.java 
>key and msg should be final fields

Done.

> 
> VersionHelper.java
>   nameToVersion can simply be Map (I missed this last round)

It should be , see line 43 of VersionHelper.

> 
>   63 public static void add(JarFile jarfile, JarEntry e, ClassFile cf) 
> throws ConstantPoolException {
> 
> - can you break “throws …” to the next line.

Done

> 
>   56 String name = cf.getName().replace('/', '.');
>   57 nameToVersion.put(name, version);
> 
> Can you add a check to make sure the version is the same if the entry is 
> present; otherwise, throw InternalError.  This will catch any unexpected code 
> path.

That’s a good idea, but is InternalError the right one?  The spec is a bit 
ambiguous but implies to me that it’s a JVM error since it’s a subclass of 
VirtualMachineError.  How about just using the MultiReleaseException?

> 
> Mandy

Re: RFR: 8153654: Update jdeps to be multi-release jar aware

[Steve Drach]

A relatively minor update.  I simplified VersionHelper.  No other changes.

http://cr.openjdk.java.net/~sdrach/8153654/webrev.12/ 
<http://cr.openjdk.java.net/~sdrach/8153654/webrev.12/>


> On Sep 14, 2016, at 4:06 PM, Steve Drach  wrote:
> 
> The most recent webrev is 
> http://cr.openjdk.java.net/~sdrach/8153654/webrev.11/ 
> <http://cr.openjdk.java.net/~sdrach/8153654/webrev.11/>
> 
> Comments inline
> 
>>> The latest web revs.  Answers to questions in-line below:
>>> 
>>> http://cr.openjdk.java.net/~sdrach/8153654/webrev.10/
>>> 
>>> http://cr.openjdk.java.net/~sdrach/8153654/webrev.jdk/
>> 
>> This looks pretty good. My main comment is in VersionHelper (see below).
>> 
>> ClassFileReader.java
>> 
>> 337 String[] msg = {"err.multirelease.option.exists", 
>> f.getName()};
>> 389 String[] msg = 
>> {"err.multirelease.jar.malformed”,
>> 390 jarfile.getName(), rn
>> 391 };
>> 
>> `msg` is not used.  These lines can be removed.
> 
> Done
> 
>> 
>> line 81-95: this wants to add to VersionHelper if it’s a versioned entry.  I 
>> suggest to move this to VersionHelper, something like this and replace the 
>> add method.
> 
> I did essentially the same thing, but not exactly the same.
> 
>> 
>> public static void addIfVersionedEntry(Path jarfile, String realEntryName, 
>> String className) {
>>   if (realEntryName.startsWith(META_INF_VERSIONS)) {
>>   int len = META_INF_VERSIONS.length();
>>   int n = realEntryName.indexOf('/', len);
>>   if (n > 0) {
>>   String version = realEntryName.substring(len, n);
>>   assert (Integer.parseInt(version) > 8);
>> 
>>   String v = versionMap.get(className);
>>   if (v != null && !v.equals(version)) {
>>   // name associated with a different ClassFile
>>   throw new 
>> MultiReleaseException("err.multirelease.version.associated", className,
>>   v, version);
>>   }
>>   versionMap.put(className, version);
>>   } else {
>>   throw new MultiReleaseException("err.multirelease.jar.malformed",
>>   jarfile.toString(), realEntryName);
>>   }
>>   }
>> }
>> 
>> I prefer to call String::length rather than hardcoding the length.
> 
> OK
> 
>> I don’t see why VersionHelper caches ClassFile.  
> 
> A JarEntry (base) name has a one to many relationship with versions.  This 
> implies the class name also has a one to many relationship with versions.  
> But a ClassFile (derived from the contents of the JarEntry) has a one to one 
> relationship with version.  We desire a one to one
> relationship for className to version and one way to assure that is to create 
> two maps as I now have done.  I think the code is more obvious  now, at least 
> I hope it is.
> 
>> I think it can cache a map from class name to the version for now.  We may 
>> have to handle duplicated classes in more than one modular jar (it’s not 
>> exported and should be allowed).  We could file a separate issue to look 
>> into later.
>> 
>> This needs a CCC for the new --multi-release option.
> 
> That’s next.
> 
>> 
>> Mandy
> 

Re: RFR: 8153654: Update jdeps to be multi-release jar aware

[Steve Drach]

The most recent webrev is http://cr.openjdk.java.net/~sdrach/8153654/webrev.11/ 


Comments inline

>> The latest web revs.  Answers to questions in-line below:
>> 
>> http://cr.openjdk.java.net/~sdrach/8153654/webrev.10/
>> 
>> http://cr.openjdk.java.net/~sdrach/8153654/webrev.jdk/
> 
> This looks pretty good. My main comment is in VersionHelper (see below).
> 
> ClassFileReader.java
> 
> 337 String[] msg = {"err.multirelease.option.exists", 
> f.getName()};
> 389 String[] msg = 
> {"err.multirelease.jar.malformed”,
> 390 jarfile.getName(), rn
> 391 };
> 
> `msg` is not used.  These lines can be removed.

Done

> 
> line 81-95: this wants to add to VersionHelper if it’s a versioned entry.  I 
> suggest to move this to VersionHelper, something like this and replace the 
> add method.

I did essentially the same thing, but not exactly the same.

> 
> public static void addIfVersionedEntry(Path jarfile, String realEntryName, 
> String className) {
>if (realEntryName.startsWith(META_INF_VERSIONS)) {
>int len = META_INF_VERSIONS.length();
>int n = realEntryName.indexOf('/', len);
>if (n > 0) {
>String version = realEntryName.substring(len, n);
>assert (Integer.parseInt(version) > 8);
> 
>String v = versionMap.get(className);
>if (v != null && !v.equals(version)) {
>// name associated with a different ClassFile
>throw new 
> MultiReleaseException("err.multirelease.version.associated", className,
>v, version);
>}
>versionMap.put(className, version);
>} else {
>throw new MultiReleaseException("err.multirelease.jar.malformed",
>jarfile.toString(), realEntryName);
>}
>}
> }
> 
> I prefer to call String::length rather than hardcoding the length.

OK

>  I don’t see why VersionHelper caches ClassFile.  

A JarEntry (base) name has a one to many relationship with versions.  This 
implies the class name also has a one to many relationship with versions.  But 
a ClassFile (derived from the contents of the JarEntry) has a one to one 
relationship with version.  We desire a one to one
relationship for className to version and one way to assure that is to create 
two maps as I now have done.  I think the code is more obvious  now, at least I 
hope it is.

> I think it can cache a map from class name to the version for now.  We may 
> have to handle duplicated classes in more than one modular jar (it’s not 
> exported and should be allowed).  We could file a separate issue to look into 
> later.
> 
> This needs a CCC for the new --multi-release option.

That’s next.

> 
> Mandy

Re: RFR: 8153654: Update jdeps to be multi-release jar aware

[Steve Drach]

The latest web revs.  Answers to questions in-line below:

http://cr.openjdk.java.net/~sdrach/8153654/webrev.10/ 


http://cr.openjdk.java.net/~sdrach/8153654/webrev.jdk/ 


>>> 
>>> This looks quite good.  JDK-8163798 and JDK-8164665 will define public APIs 
>>> to get the versioned entries and real name which I think are useful for 
>>> tools.  It’s fine to proceed with this change and update jdeps to use the 
>>> public APIs when available.
>> 
>> The product team has decided not to move forward on those two issues for 
>> now, they will remain unresolved.
> 
> OK.  What about putting these helper methods in jdk.internal.jar package as 
> jlink and jdeps both use them to would avoid duplicated code.

VersionedStream is now in jdk.internal.util.jar

>  
> 
>>> 
>>> This patch parse MR-JAR only if —-multi-release option is specified. It 
>>> would also be useful to analyze all versioned entries for the default 
>>> option (i.e. analyze direct dependencies from class files) that can be done 
>>> in a separate RFE.
>>> 
>>> Comments to this patch:
>>> 
>>> ClassFileReader.java
>>> 
>>> 386 if (Integer.parseInt(v) < 9) {
>>> 387 String[] msg = 
>>> {"err.multirelease.jar.malformed",
>>> 388 jarfile.getName(), rn
>>> 389 };
>>> 390 throw new MultiReleaseException(msg);
>>> 391 }
>>> 
>>> To get here, I can think of the case when it’s not a MRJAR (so 
>>> JarFile::stream returns all entries).
>> 
>> fixed 
>> 
>>> If it’s a MR-JAR, the JarFile will be opened with a valid version.  
>>> versionedStream should only return the proper versioned entries.  Maybe you 
>>> should emit warning (add it to skippedEntries if this happens) or make it 
>>> an assert.
>> 
>> I’m not sure what you mean.
> 
> My point is that JarFile::getEntry should not return such JarEntry - is that 
> true?
> 
> In that case, how SharedSecrets.javaUtilJarAccess().getRealName(jarfile, e) 
> would return META-INF/versions/$n  where n < 9?
> 
> This is not an expected error and so InternalError (or assert) is more 
> appropriate than throwing MultiReleaseException.

I put an assert in.

> 
>> 
>>> 
>>> Can you add the following scenario in the new test and Bar and Gee are 
>>> public and shows the result when -—multi-release 9 or 10 or base?
>>> p/Foo.class
>>> META-INF/versions/9/p/Foo.class
>>> META-INF/versions/9/q/Bar.class
>>> META-INF/versions/10/q/Bar.class
>>> META-INF/versions/10/q/Gee.class
>> 
>> One can not put new public classes in the versioned section, so that layout 
>> is not legal

If I make Bar and Gee non-public, I can build a jar with them in it.  See the 
second test I added in test.tools.jdep.MultireleaseJar

> 
> But such JAR file can be created.  What about adding a non-public class under:
>META-INF/versions/9/q/Zee.class
>META-INF/versions/10/q/Zee.class
> 
> Keeping public q/Bar.class is okay as JarFile::getName(“q/Bar.class”) should 
> return null if not allowed for any Runtime.Version.

jar tool won’t validate a q.Bar as a public class.  

> 
>> 
>>> 
>>> JDK-8163798 would cover more scenarios in depth.  I’m okay to use the 
>>> versionedStream you have and leave that to JDK-8163798.
>> 
>>> BTW the copyright header is missing in the new tests.
>> 
>> All the existing test input source files do not have the copyright header.  
>> These little classes are just there to feed the test that does have the 
>> required copyright
> 
> I put the copyright headers in all source files, including tiny test classes.

Done.

> 
>> 
>>> 
>>> JdepsTask.java
>>> 401 throw new 
>>> BadArgs("err.multirelease.option.illegal", arg);
>>> 
>>> You can simply use err.invalid.arg.for.option which I think is simple and 
>>> adequate.
>> 
>> That’s not an existing property, so I left it where it is with all the 
>> multi-release properties
> 
> err.invalid.arg.for.option is an existing property.

Okay.  Unfortunately we lose a better error message.

> 
>> 
>>> jdeps.properties: what about a shorter version:
>>> 
>>> --multi-release  Specifies the version when processing
>>>  multi-release jar files.  can be
 = 9 or base.
>> 
>> I did that but I think it’s more confusing
> 
> User guides, man page to include examples would help that.
> 
> Mandy
> 

Re: RFR: 8163798: Create a JarFile versionedStream method

[Steve Drach]

I guess I’m going to keep doing this until I get it right ;-)  Here’s another 
webrev that doesn’t use an exception for a common case, and addresses most of 
Mandy concerns.  

http://cr.openjdk.java.net/~sdrach/8163798/webrev.06/ 


Comments in-line

> This version looks good.
> 
> Can you add the javadoc to describe what the stream method returns (a union 
> of the base entries + all versioned entries <= JarFile::getVersion.

Done

> 
> Nit:  you may want to call jf.getVersion().major() once rather than the 
> number of entries.

Done

> 
> Now that you have jdk.internal.util.jar internal API for MRJAR.  It may be 
> useful to add a static getRealName(JarFile jar, JarEntry entry) method for 
> convenience such that jdeps and jlink can use this MRJAR-specific internal 
> API and no need to use the shared secret. Maybe rename VersionedStream to 
> VersionedJarFileHelper?
> 
> TestVersionedStream::close
>   You may want to use jdk.testlibrary.FileUtils and deleteFileTreeWithRetry 
> may be what you want.

I can’t use it because it removes the top-level directory (i.e. JTwork/scratch).

> 
> Mandy

Re: RFR: 8163798: Create a JarFile versionedStream method

[Steve Drach]

Here’s a new webrev addressing Paul’s additional concerns

http://cr.openjdk.java.net/~sdrach/8163798/webrev.04/ 



> Ok, better. Now there is no need to create an instance of VersionedStream, 
> all methods can be static.

Done

> 
> I wonder if you can combine the checks for the index:
> 
>   64 int index = name.indexOf('/', META_INF_VERSIONS_LEN);
>   65 if (index == -1) {

Removed that test, let Integer.parseint handle it

> 
>   77 if (++index < name.length()) {
> 
> e.g. no slash, or found at end (not just malformed since we want to skip 
> version directory entries if present?)
> 
>   if (index == -1 || index == name.length() - 1)
>
> 
>>> 
>>> Also can getJarEntry ever return null?
>> 
>> yes, that’s why it’s filtered out
>> 
> 
> Under what circumstances in this case can it return null? since you have 
> iterated over the existing entries and reduced to a distinct subset.

Right, that was before I put the “fix” in JarFile at line 561.  I removed the 
null check

> 
> Paul.
> 
>>> 
>>> Claes makes a good point regarding performance. I would suggest getting 
>>> this functional and tested then tweaking for performance.
>>> 
>>> Paul.
> 

Re: RFR: 8163798: Create a JarFile versionedStream method

[Steve Drach]

Here’s a new webrev that addresses Claes’ and Paul’s concerns

http://cr.openjdk.java.net/~sdrach/8163798/webrev.03/ 
<http://cr.openjdk.java.net/~sdrach/8163798/webrev.03/>

> On Sep 11, 2016, at 1:12 PM, Steve Drach  wrote:
> 
> I made a simple change, the new webrev is 
> http://cr.openjdk.java.net/~sdrach/8163798/webrev.02/ 
> <http://cr.openjdk.java.net/~sdrach/8163798/webrev.02/>
> 
>> On Sep 9, 2016, at 4:02 PM, Steve Drach  wrote:
>> 
>> Hi,
>> 
>> Please review this changeset that adds a VersionedStream class to the 
>> jdk.internal.util.jar package.  Some may recall that I submitted a similar 
>> RFR a few weeks ago; this is a redesign from that one.  We decided not to 
>> make a public JarFile::versionedStream method at this time.  Once we get 
>> sufficient experience with this and find a few more use cases, we will 
>> revisit the idea of making this a public method in JarFile.
>> 
>> issue: https://bugs.openjdk.java.net/browse/JDK-8163798 
>> <https://bugs.openjdk.java.net/browse/JDK-8163798>
>> webrev: http://cr.openjdk.java.net/~sdrach/8163798/webrev.01/index.html 
>> <http://cr.openjdk.java.net/~sdrach/8163798/webrev.01/index.html>
>> 
>> Thanks,
>> Steve
> 

Re: RFR: 8163798: Create a JarFile versionedStream method

[Steve Drach]

>> I made a simple change, the new webrev is 
>> http://cr.openjdk.java.net/~sdrach/8163798/webrev.02/ 
>> <http://cr.openjdk.java.net/~sdrach/8163798/webrev.02/>
>> 
> 
> I don’t like the state interplay between allowedVersions and getBaseSuffix, 
> and the filtering for null. Consider merging filter.map.filter into a single 
> flatMap.

Moved it into a map as Claes suggested

> 
> Also can getJarEntry ever return null?

yes, that’s why it’s filtered out

> 
> Claes makes a good point regarding performance. I would suggest getting this 
> functional and tested then tweaking for performance.
> 
> Paul.
> 
> 
> 
>>> On Sep 9, 2016, at 4:02 PM, Steve Drach  wrote:
>>> 
>>> Hi,
>>> 
>>> Please review this changeset that adds a VersionedStream class to the 
>>> jdk.internal.util.jar package.  Some may recall that I submitted a similar 
>>> RFR a few weeks ago; this is a redesign from that one.  We decided not to 
>>> make a public JarFile::versionedStream method at this time.  Once we get 
>>> sufficient experience with this and find a few more use cases, we will 
>>> revisit the idea of making this a public method in JarFile.
>>> 
>>> issue: https://bugs.openjdk.java.net/browse/JDK-8163798 
>>> <https://bugs.openjdk.java.net/browse/JDK-8163798>
>>> webrev: http://cr.openjdk.java.net/~sdrach/8163798/webrev.01/index.html 
>>> <http://cr.openjdk.java.net/~sdrach/8163798/webrev.01/index.html>
>>> 
>>> Thanks,
>>> Steve
>> 
> 

Re: RFR: 8163798: Create a JarFile versionedStream method

[Steve Drach]

> jdk/internal/util/jar/VersionedStream.java:
> 
> - use Integer.parseInt(name, vptr, ptr, 10) to avoid creating vstr on
>  every entry

done

> 
> - folding allowedVersions into getBaseSuffix seems easier than to keep
>  global state (sptr) and splitting the logic over a filter and a map
>  operation

done

> 
> - distinct() could be used instead of explicitly collecting to a

I did use distinct in webrev.02.  I left it in webrev.03 (coming soon).

>  LinkedHashSet, but this does make me wonder if we could do even
>  better by creating a map from base name to the appropriate versioned
>  JarEntry and return a stream over that map to avoid looking things up
>  via jf::getJarEntry.

I look things up with getJarEntry to get the aliased entry.  Otherwise we get 
the real name and we want to refer to entries by the base name.

> Could be quite a bit more efficient, and as we
>  have to create a set or do distinct the overheads should be roughly
>  the same either way
> 
> - declaring META_INF_VERSIONS_LEN seems like a premature optimization

Left it in, it’s harmless and doesn’t need to be computed every time

> 
> - nit: static final is preferred over final static

done

> 
> - nit: rename *ptr to *Index

done

> 
> Test and JarFile changes seem fine to me.
> 
> Thanks!
> 
> /Claes
> 
> On 2016-09-11 22:12, Steve Drach wrote:
>> I made a simple change, the new webrev is 
>> http://cr.openjdk.java.net/~sdrach/8163798/webrev.02/ 
>> <http://cr.openjdk.java.net/~sdrach/8163798/webrev.02/>
>> 
>>> On Sep 9, 2016, at 4:02 PM, Steve Drach  wrote:
>>> 
>>> Hi,
>>> 
>>> Please review this changeset that adds a VersionedStream class to the 
>>> jdk.internal.util.jar package.  Some may recall that I submitted a similar 
>>> RFR a few weeks ago; this is a redesign from that one.  We decided not to 
>>> make a public JarFile::versionedStream method at this time.  Once we get 
>>> sufficient experience with this and find a few more use cases, we will 
>>> revisit the idea of making this a public method in JarFile.
>>> 
>>> issue: https://bugs.openjdk.java.net/browse/JDK-8163798 
>>> <https://bugs.openjdk.java.net/browse/JDK-8163798>
>>> webrev: http://cr.openjdk.java.net/~sdrach/8163798/webrev.01/index.html 
>>> <http://cr.openjdk.java.net/~sdrach/8163798/webrev.01/index.html>
>>> 
>>> Thanks,
>>> Steve
>> 

Re: RFR: 8163798: Create a JarFile versionedStream method

[Steve Drach]

I made a simple change, the new webrev is 
http://cr.openjdk.java.net/~sdrach/8163798/webrev.02/ 
<http://cr.openjdk.java.net/~sdrach/8163798/webrev.02/>

> On Sep 9, 2016, at 4:02 PM, Steve Drach  wrote:
> 
> Hi,
> 
> Please review this changeset that adds a VersionedStream class to the 
> jdk.internal.util.jar package.  Some may recall that I submitted a similar 
> RFR a few weeks ago; this is a redesign from that one.  We decided not to 
> make a public JarFile::versionedStream method at this time.  Once we get 
> sufficient experience with this and find a few more use cases, we will 
> revisit the idea of making this a public method in JarFile.
> 
> issue: https://bugs.openjdk.java.net/browse/JDK-8163798 
> <https://bugs.openjdk.java.net/browse/JDK-8163798>
> webrev: http://cr.openjdk.java.net/~sdrach/8163798/webrev.01/index.html 
> <http://cr.openjdk.java.net/~sdrach/8163798/webrev.01/index.html>
> 
> Thanks,
> Steve

RFR: 8163798: Create a JarFile versionedStream method

[Steve Drach]

Hi,

Please review this changeset that adds a VersionedStream class to the 
jdk.internal.util.jar package.  Some may recall that I submitted a similar RFR 
a few weeks ago; this is a redesign from that one.  We decided not to make a 
public JarFile::versionedStream method at this time.  Once we get sufficient 
experience with this and find a few more use cases, we will revisit the idea of 
making this a public method in JarFile.

issue: https://bugs.openjdk.java.net/browse/JDK-8163798 

webrev: http://cr.openjdk.java.net/~sdrach/8163798/webrev.01/index.html 


Thanks,
Steve

Re: RFR: 8153654: Update jdeps to be multi-release jar aware

[Steve Drach]

new webrev addressing issues below: 
http://cr.openjdk.java.net/~sdrach/8153654/webrev.09/ 



>> Hi,
>> 
>> Please review the following two changesets that enables jdeps to use 
>> multi-release jar files.  The output from the tool shows versioned 
>> dependencies by prefixing them with “version #/“ where version # is 9, 10, 
>> etc.
>> 
>> webrevs: http://cr.openjdk.java.net/~sdrach/8153654/webrev.08/ 
> 
> This looks quite good.  JDK-8163798 and JDK-8164665 will define public APIs 
> to get the versioned entries and real name which I think are useful for 
> tools.  It’s fine to proceed with this change and update jdeps to use the 
> public APIs when available.

The product team has decided not to move forward on those two issues for now, 
they will remain unresolved.

> 
> This patch parse MR-JAR only if —-multi-release option is specified. It would 
> also be useful to analyze all versioned entries for the default option (i.e. 
> analyze direct dependencies from class files) that can be done in a separate 
> RFE.
> 
> Comments to this patch:
> 
> ClassFileReader.java
> 
> 386 if (Integer.parseInt(v) < 9) {
> 387 String[] msg = 
> {"err.multirelease.jar.malformed",
> 388 jarfile.getName(), rn
> 389 };
> 390 throw new MultiReleaseException(msg);
> 391 }
> 
> To get here, I can think of the case when it’s not a MRJAR (so 
> JarFile::stream returns all entries).

fixed 

>  If it’s a MR-JAR, the JarFile will be opened with a valid version.  
> versionedStream should only return the proper versioned entries.  Maybe you 
> should emit warning (add it to skippedEntries if this happens) or make it an 
> assert.

I’m not sure what you mean.

> 
> 382 if (rn.startsWith("META-INF/versions/")) {
> 383 int n = rn.indexOf('/', 18); // 
> "META-INF/versions/".length()
> 
> 421 final String META_INF_VERSIONS = "META-INF/versions/“;
> 429 if (name.length() > META_INF_VERSIONS.length()) {
> 
> They both extract the version of this entry.  This can be refactored to add a 
> method like VersionHelper::getVersion(JarEntry).

done

> 
> versionedStream method may be better to be moved to VersionHelper.

done

> 
> 442return (version == jf.getVersion().major() && vStr.length() > offset + 
> 1)
> 
> This version only selects the entries in the base section and versioned 
> section.

I thought that would work, at least I had a scenario in my head where it worked 
;-)  But it doesn’t so I changed it.

> 
> Can you add the following scenario in the new test and Bar and Gee are public 
> and shows the result when -—multi-release 9 or 10 or base?
>   p/Foo.class
>   META-INF/versions/9/p/Foo.class
>   META-INF/versions/9/q/Bar.class
>   META-INF/versions/10/q/Bar.class
>   META-INF/versions/10/q/Gee.class

One can not put new public classes in the versioned section, so that layout is 
not legal

> 
> JDK-8163798 would cover more scenarios in depth.  I’m okay to use the 
> versionedStream you have and leave that to JDK-8163798.

>  BTW the copyright header is missing in the new tests.

All the existing test input source files do not have the copyright header.  
These little classes are just there to feed the test that does have the 
required copyright

> 
> JdepsTask.java
> 401 throw new 
> BadArgs("err.multirelease.option.illegal", arg);
> 
> You can simply use err.invalid.arg.for.option which I think is simple and 
> adequate.

That’s not an existing property, so I left it where it is with all the 
multi-release properties

> 
> MultiReleaseException.java
>  47 public MultiReleaseException(String[] err) {
> 
> It would be better to have a key parameter: MultiReleaseException(String key, 
> String… param)

done

> VersionHelper.java
>  40 public static String get(String origin) {
> 
> s/origin/classname to make the param clearer.

done

> jdeps.properties: what about a shorter version:
> 
>  --multi-release  Specifies the version when processing
>multi-release jar files.  can be
>> = 9 or base.

I did that but I think it’s more confusing

> 
>> http://cr.openjdk.java.net/~sdrach/8153654/webrev.jdk/ 
> 
> 
> The use of shared secrets is just temporary and I hope it will soon be 
> replaced with a public API when JDK-8164665 is resolved.

See my earlier comment

> 
> Mandy

RFR: 8153654: Update jdeps to be multi-release jar aware

[Steve Drach]

Hi,

Please review the following two changesets that enables jdeps to use 
multi-release jar files.  The output from the tool shows versioned dependencies 
by prefixing them with “version #/“ where version # is 9, 10, etc.

webrevs: http://cr.openjdk.java.net/~sdrach/8153654/webrev.08/ 

   http://cr.openjdk.java.net/~sdrach/8153654/webrev.jdk/ 


issue: https://bugs.openjdk.java.net/browse/JDK-8153654 


Thanks
Steve

Fwd: RFR 8163798: Add a versionedStream method to JarFile

[Steve Drach]

Sorry, forget to Cc core-libs on my response to Peter’s questions

> Begin forwarded message:
> 
> From: Steve Drach 
> Subject: Re: RFR 8163798: Add a versionedStream method to JarFile
> Date: August 26, 2016 at 9:44:39 AM PDT
> To: Peter Levart 
> 
> Hi Peter,
> 
>> Javadoc for the new method mentions public and non-public classes:
>> 
>> "@{code JarEntries}
>>* containing non-public classes in the
>>* versioned directory that corresponds to the version returned by
>>* {@link JarFile#getVersion()} are included in the stream because they are
>>* typically accessed by the public classes."
>> 
>> Why?
> 
> The public “interface” for a multi-release jar is the set of public or 
> protected classes in the base (or root) section of the jar.  By definition, 
> no new (i.e. without a corresponding base class) public or protected classes 
> can be contained in a versioned directory.  Versioned directories can contain 
> new package-private classes.  In practice these package-private classes are 
> dependencies for the public/protected classes in
> in the same versioned directory.
> 
>> The logic of multi-release jar files is indifferent towards the access 
>> attributes of class files. It never parses the class file to interpret the 
>> access attributes of the containing class.
> 
> I know.  I thought about doing that but rejected it as too heavyweight and 
> just decided to use the heuristic that real class files have the suffix 
> “.class” and that if there was not a corresponding base class, then the class 
> has to be package-private according to the “rules” of multi-release jar 
> files.  The jar tool enforces these rules by actually parsing the class files.
> 
>> The class-level javadoc of JarFile does mention "public classes", but I view 
>> this just as a recommendation how they should be constructed.
>> 
>> The logic of multi-release jar files is indifferent towards the types of 
>> resources contained in the jar entries too, as far as I can see. Your logic 
>> in versionedStream() is not. The proposed new method is the only place in 
>> JarFile and ZipFile where the check for ".class" suffix of the entry name is 
>> performed. Why this special treatement of class files?
> 
> I hope the explanation above is sufficient.  The versionedStream looks at the 
> jar file in an inverted or backwards way.  In the usual case, a
> multi-release jar file entry is obtained by 
> JarFile.getJarEntry(base-entry-name).  In this case if the jar file was 
> opened for version 10, we’d not be able to see or get package-private classes 
> in the version 9 directory.  Oh, one thing I should mention, Mr. Jar’s can be 
> sparsely populated in the versioned directories, So if a version 10 public 
> class was not in the version 10 directory, but was in the version 9 
> directory, then the version 9 one would be returned.  
> 
>> I also don't understand why the versioned stream should contain the 
>> untranslated base versioned directories. For example, the following entries 
>> for the v10 stream in the test:
>> 
>> "META-INF/versions/9/"
>> "META-INF/versions/10/“
> 
> I just did it because they are returned by JarFile::stream.  I’m ambivalent 
> about it, so I wanted to see the comments if any.
> 
>> 
>> All other resources and sub-directories in the META-INF/versions/XX/... 
>> directories are "normalized" (meaning that the versioned prefix is stripped 
>> from the names in the returned entries). Normalizing above directories would 
>> translate them to "root" directory, which is never included as an entry of a 
>> normal jar or zip file. So I think they could simply be skipped in the 
>> resulting versioned stream. Couldn't they?
> 
> Yes.
> 
>> 
>> Considering all of the above, I think versionedStream() method could be much 
>> simpler. For example, something like the following:
>> 
>> 
>>   public Stream versionedStream() {
>>   return
>>   !isMultiRelease()
>>   ? stream()
>>   : stream()
>>   .flatMap(je -> {
>>   String name = je.getName();
>>   if (name.startsWith(META_INF_VERSIONS)) {
>>   // versioned entry
>>   if (name.length() > META_INF_VERSIONS.length()) {
>>   // potentially real entry
>>   String vStr = 
>> name.substring(META_INF_VERSIONS.length());
>>   int offset = vStr.indexOf(&#

RFR 8163798: Add a versionedStream method to JarFile

[Steve Drach]

Hi,

Please review this changeset that adds a versionedStream method to JarFile.

webrev: http://cr.openjdk.java.net/~sdrach/8163798/webrev.00/ 

issue: https://bugs.openjdk.java.net/browse/JDK-8163798 


Thanks
Steve

8164585: JarFile::isMultiRelease does not return true in all cases where it should return true

[Steve Drach]

Hi,

Please review this simple change to JarFile

issue: https://bugs.openjdk.java.net/browse/JDK-8164585 

webrev: http://cr.openjdk.java.net/~sdrach/8164585/webrev.00/ 


Thanks,
Steve

Re: RFR: 8164389: jdk.nio.zipfs.JarFileSystem does not completely traverse the versioned entries in a multi-release jar file

[Steve Drach]

I’ve updated the webrev to incorporate most of Paul’s suggestions.

http://cr.openjdk.java.net/~sdrach/8164389/webrev.01/index.html 
<http://cr.openjdk.java.net/~sdrach/8164389/webrev.01/index.html>


> On Aug 18, 2016, at 5:01 PM, Paul Sandoz  wrote:
> 
> 
>> On 18 Aug 2016, at 16:36, Steve Drach  wrote:
>> 
>> Hi,
>> 
>> Please review this rather simple fix to JarFileSystem.
>> 
>> issue: https://bugs.openjdk.java.net/browse/JDK-8164389 
>> <https://bugs.openjdk.java.net/browse/JDK-8164389>
>> webrev: http://cr.openjdk.java.net/~sdrach/8164389/webrev/index.html 
>> <http://cr.openjdk.java.net/~sdrach/8164389/webrev/index.html>
>> 
> 
> JarFileSystem
> —
> 
> 162 private void walk(IndexNode inode, Consumer process) {
> 163 if (inode == null) return;
> 164 if (inode.isDir())
> 165 walk(inode.child, process);
> 166 else
> 167 process.accept(inode);
> 168 walk(inode.sibling, process);
> 169 }
> 
> I would recommend using braces, even for single statement blocks.
> 
> 
> JFSTester
> —
> 
> You might wanna create a temporary jar file if possible, just in case the 
> test somehow fails to clean things up.
> 
> 
>  80 public void TestWalk() throws IOException {
> 
> Lower case for first letter.
> 
> Paul.

Re: RFR: 8164389: jdk.nio.zipfs.JarFileSystem does not completely traverse the versioned entries in a multi-release jar file

[Steve Drach]

>>> You might wanna create a temporary jar file if possible, just in case the 
>>> test somehow fails to clean things up.
>> 
>> Unsure how to do that, or perhaps I don’t understand your request.
> 
> I believe it’s possible to create a temporary directory, see 
> Files.createTempDirectory, and use that to place the test.jar, although it 
> might be more awkward to clean up.
> 
> Actually maybe it’s ok with regards to test execution if you can confirm the 
> “user.dir” is set up correctly with jtreg, i cannot recall.

It’s usually JTwork/scratch.  A lot of tests use user.dir.

Re: RFR: 8164389: jdk.nio.zipfs.JarFileSystem does not completely traverse the versioned entries in a multi-release jar file

[Steve Drach]

> You might wanna create a temporary jar file if possible, just in case the 
> test somehow fails to clean things up.

Unsure how to do that, or perhaps I don’t understand your request.

RFR: 8164389: jdk.nio.zipfs.JarFileSystem does not completely traverse the versioned entries in a multi-release jar file

[Steve Drach]

Hi,

Please review this rather simple fix to JarFileSystem.

issue: https://bugs.openjdk.java.net/browse/JDK-8164389 

webrev: http://cr.openjdk.java.net/~sdrach/8164389/webrev/index.html 


Thanks,
Steve

RFR: 8156499 Update jlink to support creating images with modules that are packaged as multi-release JARs

[Steve Drach]

Hi,

Please review this changset that makes jlink multi-release jar aware.

issue: https://bugs.openjdk.java.net/browse/JDK-8156499 

webrev: http://cr.openjdk.java.net/~sdrach/8156499/webrev.00/index.html 


Thanks
Steve

Re: RFR: 8158295 Add a multi-release jar validation mechanism to jar tool

[Steve Drach]

I’ve updated the webrev to incorporate the changes suggested by Oleg Barbashov. 
 This changeset has a couple of minor changes to the tests and I’ve changed the 
Validator class to be an instance of Consumer.

http://cr.openjdk.java.net/~sdrach/8158295/webrev.03/index.html 
<http://cr.openjdk.java.net/~sdrach/8158295/webrev.03/index.html>


> On Jul 18, 2016, at 6:33 PM, Steve Drach  wrote:
> 
>>> Please review the following:
>>> 
>>> webrev: http://cr.openjdk.java.net/~sdrach/8158295/webrev.01/ 
>>> <http://cr.openjdk.java.net/~sdrach/8158295/webrev.01/>
>>> issue: https://bugs.openjdk.java.net/browse/JDK-8158295 
>>> <https://bugs.openjdk.java.net/browse/JDK-8158295>
>>> 
>>> This changeset adds a multi-release jar validator to jar tool.  After the 
>>> jar tool builds a multi-release jar, the potential resultant jar file is 
>>> passed to the validator to assure that the jar file meets the minimal 
>>> standards of a multi-release jar, in particular that versioned classes have 
>>> the same api as base classes they override.  There are other checks, for 
>>> example warning if two classes are identical.  If the jar file is invalid, 
>>> it is not kept, so —create will not produce a jar file and —update will 
>>> keep the input jar file.
> 
> I’ve updated the webrev to address the issues raised — 
> http://cr.openjdk.java.net/~sdrach/8158295/webrev.02/index.html 
> <http://cr.openjdk.java.net/~sdrach/8158295/webrev.02/index.html>
> 
>> 
>> jar/Main.java
>> 
>> 284 // This code would be repeated in the create and update 
>> sections.
>> 285 // In order not to do that, it's here as a consumer.
>> 286 Consumer validateAndClose = tmpfile -> {
>> Why does it need to be Consumer rather than just a method?
> 
> I changed it to a method.
> 
>> 
>> Then i think you don’t need to rethow, but you can still keep the try block 
>> and use finally for File.deleteIfExist since it will not complain for the 
>> case where you move.
> 
> Done.
> 
>> 
>> 
>> 558 int i1 = s1.indexOf('/', n);
>> 559 int i2 = s1.indexOf('/', n);
>> 560 if (i1 == -1) throw new NumberFormatException(s1);
>> 561 if (i2 == -2) throw new NumberFormatException(s2);
>> 
>> I think you are trying to reject entries directly under the versioned area 
>> so it’s not about numbers (same in the validator, so there is some 
>> redundancy?).
> 
> A couple things here.  The most obvious is it should be “if (i2 == -1)”.  I 
> replaced NumberFormatException with a new InvalidJarException.  I added a 
> comment that
> this code is used to sort the version numbers as string representations of 
> numbers, therefore “9” goes before “10”, not usual for string sorts.
> 
>> 
>> 
>> 588 AtomicBoolean validHolder = new AtomicBoolean();
>> 589 validHolder.set(valid);
>> 
>> Pass valid to the constructor
> 
> Done.
> 
>> 
>> 
>> Validator/FingerPrint.java
>> 
>> It would be useful to have some comments on what is checked in terms of API 
>> compatibility. e.g. describe what FingerPrint collects and how Validator 
>> uses it.
> 
> Added some commentary to FingerPrint.
> 
>> 
>> 
>> FingerPrint.java
>> 
>> 205 private final Map fields = new HashMap<>();
>> 206 private final Map methods = new HashMap<>();
>> 
>> Not sure you need to use a Map, why not use a Set?
> 
> Not sure why I did it with Maps instead of Sets, but I wanted to keep the 
> method and field names and maybe that made sense at one time.  It doesn’t 
> now, so Sets they are.
> 
>> 
>> Change Method to be literally a representation of a single method rather 
>> than a consolidation that is lossy for the set of exceptions.
> 
> Done.
> 
> 

Re: RFR: 8158295 Add a multi-release jar validation mechanism to jar tool

[Steve Drach]

> On Jul 28, 2016, at 11:59 AM, Oleg G. Barbashov  
> wrote:
> 
> 27.07.2016 15:34, Oleg G. Barbashov пишет:
>> 
>> 07.07.2016 23:32, Steve Drach пишет:
>>> Hi,
>>> 
>>> Please review the following:
>>> 
>>> webrev: http://cr.openjdk.java.net/~sdrach/8158295/webrev.01/ 
>>> <http://cr.openjdk.java.net/~sdrach/8158295/webrev.01/>
>>> issue: https://bugs.openjdk.java.net/browse/JDK-8158295 
>>> <https://bugs.openjdk.java.net/browse/JDK-8158295>
>>> 
>>> This changeset adds a multi-release jar validator to jar tool. After the 
>>> jar tool builds a multi-release jar, the potential resultant jar file is 
>>> passed to the validator to assure that the jar file meets the minimal 
>>> standards of a multi-release jar, in particular that versioned classes have 
>>> the same api as base classes they override.  There are other checks, for 
>>> example warning if two classes are identical.  If the jar file is invalid, 
>>> it is not kept, so —create will not produce a jar file and —update will 
>>> keep the input jar file.
>>> 
>>> Thanks
>>> Steve
>> 
>> 574 private boolean validate(String fname) {
>> 575 boolean valid = true;
>> 576 Validator validator = new Validator(this);
>> 577
>> 578 try (JarFile jf = new JarFile(fname)) {
>> 579 AtomicBoolean validHolder = new AtomicBoolean(valid);
>> 580 jf.stream()
>> 581 .filter(e -> !e.isDirectory())
>> 582 .filter(e -> !e.getName().equals(MANIFEST_NAME))
>> 583 .filter(e -> !e.getName().endsWith(MODULE_INFO))
>> 584 .sorted(entryComparator)
>> 585 .forEachOrdered(je -> {
>> 586 boolean b = validator.accept(je, jf);
>> 587 if (validHolder.get()) validHolder.set(b);
>> 588 });
>> 589 valid = validHolder.get();
>> 590 } catch (IOException e) {
>> 591 error(formatMsg2("error.validator.jarfile.exception", fname, 
>> e.getMessage()));
>> 592 valid = false;
>> 593 } catch (InvalidJarException e) {
>> 594 error(formatMsg("error.validator.bad.entry.name", 
>> e.getMessage()));
>> 595 valid = false;
>> 596 }
>> 597 return valid;
>> 598 }
>> 
>> (IMHO) Using of AtomicBoolean and forEachOrdered() for stateful validator 
>> here looks forced. It may be avoided by using regular iterator with "for" 
>> loop:
>> 
>>Stream sorted = jf.stream()
>>.filter(e -> !e.isDirectory())
>>.filter(e -> !e.getName().equals(MANIFEST_NAME))
>>.filter(e -> !e.getName().endsWith(MODULE_INFO))
>>.sorted(entryComparator);
>>for (Iterator iter = sorted.iterator(); 
>> iter.hasNext();) {
>>if (!validator.accept(iter.next(), jf)) {
>>valid = false;
>>}
>>}
>> 
>> or even better by storing "valid" state inside the Validator after turning 
>> it to regular Consumer:
>> 
>>try (JarFile jf = new JarFile(fname)) {
>>Validator validator = new Validator(this, jf);
>>jf.stream()
>>.filter(e -> !e.isDirectory())
>>.filter(e -> !e.getName().equals(MANIFEST_NAME))
>>.filter(e -> !e.getName().endsWith(MODULE_INFO))
>>.sorted(entryComparator)
>>.forEachOrdered(validator);
>>return validator.isValidated();
>>} catch (IOException e) {
>>error(formatMsg2("error.validator.jarfile.exception", fname, 
>> e.getMessage()));
>>return false;
>>} catch (NumberFormatException e) {
>>error(formatMsg("error.validator.bad.entry.name", 
>> e.getMessage()));
>>return false;
>>}
>> 
>> Moreover what is the reason to have an external loop over the jar's entries? 
>> Even if we will use several different filter sets in future it would be 
>> better to use it as optional parameter for Validator.
>> 
> I mean that the jar file entries iteration code ("580 
> jf.stream().filter(e ")...588 }); ) 
> processing should be a part of "Validator”.

Yes, I know what you meant.  I think it’s fine where it is, but I do like your 
suggestion to make Validator a Consumer and I’ll be releasing an updated webrev 
soon, with that change and the changes you recommended for the tests.  Thanks 
for the ideas!

Re: RFR: 8158295 Add a multi-release jar validation mechanism to jar tool

[Steve Drach]

>>> I would also suggest to pass VM options to jar util in 
>>> test/tools/jar/multiRelease/Basic.java:
>>> 
>>> 540 commands.add(jar);
>>> +541 commands.addAll(Utils.getForwardVmOptions());
>> Done.
>> 
>>> Also it seems that there is no need to compile classes using separate 
>>> process:
>>> 
>>> 526 String javac = JDKToolFinder.getJDKTool("javac");
>>> 
>>> It is possible to use jdk/test/lib/testlibrary/CompilerUtils.java instead.
>> Yes, it does seem possible, and I’ll remember that in the future, but what I 
>> have works, so I think I’ll stick with it.
> 
> In this case it is also needed to pass the jtreg's javac options 
> ("test.compiler.opts”).

OK

> 
> - Oleg
>> 
>>> Thanks,
>>> Oleg
>>> 
>>> 07.07.2016 23:32, Steve Drach пишет:
>>>> Hi,
>>>> 
>>>> Please review the following:
>>>> 
>>>> webrev: http://cr.openjdk.java.net/~sdrach/8158295/webrev.01/ 
>>>> <http://cr.openjdk.java.net/~sdrach/8158295/webrev.01/>
>>>> issue: https://bugs.openjdk.java.net/browse/JDK-8158295 
>>>> <https://bugs.openjdk.java.net/browse/JDK-8158295>
>>>> 
>>>> This changeset adds a multi-release jar validator to jar tool.  After the 
>>>> jar tool builds a multi-release jar, the potential resultant jar file is 
>>>> passed to the validator to assure that the jar file meets the minimal 
>>>> standards of a multi-release jar, in particular that versioned classes 
>>>> have the same api as base classes they override.  There are other checks, 
>>>> for example warning if two classes are identical.  If the jar file is 
>>>> invalid, it is not kept, so —create will not produce a jar file and 
>>>> —update will keep the input jar file.
>>>> 
>>>> Thanks
>>>> Steve
>>> 
> 

Re: RFR: 8158295 Add a multi-release jar validation mechanism to jar tool

[Steve Drach]

> I would also suggest to pass VM options to jar util in 
> test/tools/jar/multiRelease/Basic.java:
> 
> 540 commands.add(jar);
> +541 commands.addAll(Utils.getForwardVmOptions());

Done.

> 
> Also it seems that there is no need to compile classes using separate process:
> 
> 526 String javac = JDKToolFinder.getJDKTool("javac");
> 
> It is possible to use jdk/test/lib/testlibrary/CompilerUtils.java instead.

Yes, it does seem possible, and I’ll remember that in the future, but what I 
have works, so I think I’ll stick with it.

> 
> Thanks,
> Oleg
> 
> 07.07.2016 23:32, Steve Drach пишет:
>> Hi,
>> 
>> Please review the following:
>> 
>> webrev: http://cr.openjdk.java.net/~sdrach/8158295/webrev.01/ 
>> <http://cr.openjdk.java.net/~sdrach/8158295/webrev.01/>
>> issue: https://bugs.openjdk.java.net/browse/JDK-8158295 
>> <https://bugs.openjdk.java.net/browse/JDK-8158295>
>> 
>> This changeset adds a multi-release jar validator to jar tool.  After the 
>> jar tool builds a multi-release jar, the potential resultant jar file is 
>> passed to the validator to assure that the jar file meets the minimal 
>> standards of a multi-release jar, in particular that versioned classes have 
>> the same api as base classes they override.  There are other checks, for 
>> example warning if two classes are identical.  If the jar file is invalid, 
>> it is not kept, so —create will not produce a jar file and —update will keep 
>> the input jar file.
>> 
>> Thanks
>> Steve
> 
> 

Re: Question on MVJAR usage

[Steve Drach]

> Thank you Steve. I have only one follow-up then. If the platform version is 
> *always* a major version, does that imply that minor versions will not add 
> API?

No.  Minor releases can add new JDK specific apis.  We’ve chosen to support 
only major release for now, a known restriction.

> The big benefit of MRJAR :-) is that I can provide separate code for 
> different targeted runtimes. If 9.1 wouldn't add new API, then I am good. If 
> that's the policy, great. I just want to confirm it is because I don't see 
> too much of a technical difference between the two cases (8 to 9 vs 9.0 to 
> 9.1). Does that make sense?

Yes, it makes sense, but as I said, it’s a known restriction.

> 
> Cheers,
> Paul
> 
> On Tue, Jul 19, 2016 at 1:41 PM, Steve Drach  <mailto:steve.dr...@oracle.com>> wrote:
> Hi Paul,
> 
> > I have some questions. I believe core-lib is the right place. If not please
> > let me know.
> 
> This is the right place.  First, the name was changed to Multi-Release JAR, 
> so it’s MRJAR (or Mr. Jar) instead of MVJAR.
> 
> >
> > 1) Given a Java 9 runtime, is there any perceptible difference between a
> > non-multiversion jar, and a versioned jar which has placed all its classes
> > under /META-INF/versions/9 ? Pretend each jar has the same identical
> > binaries/resources.
> 
> There is probably a small difference in performance although I haven’t 
> measured it.  Whether the difference is perceptible or not probably depends 
> on the characteristics of the sensor.
> 
> >
> > 2) Does the runtime care if the class version does not match what's under
> > /META-INF/versions/9? For example, what if I have targeted a Java 8 class
> > file format under versions/9?
> 
> The MRJAR runtime does not care.  However if you put a class file targeted 
> for Java 10 in the  /META-INF/versions/9 directory and run it on a Java 9 
> platform, you’ll probably get an error.
> 
> >
> > 3) Why does the new MVJAR JEP writeup [1] use versions/8 in the example?
> 
> Because we don’t have a real example that uses Java 9 and the JEP was written 
> when we thought we’d target this feature for Java 8.
> 
> > Is
> > it simply for illustration, but I don't see how that's a useful example
> > since it's an impossibility. There is no MVJAR support prior to Java 8 so
> > isn't a better (and realer) example be /9 and /10?
> 
> Yes, it would be better, but as I said, we don’t have a real world example 
> yet, so anything we do would be a contrived example.  We probably need to do 
> something with that part of the JEP.
> 
> >
> > 3) The same MVJAR JEP writeup doesn't clearly indicate what is considered a
> > "platform version". All the examples show a single digit, but I believe
> > Verona [2] has specificed the platform to include both major and minor
> > versions. For example, Verona says the minor version may include "revisions
> > to standard APIs mandated by a Maintenance Release of the relevant Platform
> > Specification". Because it mentions platform, it should be possible to do
> > /9, /9.0, and /9.1. Please advise?
> 
> Platform versions are major versions, i.e. 8, 9, 10, etc.  They are the 
> values derived from Runtime.Version::major
> 
> >
> > 4) Although MVJAR JEP writeup says "JAR metadata, such as that found in the
> > MANIFEST.MF file and the META-INF/services directory, need not be
> > versioned." The keyword here is "need not" which is not the same as "can
> > not" or "may not”.
> 
> Yes, you are right, it’s incorrect.  Perhaps it should say "JAR metadata, 
> such as that found in the MANIFEST.MF file and the META-INF/services 
> directory, are not versioned.”
> 
> > So if it is needed, how does one version different
> > services for different platforms?
> 
> It can’t be done.
> 
> > Can there be /META-INF under the
> > appropriate versioned folder?
> 
> Technically, it can be done but it won’t be interpreted as a “true” META-INF 
> directory, it’ll just be a path component for the jar entry.
> 
> > Maybe /META-INF/versions/9/META-INF?
> 
> You won’t achieve what you expect, depending of course on what you expect. ;-)
> 
> > I do not
> > see anything in the JEP that says it's supported or non-supported. Please
> > advise.
> 
> It’s not supported.
> 
> >
> > [1] http://openjdk.java.net/jeps/238 <http://openjdk.java.net/jeps/238>
> > [2] http://openjdk.java.net/jeps/223 <http://openjdk.java.net/jeps/223>
> >
> > Cheers,
> > Paul
> 
> 

Re: Question on MVJAR usage

[Steve Drach]

Hi Paul,

> I have some questions. I believe core-lib is the right place. If not please
> let me know.

This is the right place.  First, the name was changed to Multi-Release JAR, so 
it’s MRJAR (or Mr. Jar) instead of MVJAR.

> 
> 1) Given a Java 9 runtime, is there any perceptible difference between a
> non-multiversion jar, and a versioned jar which has placed all its classes
> under /META-INF/versions/9 ? Pretend each jar has the same identical
> binaries/resources.

There is probably a small difference in performance although I haven’t measured 
it.  Whether the difference is perceptible or not probably depends on the 
characteristics of the sensor.

> 
> 2) Does the runtime care if the class version does not match what's under
> /META-INF/versions/9? For example, what if I have targeted a Java 8 class
> file format under versions/9?

The MRJAR runtime does not care.  However if you put a class file targeted for 
Java 10 in the  /META-INF/versions/9 directory and run it on a Java 9 platform, 
you’ll probably get an error.

> 
> 3) Why does the new MVJAR JEP writeup [1] use versions/8 in the example?

Because we don’t have a real example that uses Java 9 and the JEP was written 
when we thought we’d target this feature for Java 8.

> Is
> it simply for illustration, but I don't see how that's a useful example
> since it's an impossibility. There is no MVJAR support prior to Java 8 so
> isn't a better (and realer) example be /9 and /10?

Yes, it would be better, but as I said, we don’t have a real world example yet, 
so anything we do would be a contrived example.  We probably need to do 
something with that part of the JEP.

> 
> 3) The same MVJAR JEP writeup doesn't clearly indicate what is considered a
> "platform version". All the examples show a single digit, but I believe
> Verona [2] has specificed the platform to include both major and minor
> versions. For example, Verona says the minor version may include "revisions
> to standard APIs mandated by a Maintenance Release of the relevant Platform
> Specification". Because it mentions platform, it should be possible to do
> /9, /9.0, and /9.1. Please advise?

Platform versions are major versions, i.e. 8, 9, 10, etc.  They are the values 
derived from Runtime.Version::major

> 
> 4) Although MVJAR JEP writeup says "JAR metadata, such as that found in the
> MANIFEST.MF file and the META-INF/services directory, need not be
> versioned." The keyword here is "need not" which is not the same as "can
> not" or "may not”.

Yes, you are right, it’s incorrect.  Perhaps it should say "JAR metadata, such 
as that found in the MANIFEST.MF file and the META-INF/services directory, are 
not versioned.”

> So if it is needed, how does one version different
> services for different platforms?

It can’t be done.

> Can there be /META-INF under the
> appropriate versioned folder?

Technically, it can be done but it won’t be interpreted as a “true” META-INF 
directory, it’ll just be a path component for the jar entry.

> Maybe /META-INF/versions/9/META-INF?

You won’t achieve what you expect, depending of course on what you expect. ;-)

> I do not
> see anything in the JEP that says it's supported or non-supported. Please
> advise.

It’s not supported.

> 
> [1] http://openjdk.java.net/jeps/238
> [2] http://openjdk.java.net/jeps/223
> 
> Cheers,
> Paul

Re: RFR: 8157524 Revert JarFile methods "entries" and "stream" to Java 8 behavior

[Steve Drach]

It doesn’t add that much value.  I’ll take it out.

> On Jul 19, 2016, at 1:12 AM, Paul Sandoz  wrote:
> 
> 
>> On 18 Jul 2016, at 20:06, Steve Drach  wrote:
>> 
>>> 
>>> I would be inclined to drop the note for Enumeration and add a @see tag 
>>> referencing the Stream returned method.
>> 
>> I actually had to use the versionedEntries method for jdeps.  Because of 
>> that and because I don’t see the harm with leaving it in, I left it in.
>> 
> 
> You are implicitly making reference in the @apiNote note of entries() to a 
> method in the @apiNote of stream()
> 
> 533  * Iterator it = versionedStream(jf).iterator();
> 
> That’s quite confusing. If you wanna keep the @apiNote in entires i recommend 
> keep the two notes independent, and also add a @see to stream().
> 
> Paul.

Re: RFR: 8158295 Add a multi-release jar validation mechanism to jar tool

[Steve Drach]

>> Please review the following:
>> 
>> webrev: http://cr.openjdk.java.net/~sdrach/8158295/webrev.01/ 
>> 
>> issue: https://bugs.openjdk.java.net/browse/JDK-8158295 
>> 
>> 
>> This changeset adds a multi-release jar validator to jar tool.  After the 
>> jar tool builds a multi-release jar, the potential resultant jar file is 
>> passed to the validator to assure that the jar file meets the minimal 
>> standards of a multi-release jar, in particular that versioned classes have 
>> the same api as base classes they override.  There are other checks, for 
>> example warning if two classes are identical.  If the jar file is invalid, 
>> it is not kept, so —create will not produce a jar file and —update will keep 
>> the input jar file.

I’ve updated the webrev to address the issues raised — 
http://cr.openjdk.java.net/~sdrach/8158295/webrev.02/index.html 


> 
> jar/Main.java
> 
> 284 // This code would be repeated in the create and update 
> sections.
> 285 // In order not to do that, it's here as a consumer.
> 286 Consumer validateAndClose = tmpfile -> {
> Why does it need to be Consumer rather than just a method?

I changed it to a method.

> 
> Then i think you don’t need to rethow, but you can still keep the try block 
> and use finally for File.deleteIfExist since it will not complain for the 
> case where you move.

Done.

> 
> 
> 558 int i1 = s1.indexOf('/', n);
> 559 int i2 = s1.indexOf('/', n);
> 560 if (i1 == -1) throw new NumberFormatException(s1);
> 561 if (i2 == -2) throw new NumberFormatException(s2);
> 
> I think you are trying to reject entries directly under the versioned area so 
> it’s not about numbers (same in the validator, so there is some redundancy?).

A couple things here.  The most obvious is it should be “if (i2 == -1)”.  I 
replaced NumberFormatException with a new InvalidJarException.  I added a 
comment that
this code is used to sort the version numbers as string representations of 
numbers, therefore “9” goes before “10”, not usual for string sorts.

> 
> 
> 588 AtomicBoolean validHolder = new AtomicBoolean();
> 589 validHolder.set(valid);
> 
> Pass valid to the constructor

Done.

> 
> 
> Validator/FingerPrint.java
> 
> It would be useful to have some comments on what is checked in terms of API 
> compatibility. e.g. describe what FingerPrint collects and how Validator uses 
> it.

Added some commentary to FingerPrint.

> 
> 
> FingerPrint.java
> 
> 205 private final Map fields = new HashMap<>();
> 206 private final Map methods = new HashMap<>();
> 
> Not sure you need to use a Map, why not use a Set?

Not sure why I did it with Maps instead of Sets, but I wanted to keep the 
method and field names and maybe that made sense at one time.  It doesn’t now, 
so Sets they are.

> 
> Change Method to be literally a representation of a single method rather than 
> a consolidation that is lossy for the set of exceptions.

Done.

Re: RFR: 8157524 Revert JarFile methods "entries" and "stream" to Java 8 behavior

[Steve Drach]

>> It’s used in two places, moving it to jarPackages won’t work, I think.  I 
>> also put a check in to see if it’s a multi-release jar.
> I meant to move the method down to where jarPackages is located. I would 
> probably adjust the comment too but we can do that once your changes are in 
> jdk9/dev as we have several patches that will change this code. So I think 
> what you have is okay.

Okay.  Will do.

Re: RFR: 8157524 Revert JarFile methods "entries" and "stream" to Java 8 behavior

[Steve Drach]

>> Please review this change to JarFile that reverts JarFile::stream and 
>> JarFile::entries to JDK 8 behavior.  The code is identical to that in JDK 8 
>> except line 504 in JarEntryIterator that now uses a different constructor to 
>> create a JarFileEntry.  I also added some implementation notes explaining 
>> how to create a versionedStream and a versionedEntries method.
>> 
>> issue: https://bugs.openjdk.java.net/browse/JDK-8157524 
>> 
>> webrev: http://cr.openjdk.java.net/~sdrach/8157524/webrev/index.html 
>> 

I put an updated webrev at 
http://cr.openjdk.java.net/~sdrach/8157524/webrev.01/index.html 



> For @implNote then you can put the example in {@code ...} and that will allow 
> to use angled brackets (no need for < etc).

Okay.  I doesn’t seem to allow nested @, so I just dropped the @Override 
annotations on the example

> 
> In ModulePath then it would be good to move versionedStream to jarPackages.

It’s used in two places, moving it to jarPackages won’t work, I think.  I also 
put a check in to see if it’s a multi-release jar.

> A comment on the method would be useful too although we have other changes 
> coming to this code so we can do it then.

I put a comment in

> 
> You should use an @apiNote, as this is presenting some interesting 
> information on how to use this method and not some particular characteristics 
> of the implementation.

Okay

> 
> I would be inclined to drop the note for Enumeration and add a @see tag 
> referencing the Stream returned method.

I actually had to use the versionedEntries method for jdeps.  Because of that 
and because I don’t see the harm with leaving it in, I left it in.

RFR: 8157524 Revert JarFile methods "entries" and "stream" to Java 8 behavior

[Steve Drach]

Hi,

Please review this change to JarFile that reverts JarFile::stream and 
JarFile::entries to JDK 8 behavior.  The code is identical to that in JDK 8 
except line 504 in JarEntryIterator that now uses a different constructor to 
create a JarFileEntry.  I also added some implementation notes explaining how 
to create a versionedStream and a versionedEntries method.

issue: https://bugs.openjdk.java.net/browse/JDK-8157524 

webrev: http://cr.openjdk.java.net/~sdrach/8157524/webrev/index.html 


Thanks,
Steve

RFR: 8158295 Add a multi-release jar validation mechanism to jar tool

[Steve Drach]

Hi,

Please review the following:

webrev: http://cr.openjdk.java.net/~sdrach/8158295/webrev.01/ 

issue: https://bugs.openjdk.java.net/browse/JDK-8158295 


This changeset adds a multi-release jar validator to jar tool.  After the jar 
tool builds a multi-release jar, the potential resultant jar file is passed to 
the validator to assure that the jar file meets the minimal standards of a 
multi-release jar, in particular that versioned classes have the same api as 
base classes they override.  There are other checks, for example warning if two 
classes are identical.  If the jar file is invalid, it is not kept, so —create 
will not produce a jar file and —update will keep the input jar file.

Thanks
Steve

RFR: 8155770 Correct URLClassLoader API documentation to explicitly say jar-scheme URL's are accepted

[Steve Drach]

Hi,

Please review this simple change to the URLClassLoader specification.

issue: https://bugs.openjdk.java.net/browse/JDK-8155770 

webrev: http://cr.openjdk.java.net/~sdrach/8155770/webrev/ 


Thanks,
Steve

Re: RFR: 8150680 JarFile.Release enum needs reconsideration with respect to it's values

[Steve Drach]

> 144 private boolean notVersioned;   // legacy constructor called
> 
> Do you need this?

Unfortunately yes.  It’s used in entries() and stream().  If it’s set, they 
have the JDK 8 semantics.  If not set, entries/stream only see the appropriate 
versioned entries.  This will go away when JDK-8157524 is resolved.

> Is this condition already met if versionMajor == BASE_VERSION_MAJOR ?

No

> 
> i.e. why do you need to restrict to only the “legacy” constructor, since 
> surely the same conditions should apply if a BASE_VERSION compatible version 
> is passed into the version accepting constructor?

It’s the only way to differentiate how the version accepting constructor was 
called, from another constructor or from the “outside” world.

> 
> Paul.

Re: RFR: 8150680 JarFile.Release enum needs reconsideration with respect to it's values

[Steve Drach]

Hi Paul,

> Hi. I would like to point out that it appears JarFile.Release enum is 
> specifically tailored to address multi-version jar specification.

I think you are looking at the current code, not the webrev.  What the webrev 
does is remove the JarFile.Release enum.

> However, I find nothing in that enum specific except for the documentation 
> and BASE_VERSION. Wouldn't it better to create a top-level Release enum that 
> can be used to identify anything in the JDK with release semantics -- apart 
> from Jar files? 
> 
> Cheers,
> Paul
> 
> On Tue, Jun 21, 2016 at 1:23 PM, Steve Drach  <mailto:steve.dr...@oracle.com>> wrote:
> Hi Paul,
> 
> I believe this webrev addresses your concerns:
> 
> http://cr.openjdk.java.net/~sdrach/8150680/webrev.03/index.html 
> <http://cr.openjdk.java.net/~sdrach/8150680/webrev.03/index.html> 
> <http://cr.openjdk.java.net/~sdrach/8150680/webrev.03/index.html 
> <http://cr.openjdk.java.net/~sdrach/8150680/webrev.03/index.html>>
> 
> 
> > On Jun 16, 2016, at 3:49 PM, Paul Sandoz  > <mailto:paul.san...@oracle.com>> wrote:
> >
> >
> >> On 16 Jun 2016, at 14:44, Steve Drach  >> <mailto:steve.dr...@oracle.com>> wrote:
> >>
> >> This webrev uses methods instead of fields to return the base and runtime 
> >> values used internally by JarFile.  I’ve also optimized it a bit.
> >>
> >> http://cr.openjdk.java.net/~sdrach/8150680/webrev.02/ 
> >> <http://cr.openjdk.java.net/~sdrach/8150680/webrev.02/> 
> >> <http://cr.openjdk.java.net/~sdrach/8150680/webrev.02/ 
> >> <http://cr.openjdk.java.net/~sdrach/8150680/webrev.02/>>
> >>
> >
> > JarFIle
> > —
> >
> > 132 private final static int base_version;
> >
> > You are using lower case, here, this caught me out as i thought it was an 
> > non-static field. Call it something like BASE_VERSION_MAJOR.
> >
> >
> > 155 BASE_VERSION = 
> > Runtime.Version.parse(String.valueOf(base_version));
> >
> > 164 RUNTIME_VERSION = 
> > Runtime.Version.parse(String.valueOf(runtimeVersion));
> >
> > Use Integer.toString rather than String.valueOf (also update specification).
> >
> >
> > 337 public final Runtime.Version getVersion() {
> > 338 if (VERSION == null) {
> > 339 if (isMultiRelease()) {
> > 340 VERSION = 
> > Runtime.Version.parse(String.valueOf(version));
> > 341 } else {
> > 342 VERSION = BASE_VERSION;
> > 343 }
> > 344 }
> > 345 return VERSION;
> > 346 }
> > 347 private Runtime.Version VERSION;
> >
> > You are using the style for a static field.
> >
> > In the JarFile constructor why don’t you just store the version passed in 
> > unless MULTI_RELEASE_FORCED?
> >
> > Have final fields:
> >
> >  final Runtime.Version version;
> >  final int version_major;
> >
> > then do:
> >
> >  if (MULTI_RELEASE_FORCED || version.major() == RUNTIME_VERSION.major()) {
> >  // This also deals with the common case where the value from 
> > JarFile.runtimeVersion() is passed
> >  this.version = RUNTIME_VERSION;
> >  } else if (version.major() <= BASE_VERSION_MAJOR) {
> >  // This also deals with the common case where the value from 
> > JarFile.baseVersion() is passed
> >  this.version = BASE_VERSION;
> >  } else {
> > // Canonicalize
> > this.version = Runtime.Version.parse(Integer.toString(version.major()));
> >  }
> >  this.version_major = version.major();
> >
> > Paul.
> >
> >
> >
> >
> >>> On Jun 15, 2016, at 4:31 PM, Joseph D. Darcy  >>> <mailto:joe.da...@oracle.com>> wrote:
> >>>
> >>> Steve,
> >>>
> >>> In JarFile, please use methods not fields to return the new information. 
> >>> The information in question is not constant across versions. Using 
> >>> methods instead of fields avoid over-committing on a particular 
> >>> implementation, etc.
> >>>
> >>> Cheers,
> >>>
> >>> -Joe
> >>>
> >>> On 6/15/2016 3:49 PM, Steve Drach wrote:
> >>>> I’ve updated the webrev to address the issue of the constructor 
> >>>> accepting values like Version.parse(“7.1”)
> >>>>
> >>>> http://cr.openjdk.java.net/~sdrach/8150680/webrev.01/ 
> >>>

Re: RFR: 8150680 JarFile.Release enum needs reconsideration with respect to it's values

[Steve Drach]

Hi Paul,

I believe this webrev addresses your concerns:

http://cr.openjdk.java.net/~sdrach/8150680/webrev.03/index.html 
<http://cr.openjdk.java.net/~sdrach/8150680/webrev.03/index.html>


> On Jun 16, 2016, at 3:49 PM, Paul Sandoz  wrote:
> 
> 
>> On 16 Jun 2016, at 14:44, Steve Drach  wrote:
>> 
>> This webrev uses methods instead of fields to return the base and runtime 
>> values used internally by JarFile.  I’ve also optimized it a bit.
>> 
>> http://cr.openjdk.java.net/~sdrach/8150680/webrev.02/ 
>> <http://cr.openjdk.java.net/~sdrach/8150680/webrev.02/>
>> 
> 
> JarFIle
> —
> 
> 132 private final static int base_version;
> 
> You are using lower case, here, this caught me out as i thought it was an 
> non-static field. Call it something like BASE_VERSION_MAJOR.
> 
> 
> 155 BASE_VERSION = 
> Runtime.Version.parse(String.valueOf(base_version));
> 
> 164 RUNTIME_VERSION = 
> Runtime.Version.parse(String.valueOf(runtimeVersion));
> 
> Use Integer.toString rather than String.valueOf (also update specification).
> 
> 
> 337 public final Runtime.Version getVersion() {
> 338 if (VERSION == null) {
> 339 if (isMultiRelease()) {
> 340 VERSION = Runtime.Version.parse(String.valueOf(version));
> 341 } else {
> 342 VERSION = BASE_VERSION;
> 343 }
> 344 }
> 345 return VERSION;
> 346 }
> 347 private Runtime.Version VERSION;
> 
> You are using the style for a static field.
> 
> In the JarFile constructor why don’t you just store the version passed in 
> unless MULTI_RELEASE_FORCED?
> 
> Have final fields:
> 
>  final Runtime.Version version;
>  final int version_major;
> 
> then do:
> 
>  if (MULTI_RELEASE_FORCED || version.major() == RUNTIME_VERSION.major()) {
>  // This also deals with the common case where the value from 
> JarFile.runtimeVersion() is passed
>  this.version = RUNTIME_VERSION;
>  } else if (version.major() <= BASE_VERSION_MAJOR) {
>  // This also deals with the common case where the value from 
> JarFile.baseVersion() is passed
>  this.version = BASE_VERSION;
>  } else {
> // Canonicalize
> this.version = Runtime.Version.parse(Integer.toString(version.major()));
>  }
>  this.version_major = version.major();
> 
> Paul.
> 
> 
> 
> 
>>> On Jun 15, 2016, at 4:31 PM, Joseph D. Darcy  wrote:
>>> 
>>> Steve,
>>> 
>>> In JarFile, please use methods not fields to return the new information. 
>>> The information in question is not constant across versions. Using methods 
>>> instead of fields avoid over-committing on a particular implementation, etc.
>>> 
>>> Cheers,
>>> 
>>> -Joe
>>> 
>>> On 6/15/2016 3:49 PM, Steve Drach wrote:
>>>> I’ve updated the webrev to address the issue of the constructor accepting 
>>>> values like Version.parse(“7.1”)
>>>> 
>>>> http://cr.openjdk.java.net/~sdrach/8150680/webrev.01/ 
>>>> <http://cr.openjdk.java.net/~sdrach/8150680/webrev.01/>
>>>> 
>>>>> On Jun 15, 2016, at 8:56 AM, Steve Drach  wrote:
>>>>> 
>>>>>>> Please review the following changeset:
>>>>>>> 
>>>>>>> webrev: http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html 
>>>>>>> <http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html>
>>>>>>> issue: https://bugs.openjdk.java.net/browse/JDK-8150680 
>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8150680>
>>>>>>> 
>>>>>>> The issue calls for reconsidering the JarFile.Release enum.  A comment 
>>>>>>> in the bug report suggests replacing JarFile.Release with 
>>>>>>> Runtime.Version, and that’s what I did.  Specifically I removed the 
>>>>>>> enum, changed the constructor to accept a Runtime.Version object 
>>>>>>> instead of a JarFile.Release object, updated all places in the JDK that 
>>>>>>> invoked the constructor and updated all tests.
>>>>>>> 
>>>>>> Moving to Runtime.Version seems right but doesn't the javadoc for the 
>>>>>> constructor need to be updated to make it clear how it behavior when 
>>>>>> invoking with something like Version.parse("7.1") ? If I read the code 
>>>>>> correctly then this will be accepted and getVersion() will return 7.1.
>>>>> Yes, it needs to be updated and it needs to be fixed.  Thanks for finding 
>>>>> that.
>>>>> 
>>>>>> Fields or methods is another discussion point for the base and runtime 
>>>>>> versions.
>>>>> My thinking is, in this case fields and methods are equivalent, the 
>>>>> method not giving any more flexibility than a field.  For example the 
>>>>> method JarFile.baseVersion will just return the value contained in the 
>>>>> private final static field BASE_VERSION.  Or the public final static 
>>>>> field BASE_VERSION can be directly accessed.  I see no advantage of a 
>>>>> method here.  But I’m willing to be enlightened.
>>>>> 
>>>>>> -Alan.
>>> 
>> 
> 

Re: RFR: 8150680 JarFile.Release enum needs reconsideration with respect to it's values

[Steve Drach]

Hi Remi,

> do you have checked that the your patch doesn't load a bunch of supplementary 
> classes at start-up ?
> because JarFile is used at start-up, it triggers the load of Runtime.Version 
> and Runtime.Version has a dependency on a dozen of classes in java.util.regex.

Is it still true, in JDK 9, that JarFile is loaded at start-up?  Using a simple 
Hello World program, I started it with the option -verbose:class and did not 
see either JarFile or Version loaded.  Just to verify, I added a print 
statement to a static initializer in both JarFile and Version, and again did 
not see either class loaded.  I also tried the tests with a jar file on the 
class path and did see both classes loaded.

> 
> cheers,
> Rémi
> 
> ----- Mail original -
>> De: "Steve Drach" 
>> À: "Joseph D. Darcy" 
>> Cc: "Core-Libs-Dev" 
>> Envoyé: Jeudi 16 Juin 2016 23:44:14
>> Objet: [SPAM-RENATER]Re: RFR: 8150680 JarFile.Release enum needs 
>> reconsideration withrespect to it's values
>> 
>> This webrev uses methods instead of fields to return the base and runtime
>> values used internally by JarFile.  I’ve also optimized it a bit.
>> 
>> http://cr.openjdk.java.net/~sdrach/8150680/webrev.02/
>> <http://cr.openjdk.java.net/~sdrach/8150680/webrev.02/>
>> 
>>> On Jun 15, 2016, at 4:31 PM, Joseph D. Darcy  wrote:
>>> 
>>> Steve,
>>> 
>>> In JarFile, please use methods not fields to return the new information.
>>> The information in question is not constant across versions. Using methods
>>> instead of fields avoid over-committing on a particular implementation,
>>> etc.
>>> 
>>> Cheers,
>>> 
>>> -Joe
>>> 
>>> On 6/15/2016 3:49 PM, Steve Drach wrote:
>>>> I’ve updated the webrev to address the issue of the constructor accepting
>>>> values like Version.parse(“7.1”)
>>>> 
>>>> http://cr.openjdk.java.net/~sdrach/8150680/webrev.01/
>>>> <http://cr.openjdk.java.net/~sdrach/8150680/webrev.01/>
>>>> 
>>>>> On Jun 15, 2016, at 8:56 AM, Steve Drach  wrote:
>>>>> 
>>>>>>> Please review the following changeset:
>>>>>>> 
>>>>>>> webrev: http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html
>>>>>>> <http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html>
>>>>>>> issue: https://bugs.openjdk.java.net/browse/JDK-8150680
>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8150680>
>>>>>>> 
>>>>>>> The issue calls for reconsidering the JarFile.Release enum.  A comment
>>>>>>> in the bug report suggests replacing JarFile.Release with
>>>>>>> Runtime.Version, and that’s what I did.  Specifically I removed the
>>>>>>> enum, changed the constructor to accept a Runtime.Version object
>>>>>>> instead of a JarFile.Release object, updated all places in the JDK
>>>>>>> that invoked the constructor and updated all tests.
>>>>>>> 
>>>>>> Moving to Runtime.Version seems right but doesn't the javadoc for the
>>>>>> constructor need to be updated to make it clear how it behavior when
>>>>>> invoking with something like Version.parse("7.1") ? If I read the code
>>>>>> correctly then this will be accepted and getVersion() will return 7.1.
>>>>> Yes, it needs to be updated and it needs to be fixed.  Thanks for finding
>>>>> that.
>>>>> 
>>>>>> Fields or methods is another discussion point for the base and runtime
>>>>>> versions.
>>>>> My thinking is, in this case fields and methods are equivalent, the
>>>>> method not giving any more flexibility than a field.  For example the
>>>>> method JarFile.baseVersion will just return the value contained in the
>>>>> private final static field BASE_VERSION.  Or the public final static
>>>>> field BASE_VERSION can be directly accessed.  I see no advantage of a
>>>>> method here.  But I’m willing to be enlightened.
>>>>> 
>>>>>> -Alan.
>>> 
>> 
>> 

Re: RFR: 8150680 JarFile.Release enum needs reconsideration with respect to it's values

[Steve Drach]

This webrev uses methods instead of fields to return the base and runtime 
values used internally by JarFile.  I’ve also optimized it a bit.

http://cr.openjdk.java.net/~sdrach/8150680/webrev.02/ 
<http://cr.openjdk.java.net/~sdrach/8150680/webrev.02/>
 
> On Jun 15, 2016, at 4:31 PM, Joseph D. Darcy  wrote:
> 
> Steve,
> 
> In JarFile, please use methods not fields to return the new information. The 
> information in question is not constant across versions. Using methods 
> instead of fields avoid over-committing on a particular implementation, etc.
> 
> Cheers,
> 
> -Joe
> 
> On 6/15/2016 3:49 PM, Steve Drach wrote:
>> I’ve updated the webrev to address the issue of the constructor accepting 
>> values like Version.parse(“7.1”)
>> 
>> http://cr.openjdk.java.net/~sdrach/8150680/webrev.01/ 
>> <http://cr.openjdk.java.net/~sdrach/8150680/webrev.01/>
>> 
>>> On Jun 15, 2016, at 8:56 AM, Steve Drach  wrote:
>>> 
>>>>> Please review the following changeset:
>>>>> 
>>>>> webrev: http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html 
>>>>> <http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html>
>>>>> issue: https://bugs.openjdk.java.net/browse/JDK-8150680 
>>>>> <https://bugs.openjdk.java.net/browse/JDK-8150680>
>>>>> 
>>>>> The issue calls for reconsidering the JarFile.Release enum.  A comment in 
>>>>> the bug report suggests replacing JarFile.Release with Runtime.Version, 
>>>>> and that’s what I did.  Specifically I removed the enum, changed the 
>>>>> constructor to accept a Runtime.Version object instead of a 
>>>>> JarFile.Release object, updated all places in the JDK that invoked the 
>>>>> constructor and updated all tests.
>>>>> 
>>>> Moving to Runtime.Version seems right but doesn't the javadoc for the 
>>>> constructor need to be updated to make it clear how it behavior when 
>>>> invoking with something like Version.parse("7.1") ? If I read the code 
>>>> correctly then this will be accepted and getVersion() will return 7.1.
>>> Yes, it needs to be updated and it needs to be fixed.  Thanks for finding 
>>> that.
>>> 
>>>> Fields or methods is another discussion point for the base and runtime 
>>>> versions.
>>> My thinking is, in this case fields and methods are equivalent, the method 
>>> not giving any more flexibility than a field.  For example the method 
>>> JarFile.baseVersion will just return the value contained in the private 
>>> final static field BASE_VERSION.  Or the public final static field 
>>> BASE_VERSION can be directly accessed.  I see no advantage of a method 
>>> here.  But I’m willing to be enlightened.
>>> 
>>>> -Alan.
> 

Re: RFR: 8150680 JarFile.Release enum needs reconsideration with respect to it's values

[Steve Drach]

I’ve updated the webrev to address the issue of the constructor accepting 
values like Version.parse(“7.1”)

http://cr.openjdk.java.net/~sdrach/8150680/webrev.01/ 
<http://cr.openjdk.java.net/~sdrach/8150680/webrev.01/>

> On Jun 15, 2016, at 8:56 AM, Steve Drach  wrote:
> 
>>> Please review the following changeset:
>>> 
>>> webrev: http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html 
>>> <http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html>
>>> issue: https://bugs.openjdk.java.net/browse/JDK-8150680 
>>> <https://bugs.openjdk.java.net/browse/JDK-8150680>
>>> 
>>> The issue calls for reconsidering the JarFile.Release enum.  A comment in 
>>> the bug report suggests replacing JarFile.Release with Runtime.Version, and 
>>> that’s what I did.  Specifically I removed the enum, changed the 
>>> constructor to accept a Runtime.Version object instead of a JarFile.Release 
>>> object, updated all places in the JDK that invoked the constructor and 
>>> updated all tests.
>>> 
>> Moving to Runtime.Version seems right but doesn't the javadoc for the 
>> constructor need to be updated to make it clear how it behavior when 
>> invoking with something like Version.parse("7.1") ? If I read the code 
>> correctly then this will be accepted and getVersion() will return 7.1.
> 
> Yes, it needs to be updated and it needs to be fixed.  Thanks for finding 
> that.
> 
>> 
>> Fields or methods is another discussion point for the base and runtime 
>> versions.
> 
> My thinking is, in this case fields and methods are equivalent, the method 
> not giving any more flexibility than a field.  For example the method 
> JarFile.baseVersion will just return the value contained in the private final 
> static field BASE_VERSION.  Or the public final static field BASE_VERSION can 
> be directly accessed.  I see no advantage of a method here.  But I’m willing 
> to be enlightened.
> 
>> 
>> -Alan.
> 

Re: RFR: 8150680 JarFile.Release enum needs reconsideration with respect to it's values

[Steve Drach]

>> Please review the following changeset:
>> 
>> webrev: http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html 
>> 
>> issue: https://bugs.openjdk.java.net/browse/JDK-8150680 
>> 
>> 
>> The issue calls for reconsidering the JarFile.Release enum.  A comment in 
>> the bug report suggests replacing JarFile.Release with Runtime.Version, and 
>> that’s what I did.  Specifically I removed the enum, changed the constructor 
>> to accept a Runtime.Version object instead of a JarFile.Release object, 
>> updated all places in the JDK that invoked the constructor and updated all 
>> tests.
>> 
> Moving to Runtime.Version seems right but doesn't the javadoc for the 
> constructor need to be updated to make it clear how it behavior when invoking 
> with something like Version.parse("7.1") ? If I read the code correctly then 
> this will be accepted and getVersion() will return 7.1.

Yes, it needs to be updated and it needs to be fixed.  Thanks for finding that.

> 
> Fields or methods is another discussion point for the base and runtime 
> versions.

My thinking is, in this case fields and methods are equivalent, the method not 
giving any more flexibility than a field.  For example the method 
JarFile.baseVersion will just return the value contained in the private final 
static field BASE_VERSION.  Or the public final static field BASE_VERSION can 
be directly accessed.  I see no advantage of a method here.  But I’m willing to 
be enlightened.

> 
> -Alan.

RFR: 8150680 JarFile.Release enum needs reconsideration with respect to it's values

[Steve Drach]

Hi,

Please review the following changeset:

webrev: http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html 

issue: https://bugs.openjdk.java.net/browse/JDK-8150680 


The issue calls for reconsidering the JarFile.Release enum.  A comment in the 
bug report suggests replacing JarFile.Release with Runtime.Version, and that’s 
what I did.  Specifically I removed the enum, changed the constructor to accept 
a Runtime.Version object instead of a JarFile.Release object, updated all 
places in the JDK that invoked the constructor and updated all tests.

Thanks,
Steve

Re: RFR: 8153652 Update javap to be multi-release jar aware

[Steve Drach]

>> Please review the following changeset that simply supplies the help 
>> information for the already existing javap command line option, 
>> -multi-release.
>> 
>> webrev: http://cr.openjdk.java.net/~sdrach/8153652/webrev.00/ 
>> 
>> issue: https://bugs.openjdk.java.net/browse/JDK-8153652 
>> 
>> 
>> It turns out that javap forwards unrecognized command line options to the 
>> JavaFileManager for processing.  One such option is -multi-release. The 
>> value that the -multi-release option is set to is used by JavaFileManager to 
>> open multi-release jar files so that the appropriate versioned view is 
>> presented to the client, javap in this case.  All this changeset does is add 
>> a help message describing the existing -multi-release command line option.
>> 
>> The values that can be assigned to this option, and the corresponding 
>> multi-release modes that the jar file is configured for are:
>> 
>> 9 -> JarFile.Release.VERSION_9
>> runtime   -> JarFile.Release.RUNTIME
>> all others -> JarFile.Release.BASE
>> 
>> If the option is not present, the jar file mode is JarFile.Release.Base.
>> 
> Is -multi-release the agreed option? Just curious as I would have expected 
> -multirelease. If GNU style then it would be --multi-release of course.

-multi-release is the option defined in javac options

Re: RFR: 81536534 Update jdeps to be multi-release jar aware

[Steve Drach]

Hi Mandy,

>> Please review the following changeset to make jdeps multi-release jar aware.
>> 
>> webrev: http://cr.openjdk.java.net/~sdrach/8153654/webrev.00/index.html 
>> 
>> issue: https://bugs.openjdk.java.net/browse/JDK-8153654 
>> 
>> 
>> The changeset adds a new command line option to jdeps.  The -multi-release 
>> option can be set to the string “runtime” or an integral string value 
>> corresponding to the Java platform releases starting at 9 (i.e. 9, 10, 11, 
>> etc.).  Jar files that jdeps accesses, either on the class path or as a 
>> target, are opened with the appropriate JarFile.Release parameter.  The 
>> mapping from -multi-release value to JarFile.Release is:
>> 
>> 9 -> JarFile.Release.VERSION_9
>> runtime   -> JarFile.Release.RUNTIME
>> all others -> JarFile.Release.BASE
>> 
>> If the option is not present, the jar file mode is JarFile.Release.Base.
> 
> Have you considered parsing all versioned entries in MRJAR as the default and 
> include the version information in the output?

Briefly ;-)  But now that you brought it up, I think the right thing to do is 
to rescind this RFR and redesign the output for jdeps so that it’ll be more 
useful for the users by doing what you suggest.

>  
> 
> A relevant implementation details:
> 
> jdeps is using its runtime environment to analyse the class dependencies.  
> The potential missing information when analyzing a MR jar is some JDK 
> internal APIs not present in the current runtime but exists in the older 
> release. jdeps maintains a small list of the removed sun.misc and sun.reflect 
> internal APIs to emit useful information since existing libraries may depend 
> on those removed types that already have replacement; otherwise, they will be 
> shown as not found which is accurate as CNFE may be thrown if that library is 
> running on JDK 9.
> 
> Mandy
> 

RFR: 81536534 Update jdeps to be multi-release jar aware

[Steve Drach]

Hi,

Please review the following changeset to make jdeps multi-release jar aware.

webrev: http://cr.openjdk.java.net/~sdrach/8153654/webrev.00/index.html 

issue: https://bugs.openjdk.java.net/browse/JDK-8153654 


The changeset adds a new command line option to jdeps.  The -multi-release 
option can be set to the string “runtime” or an integral string value 
corresponding to the Java platform releases starting at 9 (i.e. 9, 10, 11, 
etc.).  Jar files that jdeps accesses, either on the class path or as a target, 
are opened with the appropriate JarFile.Release parameter.  The mapping from 
-multi-release value to JarFile.Release is:

9 -> JarFile.Release.VERSION_9
runtime   -> JarFile.Release.RUNTIME
all others -> JarFile.Release.BASE

If the option is not present, the jar file mode is JarFile.Release.Base.

Thanks,
Steve

RFR: 8153652 Update javap to be multi-release jar aware

[Steve Drach]

Hi,

Please review the following changeset that simply supplies the help information 
for the already existing javap command line option, -multi-release.

webrev: http://cr.openjdk.java.net/~sdrach/8153652/webrev.00/ 

issue: https://bugs.openjdk.java.net/browse/JDK-8153652 


It turns out that javap forwards unrecognized command line options to the 
JavaFileManager for processing.  One such option is -multi-release.  The value 
that the -multi-release option is set to is used by JavaFileManager to open 
multi-release jar files so that the appropriate versioned view is presented to 
the client, javap in this case.  All this changeset does is add a help message 
describing the existing -multi-release command line option. 

The values that can be assigned to this option, and the corresponding 
multi-release modes that the jar file is configured for are:

9 -> JarFile.Release.VERSION_9
runtime   -> JarFile.Release.RUNTIME
all others -> JarFile.Release.BASE

If the option is not present, the jar file mode is JarFile.Release.Base.

Thanks,
Steve

RFR: 8114827: JDK 9 multi-release enabled jar tool

[Steve Drach]

Hi,

Please review the following changeset that makes it easier to create 
multi-release jar files with jar tool.  

webrev: http://cr.openjdk.java.net/~sdrach/8114827/webrev.01/
issue: https://bugs.openjdk.java.net/browse/JDK-8114827

The changeset is the implementation of a new command line option, —release n, 
that indicates all following files and directories are placed in the 
META-INF/versions/n directory of a multi-release jar file.  The new command 
line syntax is

jar [OPTION...] [ [--release VERSION] [-C dir] files] …

An example is

jar --create --file mr.jar README -C foo classes --release 9 -C foo9 classes 
Foo.class

This will put README and all the files under foo/classes in the base (or root) 
directory of a jar file and put Foo.class and all the files under foo9/classes 
in the META-INF/versions/9 directory of the jar file.

Re: RFR: JDK-8151914 java/util/jar/JarFile/MultiReleaseJar* tests do not declare module dependences

[Steve Drach]

Looks fine to me, although I am not an official reviewer.  Thanks for doing 
this.

> On May 2, 2016, at 1:03 PM, Alexandre (Shura) Iline 
>  wrote:
> 
> Hi,
> 
> Can you please take a look on:
> http://cr.openjdk.java.net/~shurailine/8151914/webrev.01/
> ?
> 
> Thank you
> 
> Shura
> 

Re: RFR: 8151542: URL resources for multi-release jar files have a #runtime fragment appended to them

[Steve Drach]

Another webrev:  
http://cr.openjdk.java.net/~sdrach/8151542/webrev.05/index.html 


Only URLClassPath has changed.  I put a comment on url.getProtocol indicating 
the URL assures it’s lower case and I changed the long lines into multiple 
shorter lines.

> Also, it may be worth considering an overloaded JarLoader constructor
> that accepts a “jar:” URL, rather than stripping and wrapping.

I tried that this morning.  There aren’t any “good” constructors for URL that 
have a jarHandler that I can use.  The ones I found do much more work than
what we are doing.

> 
> Also, some jar URLs will now go through JarLoader that previously didn’t.
> I’m not saying that this wrong, just trying to understand the behavioural 
> change. For example, some URLs that previously didn’t have their jar index
> checked now will.
> 
> I’ve been looking at this code, for another reason, sorry you have had to
> deal with it, it’s a bit of a mess.

It is indeed a bit of a mess


>  We may need to revisit some of this when
> 8155770 is resolved.
> 
> -Chris.
> 
> 
> 

Re: RFR: 8151542: URL resources for multi-release jar files have a #runtime fragment appended to them

[Steve Drach]

Hopefully the last one ;-)  This webrev removes the lowercase of protocol, and 
incorporates better (in my mind) seperation of choices for choosing the loader, 
similar to what Paul suggested.  Everything else remains the same.  Only 
URLClassPath changed from previous webrev.

http://cr.openjdk.java.net/~sdrach/8151542/webrev.04/index.html 

Re: RFR: 8151542: URL resources for multi-release jar files have a #runtime fragment appended to them

[Steve Drach]

> On Apr 29, 2016, at 4:47 PM, Xueming Shen  wrote:
> 
> On 4/29/16 3:30 PM, Steve Drach wrote:
>> I updated the webrev with changes that Alan suggested
>> 
>>  still needs to be fixed to compare the URL protocol without regard to 
>> case
> 
> -> toLowerCase(Locale.ROOT).

Actually it’s already canonicalized in URL, so I don’t need to do it.

> 
>> 
>> And Sherman suggested
>> 
>>  Shouldn't the realname just be the "super.getName()” ?
>> 
>> The webrev is at 
>> http://cr.openjdk.java.net/~sdrach/8151542/webrev.03/index.html 
>> <http://cr.openjdk.java.net/%7Esdrach/8151542/webrev.03/index.html>
>> 
> 

Re: RFR: 8151542: URL resources for multi-release jar files have a #runtime fragment appended to them

[Steve Drach]

I updated the webrev with changes that Alan suggested

still needs to be fixed to compare the URL protocol without regard to 
case

And Sherman suggested

Shouldn't the realname just be the "super.getName()” ?

The webrev is at 
http://cr.openjdk.java.net/~sdrach/8151542/webrev.03/index.html 

Re: RFR: 8151542: URL resources for multi-release jar files have a #runtime fragment appended to them

[Steve Drach]

>> I just didn’t see it as any higher than P4.  What would you like it to be?
>> 
>>
> We've stumbled on an issue where the spec and implementation are in conflict. 
> The right thing to do it to fix it while we have a handle on it.

I bumped the priority to P2.

Re: RFR: 8151542: URL resources for multi-release jar files have a #runtime fragment appended to them

[Steve Drach]

>> I put a new webrev out with the change suggested by Paul, using 
>> file.indexOf(“!/“) instead of file.endsWith(“!/“).
>> 
>> http://cr.openjdk.java.net/~sdrach/8151542/webrev.02/index.html 
>> This 
>> still needs to be fixed to compare the URL protocol without regard to case 
>> (yes, some of the old code isn't right but best not to make it worss).

Okay.

> 
>> 
>>> So you are planning to eventually change the URLClassLoader spec to allow 
>>> this or not?
>> 
>> I think we should push this changeset since URLClassLoader has always 
>> accepted the jar:…!/ URL and all this changeset does is enable it  to 
>> support multi-release jars.  And create an issue to track the change to the 
>> URLClassLoader api.
>> 
> There shouldn't be any urgency with this change going in but if you are doing 
> it this way then please create a high priority bug to get the spec and 
> implementation aligned.

Remember that I’ve not really changed anything, just enhanced what already 
exists to support multi-release jar files.  I submitted a bug, JDK-8155770, but 
I marked it as P4 according to the standard classifications:

P1 - Blocks development and/or testing work, production could not run.
P2 - Crashes, loss of data, severe memory leak.
P3 - Major loss of function.
P4 - Minor loss of function, or other problem where easy workaround is present.

I just didn’t see it as any higher than P4.  What would you like it to be?

 
 

Re: RFR: 8151542: URL resources for multi-release jar files have a #runtime fragment appended to them

[Steve Drach]

I put a new webrev out with the change suggested by Paul, using 
file.indexOf(“!/“) instead of file.endsWith(“!/“).

http://cr.openjdk.java.net/~sdrach/8151542/webrev.02/index.html 


> So you are planning to eventually change the URLClassLoader spec to allow 
> this or not?

I think we should push this changeset since URLClassLoader has always accepted 
the jar:…!/ URL and all this changeset does is enable it  to support 
multi-release jars.  And create an issue to track the change to the 
URLClassLoader api.

Re: RFR: 8151542: URL resources for multi-release jar files have a #runtime fragment appended to them

[Steve Drach]

>> I’ve updated the webrev to change all instances of the word “reified” to 
>> “real” as in getRealName().
>> 
>> Issue: https://bugs.openjdk.java.net/browse/JDK-8151542 
>> 
>> 
>> Webrev: http://cr.openjdk.java.net/~sdrach/8151542/webrev.01/ 
>> 
>>  
> The src changes looks okay but did we come to a conclusion on URLClassLoader 
> spec? If not, can we revert the change to URLClassPath getLoader and deal 
> with it separately?

If we revert the change to URLClassPath, then we can’t read multi-release jars 
referenced by the “jar:{file,http}:/some/path/mr.jar!/“ form of a URL.  These 
jars would be treated as non-versioned.  That would mean that a jar referenced 
by the URL jar:file:/some/path/mr.jar!/ and one referenced by the URL 
file:/some/path/mr.jar could supply different classes for the same requested 
entry.  Is that what we want?

RFR: 8151542: URL resources for multi-release jar files have a #runtime fragment appended to them

[Steve Drach]

I’ve updated the webrev to change all instances of the word “reified” to “real” 
as in getRealName().

Issue: https://bugs.openjdk.java.net/browse/JDK-8151542 


Webrev: http://cr.openjdk.java.net/~sdrach/8151542/webrev.01/ 

 

Re: RFR: 8151542: URL resources for multi-release jar files have a #runtime fragment appended to them

[Steve Drach]

> On Apr 28, 2016, at 12:03 PM, Alan Bateman  wrote:
> 
> 
> 
> On 28/04/2016 19:53, Steve Drach wrote:
>> :
>> Yes, and for regular jar files, that worked fine, but when we tried it with 
>> a multi-release jar we found it by passed the part of JarLoader where we 
>> open the jar file as a runtime jar, so, for example, this code fails to 
>> retrieve the correct versioned entry, returning instead the base entry.
>> 
>> URL[] urls = { new URL(“jar:file:/foo/multi-release.jar!/“) };
>> URLClassLoader cldr = new URLClassLoader(urls);
>> Class vcls = cldr.loadClass("version.Version”);
>> 
>> The change just corrects the logic when working with a “jar:…..!/“ URL.
>> 
>> 
> Can you double check the URLClassLoader spec?

We discussed it.  It seems the spec might be deficit with respect to 
"jar:file:/foo/multi-release.jar!/“ type URLs, but it didn’t matter when it 
referred to a legacy jar file.

> 
> Also I assume the URL you are want here is "file:/foo/multi-release.jar”

No, that one is correct and still works as it did before.

> as "jar:file:/foo/multi-release.jar!/" is the URL to the top-level directory 
> in the JAR file.

The spec for JarURLconnections says

"If the entry name is omitted, the URL refers to the whole JAR file: 
jar:http://www.foo.com/bar/baz.jar!/“ 
<http://www.foo.com/bar/baz.jar!/%E2%80%9C>

that’s the way we are using it, as the location of a jar file.

I’ve ran this through jprt with test core and found no errors/failures with 
this change.  

> 
> -Alan

Re: RFR: 8151542: URL resources for multi-release jar files have a #runtime fragment appended to them

[Steve Drach]

>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8151542 
>>> 
>>> 
>>> Webrev: http://cr.openjdk.java.net/~sdrach/8151542/webrev/ 
>>> 
>>> 
>>> This changeset causes the URL returned from a ClassLoader.getResource(name) 
>>> invocation to be reified, that is the URL is a direct pointer to either a 
>>> versioned or unversioned entry in the jar file. The patch also assures that 
>>> jar URL’s are always processed by the URLClassPath.JarLoader.  The 
>>> MultiReleaseJarURLConnection test was enhanced to demonstrate that reified 
>>> URLs are returned.  The SimpleHttpServer test helper class was moved into 
>>> it’s own file.
>> I was happy to see John's note on diction so I assume the method will be 
>> renamed. Have you considered making it public so that tools and libraries 
>> outside of the JDK can use this?
> 
> It sounds reasonable for this to be a public API,

I opened JDK-8155657 to track that.

> and if so, does it make sense to
> move it to JarEntry ( rather than JarFile )? So it would be 
> JarEntry::getTrueName,
> or similar.

The part of JarFile that knows all about versioning is a subclass of JarEntry, 
JarFileEntry.  JarEntry knows nothing about  versioning, nor about 
JarFileEntry.  Only JarFile knows about JarFileEntry.

Re: RFR: 8151542: URL resources for multi-release jar files have a #runtime fragment appended to them

[Steve Drach]

>> Issue: https://bugs.openjdk.java.net/browse/JDK-8151542 
>> 
>> 
>> Webrev: http://cr.openjdk.java.net/~sdrach/8151542/webrev/ 
>> 
>> 
>> This changeset causes the URL returned from a ClassLoader.getResource(name) 
>> invocation to be reified, that is the URL is a direct pointer to either a 
>> versioned or unversioned entry in the jar file. The patch also assures that 
>> jar URL’s are always processed by the URLClassPath.JarLoader.  The 
>> MultiReleaseJarURLConnection test was enhanced to demonstrate that reified 
>> URLs are returned.  The SimpleHttpServer test helper class was moved into 
>> it’s own file.
> I was happy to see John's note on diction so I assume the method will be 
> renamed. Have you considered making it public so that tools and libraries 
> outside of the JDK can use this?

I opened an issue to track this — JDK-8155657.  We’d like to get this 
changeset, including the name change for getReifieidName, integrated as soon as 
we can.

> 
> One question on URLClassPath getLoader as it's not immediately obvious that 
> this is needed. URLClassLoader specifies that any URL ending with "/" is 
> assumed to be a directory.

Yes, and for regular jar files, that worked fine, but when we tried it with a 
multi-release jar we found it by passed the part of JarLoader where we open the 
jar file as a runtime jar, so, for example, this code fails to retrieve the 
correct versioned entry, returning instead the base entry.

URL[] urls = { new URL(“jar:file:/foo/multi-release.jar!/“) };
URLClassLoader cldr = new URLClassLoader(urls);
Class vcls = cldr.loadClass("version.Version”);

The change just corrects the logic when working with a “jar:…..!/“ URL.


> Are you planning to update java.net.JarURLConnection as that has special 
> handling for #runtime that will need to be replaced.

No, we are not.  Appending the #runtime fragment in URLClassPath is the way we 
communicate to URLJarFile that the jar should be opened as a runtime versioned 
JarFile.  All this changeset does is assure that the internal implementation 
details are not leaked to the outside world.

> 
> -Alan

Re: RFR: 8151542: URL resources for multi-release jar files have a #runtime fragment appended to them

[Steve Drach]

Keeping with the path precedent, is it acceptable to change “getReifiedName” to 
“getRealName”?  

>>> 
>>> Diction Note: Reified X means X wasn't real (in some sense) until now. As 
>>> in non-reified types, which are not real at runtime because the static 
>>> compiler discarded them.
>>> 
>> 
>> I suggested reified because i thought it fit with the notion of making 
>> something abstract more concrete, but perhaps this just confuses matters?
> 
> It's the real name, but since it already exists (because that's how it is 
> stored) it isn't really reified, it's just revealed.
> 
> This API uses the term "real name" for an almost identical phenomenon (target 
> of a sym-link in a file system):
> 
> https://docs.oracle.com/javase/8/docs/api/java/nio/file/Path.html#toRealPath-java.nio.file.LinkOption...-
> 
> In a versioned jar, the logical names are sometimes mapped to other names 
> under which the elements are actually stored.
> Thus, they behave like sym-links.  (But with a bit of control context thrown 
> in, the active version.)
> 
> On old-fashioned file systems with real version numbers, the Common Lisp 
> function TRUENAME does exactly what you are trying to do here.
> 
> http://www.mathcs.duq.edu/simon/Gcl/gcl_1141.html
> 
> (And in some way, we are sliding down the slope toward re-inventing those 
> file systems, aren't we?)
> 
> The older pre-nio API for File calls it "getCanonicalPath", but I think "true 
> name" is better than
> "canonical name", since "canonical" means "follows the rules", rather than 
> what we need in this case,
> which is "where it really is stored".
> 
> http://docs.oracle.com/javase/8/docs/api/java/io/File.html#getCanonicalPath--
> 
>> 
>>> In this case it appears you are simply exposing a translated name, not 
>>> making it real for the first time.
>>> 
>>> If this is true, I think you want to say "true name" or "real name" or 
>>> "translated name", not "reified name”.
>> 
>> or “versioned name" would work for me.
> 
> I'm just whinging about the term "reified" which doesn't seem to work, 
> logically.
> 
> "Versioned name" would work for me too.  But "true name" has the two good 
> precedents cited above.
> 
> — John

RFR: 8151542: URL resources for multi-release jar files have a #runtime fragment appended to them

[Steve Drach]

Issue: https://bugs.openjdk.java.net/browse/JDK-8151542 


Webrev: http://cr.openjdk.java.net/~sdrach/8151542/webrev/ 


This changeset causes the URL returned from a ClassLoader.getResource(name) 
invocation to be reified, that is the URL is a direct pointer to either a 
versioned or unversioned entry in the jar file. The patch also assures that jar 
URL’s are always processed by the URLClassPath.JarLoader.  The 
MultiReleaseJarURLConnection test was enhanced to demonstrate that reified URLs 
are returned.  The SimpleHttpServer test helper class was moved into it’s own 
file.

Re: Multi-Release JAR runtime support

[Steve Drach]

Yes.

> On Apr 19, 2016, at 5:11 PM, mark.reinh...@oracle.com wrote:
> 
> 2016/4/19 12:37:41 -0700, Hervé BOUTEMY :
>> that's it: I added this Multi-Release: true attribute configuration in the 
>> demo and now it works like a charm, thank you
>> 
>> Perhaps this requirement should be described in 
>> http://openjdk.java.net/jeps/238
> 
> Yes -- that was certainly non-obvious!
> 
> Steve, could you please add this to the JEP?  (A JEP should, in general,
> contain and/or refer to enough information for an experienced developer
> to get started using the feature.)
> 
> Thanks,
> - Mark

Re: Multi-Release JAR runtime support

[Steve Drach]

Hi Herve,

I checked the jar file created from your code and, as others have suspected, 
the manifest does not contain the “Multi-Release” attribute.  I added the 
attribute with emacs and tried it out:

$ java -classpath multirelease/target/multirelease-0.8-SNAPSHOT.jar base.Base
9-ea+113
FROM BASE -> NINE

Regards
Steve

Request for comments -- resource reification vs. mrjar scheme for runtime versioning of multi-release jar files

[Steve Drach]

We’ve identified two possible ways to address issue JDK-8151542. 


One way is to append a #runtime fragment to the input URL in URLClassPath to 
convey to URLJarFile  that we want to have the JarFile opened with the 
parameter Release.RUNTIME, so any loaded classes are runtime versioned.  This 
is currently implemented and was integrated as part of issue JDK-8132734 
  For this case, a resource 
URL returned from ClassLoader.getResource(s) is reified, that is the returned 
URL does not have the #runtime fragment appended to it, instead the URL points 
directly to the desired entry.

The other way is to create a new URL scheme “mrjar” so that when URLJarFile 
sees the URL it knows to open the Jarfile with the Release.RUNTIME parameter.  
No fragment is required.  A returned resource URL will be of the form 
“mrjar:!/{entry}

We’ve put together the following list of pros/cons for each approach.  We’re 
soliciting feedback from the mailing list.  We currently have a working patch 
for the reification approach, but not one for the new scheme approach.

reification pros
—
. produces a standard/normal URL pointing directly to the jar entry
. the String equivalent is parsable so it shouldn’t affect legacy code

reification cons
—
. exposes the internals of a jar (i.e explicitly shows the META-INF/versions 
directory)
. the input URL is modified by attaching a #runtime fragment to it
. URL is not “portable” across jars as platform release version changes

mrjar scheme pros
—
. one URL that always points to runtime versioned entries
. the same URL (with entry appended) is returned by the getResource method
. portable across different platform releases
. jigsaw has also introduced a new URL scheme

mrjar scheme cons
—
. may break String based parsing of URLS
. non-standard URL scheme
. what does it mean with non MR files?
. we haven’t put together a prototype yet

As I said, we’re soliciting feedback from the list.  My personal opinion is 
that we should go with what we have, the reification approach, since it’s least 
likely to break existing code.

RFR: 8153213: Jar manifest attribute "Multi-Release" accepts any value

[Steve Drach]

Hi,

I’ve updated the following patch, incorporating code by Claes Redestad to 
handle some corner cases while processing the attribute value.  Note that we’ve 
limited the location of the value part of the attribute to accommodate startup 
performance requirements.  It’s not without precedent as at least one other 
attribute is also limited in amount of whitespace surrounding the value.

> Please review this simple fix to require that the jar manifest Multi-Release 
> attribute has a value of “true" in order to be effective, i.e. to assert the 
> jar is a multi-release jar.

issue: https://bugs.openjdk.java.net/browse/JDK-8153213 

webrev: http://cr.openjdk.java.net/~sdrach/8153213/webrev.01/index.html 



Thanks
Steve

Re: RFR: 8153213: Jar manifest attribute "Multi-Release" accepts any value

[Steve Drach]

The new webrev is http://cr.openjdk.java.net/~sdrach/8153213/webrev/index.html 
<http://cr.openjdk.java.net/~sdrach/8153213/webrev/index.html>
and the issue is https://bugs.openjdk.java.net/browse/JDK-8153213 
<https://bugs.openjdk.java.net/browse/JDK-8153213>

Now the value “true” followed by either ‘\r’ or ‘\n’ is the only acceptable 
value.

> On Apr 8, 2016, at 11:34 AM, Steve Drach  wrote:
> 
> Okay, I’ll prepare a new webrev.  I think all we need to check for after 
> “true” is \r or \n.  If the manifest just ends without at least one of those, 
> it’s not a legal manifest.
> 
>> On Apr 8, 2016, at 11:25 AM, Claes Redestad  
>> wrote:
>> 
>> On 04/08/2016 07:54 PM, Steve Drach wrote:
>>>>>>> I’ve put up a new webrev that addresses the issue of having spaces 
>>>>>>> before (and after) the value “true” in the Multi-Release attribute.
>>>>>>> 
>>>>>> Is some or all of that really necessary? since the we can specify domain 
>>>>>> of values.
>>>>> I think it is.  The spec states that one can have an arbitrary amount of 
>>>>> leading/trailing spaces in the value part of the attribute.  Apparently 
>>>>> attribute values could also have “hidden” characters such as vertical tab 
>>>>> or nak for example.  I wish the spec was tighter here.  I’m merely 
>>>>> stating the the domain can be
>>>>> “ *TRUE *” for upper/lower case letters.
>>>>> 
>>>> AFAICT the so called “spec" says nothing about trimming white space from 
>>>> values and although not explicitly called out the actual value has to be 
>>>> what constitutes the character sequence for the “otherchar" definition 
>>>> (otherwise the continuation space and newline characters would also be 
>>>> part of the actual value and that makes no sense).
>>> No it doesn’t say you can trim white space, but if one doesn’t, then do we 
>>> accept “true”, “ true”, “  true”, etc.?
>>> 
>>>> So it seems you may have quite a bit of wiggle room here :-) and it’s up 
>>>> to each attribute definition to state what constitutes its domain of 
>>>> possible valid values based on “other char”.
>>> So, we can say *otherchar is upper/lower case “true” only?
>>> 
>>>> 
>>>>>> For example, the Sealed attribute can take on a value of “true” or 
>>>>>> “false” (case ignored), but there is no white space trimming.
>>>>> Well then “Sealed:   true” won’t work, but the spec says it should work.
>>>>> 
>>>> Where does the “spec” state that it should?
>>> It really comes down to the interpretation of otherchar.  If it can be 
>>> interpreted on an attribute by attribute basis then it’s not a problem.
>>> 
>>>> The value is literally the string “ true”, at least in one interpretation 
>>>> of the “spec” :-)
>>> I’m fine with that.  And we really should do something about the spec, it’s 
>>> too loose and ambiguous.
>>> 
>>>> I suspect it’s ok to go with finding “Multi-Release: true” (lower/upper 
>>>> case) as in your first patch, then check if it is terminated with a 
>>>> newline (and ignore the continuation case)
>>> Okay.  I’d like Claes to weigh in because he’s the one who brought it up.  
>>> He’s traveling today, so I don’t expect to hear from him soon.
>> 
>> Yeah, I'm guilty of raising the silly question of whether or not accepting 
>> untrimmed true is OK according to spec.
>> 
>> With a generous interpretation and the presence of prior art (Sealed) I 
>> think it's OK to go back to the first version of the patch (with the 
>> addition to check that Multi-Release: true is followed by a LF, CR or the 
>> end of the manifest) and add a similar note to the spec/notes that 
>> Multi-Release has the same value restrictions as Sealed.
>> 
>> Perhaps both cases should be clarified in the notes to say that 
>> leading/trailing whitespace is not accepted, since that isn't directly 
>> obvious to a casual reader.
>> 
>> Thanks!
>> 
>> /Claes
>> 
>>> 
>>>> Paul.
>>>> 
>>>> 
>>>>> I am fine without doing this nonsense, but I think we need to change the 
>>>>> spec to make it correct.  We could change the definition of "otherchar” 
>>>>> to something like this
>>>>> 
>>>>> otherchar:=ALPHA EXTALPHANUM*
>>>>> ALPHA:=[A-Z | a-z | _]
>>>>> EXTALPHANUM:=[ALPHA | 0-9 | SPACE]
>>>>> 
>>>>> Even this will still allow trailing spaces, so after matching TRUE we’d 
>>>>> need to make sure the next char is SPACE, \r, or \n.
>>>>> 
>>>>> Other ideas?
>>>>> 
>>>>>> Paul.
>>>>>> 
>>>>>>>> Hi,
>>>>>>>> Please review this simple fix to require that the jar manifest 
>>>>>>>> Multi-Release attribute has a value of “true" in order to be 
>>>>>>>> effective, i.e. to assert the jar is a multi-release jar.
>>>>>>>> issue: https://bugs.openjdk.java.net/browse/JDK-8153213 
>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8153213>
>>>>>>>> webrev: http://cr.openjdk.java.net/~sdrach/8153213/webrev/index.html 
>>>>>>>> <http://cr.openjdk.java.net/~sdrach/8153213/webrev/index.html>
>>>>>>>> Thanks
>>>>>>>> Steve
> 

Re: RFR: 8153213: Jar manifest attribute "Multi-Release" accepts any value

[Steve Drach]

Okay, I’ll prepare a new webrev.  I think all we need to check for after “true” 
is \r or \n.  If the manifest just ends without at least one of those, it’s not 
a legal manifest.

> On Apr 8, 2016, at 11:25 AM, Claes Redestad  wrote:
> 
> On 04/08/2016 07:54 PM, Steve Drach wrote:
>>>>>> I’ve put up a new webrev that addresses the issue of having spaces 
>>>>>> before (and after) the value “true” in the Multi-Release attribute.
>>>>>> 
>>>>> Is some or all of that really necessary? since the we can specify domain 
>>>>> of values.
>>>> I think it is.  The spec states that one can have an arbitrary amount of 
>>>> leading/trailing spaces in the value part of the attribute.  Apparently 
>>>> attribute values could also have “hidden” characters such as vertical tab 
>>>> or nak for example.  I wish the spec was tighter here.  I’m merely stating 
>>>> the the domain can be
>>>> “ *TRUE *” for upper/lower case letters.
>>>> 
>>> AFAICT the so called “spec" says nothing about trimming white space from 
>>> values and although not explicitly called out the actual value has to be 
>>> what constitutes the character sequence for the “otherchar" definition 
>>> (otherwise the continuation space and newline characters would also be part 
>>> of the actual value and that makes no sense).
>> No it doesn’t say you can trim white space, but if one doesn’t, then do we 
>> accept “true”, “ true”, “  true”, etc.?
>> 
>>> So it seems you may have quite a bit of wiggle room here :-) and it’s up to 
>>> each attribute definition to state what constitutes its domain of possible 
>>> valid values based on “other char”.
>> So, we can say *otherchar is upper/lower case “true” only?
>> 
>>> 
>>>>> For example, the Sealed attribute can take on a value of “true” or 
>>>>> “false” (case ignored), but there is no white space trimming.
>>>> Well then “Sealed:   true” won’t work, but the spec says it should work.
>>>> 
>>> Where does the “spec” state that it should?
>> It really comes down to the interpretation of otherchar.  If it can be 
>> interpreted on an attribute by attribute basis then it’s not a problem.
>> 
>>> The value is literally the string “ true”, at least in one interpretation 
>>> of the “spec” :-)
>> I’m fine with that.  And we really should do something about the spec, it’s 
>> too loose and ambiguous.
>> 
>>> I suspect it’s ok to go with finding “Multi-Release: true” (lower/upper 
>>> case) as in your first patch, then check if it is terminated with a newline 
>>> (and ignore the continuation case)
>> Okay.  I’d like Claes to weigh in because he’s the one who brought it up.  
>> He’s traveling today, so I don’t expect to hear from him soon.
> 
> Yeah, I'm guilty of raising the silly question of whether or not accepting 
> untrimmed true is OK according to spec.
> 
> With a generous interpretation and the presence of prior art (Sealed) I think 
> it's OK to go back to the first version of the patch (with the addition to 
> check that Multi-Release: true is followed by a LF, CR or the end of the 
> manifest) and add a similar note to the spec/notes that Multi-Release has the 
> same value restrictions as Sealed.
> 
> Perhaps both cases should be clarified in the notes to say that 
> leading/trailing whitespace is not accepted, since that isn't directly 
> obvious to a casual reader.
> 
> Thanks!
> 
> /Claes
> 
>> 
>>> Paul.
>>> 
>>> 
>>>> I am fine without doing this nonsense, but I think we need to change the 
>>>> spec to make it correct.  We could change the definition of "otherchar” to 
>>>> something like this
>>>> 
>>>> otherchar:=ALPHA EXTALPHANUM*
>>>> ALPHA:=[A-Z | a-z | _]
>>>> EXTALPHANUM:=[ALPHA | 0-9 | SPACE]
>>>> 
>>>> Even this will still allow trailing spaces, so after matching TRUE we’d 
>>>> need to make sure the next char is SPACE, \r, or \n.
>>>> 
>>>> Other ideas?
>>>> 
>>>>> Paul.
>>>>> 
>>>>>>> Hi,
>>>>>>> Please review this simple fix to require that the jar manifest 
>>>>>>> Multi-Release attribute has a value of “true" in order to be effective, 
>>>>>>> i.e. to assert the jar is a multi-release jar.
>>>>>>> issue: https://bugs.openjdk.java.net/browse/JDK-8153213 
>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8153213>
>>>>>>> webrev: http://cr.openjdk.java.net/~sdrach/8153213/webrev/index.html 
>>>>>>> <http://cr.openjdk.java.net/~sdrach/8153213/webrev/index.html>
>>>>>>> Thanks
>>>>>>> Steve

Re: RFR: 8153213: Jar manifest attribute "Multi-Release" accepts any value

[Steve Drach]

 I’ve put up a new webrev that addresses the issue of having spaces before 
 (and after) the value “true” in the Multi-Release attribute.
 
>>> 
>>> Is some or all of that really necessary? since the we can specify domain of 
>>> values.
>> 
>> I think it is.  The spec states that one can have an arbitrary amount of 
>> leading/trailing spaces in the value part of the attribute.  Apparently 
>> attribute values could also have “hidden” characters such as vertical tab or 
>> nak for example.  I wish the spec was tighter here.  I’m merely stating the 
>> the domain can be
>> “ *TRUE *” for upper/lower case letters.
>> 
> 
> AFAICT the so called “spec" says nothing about trimming white space from 
> values and although not explicitly called out the actual value has to be what 
> constitutes the character sequence for the “otherchar" definition (otherwise 
> the continuation space and newline characters would also be part of the 
> actual value and that makes no sense).

No it doesn’t say you can trim white space, but if one doesn’t, then do we 
accept “true”, “ true”, “  true”, etc.?

> 
> So it seems you may have quite a bit of wiggle room here :-) and it’s up to 
> each attribute definition to state what constitutes its domain of possible 
> valid values based on “other char”.

So, we can say *otherchar is upper/lower case “true” only?

> 
> 
>>> 
>>> For example, the Sealed attribute can take on a value of “true” or “false” 
>>> (case ignored), but there is no white space trimming.
>> 
>> Well then “Sealed:   true” won’t work, but the spec says it should work.
>> 
> 
> Where does the “spec” state that it should?

It really comes down to the interpretation of otherchar.  If it can be 
interpreted on an attribute by attribute basis then it’s not a problem.

> 
> The value is literally the string “ true”, at least in one interpretation of 
> the “spec” :-)

I’m fine with that.  And we really should do something about the spec, it’s too 
loose and ambiguous.

> 
> I suspect it’s ok to go with finding “Multi-Release: true” (lower/upper case) 
> as in your first patch, then check if it is terminated with a newline (and 
> ignore the continuation case)

Okay.  I’d like Claes to weigh in because he’s the one who brought it up.  He’s 
traveling today, so I don’t expect to hear from him soon.

> 
> Paul.
> 
> 
>> I am fine without doing this nonsense, but I think we need to change the 
>> spec to make it correct.  We could change the definition of "otherchar” to 
>> something like this
>> 
>> otherchar:=ALPHA EXTALPHANUM*
>> ALPHA:=[A-Z | a-z | _]
>> EXTALPHANUM:=[ALPHA | 0-9 | SPACE]
>> 
>> Even this will still allow trailing spaces, so after matching TRUE we’d need 
>> to make sure the next char is SPACE, \r, or \n.
>> 
>> Other ideas?
>> 
>>> 
>>> Paul.
>>> 
> Hi,
> Please review this simple fix to require that the jar manifest 
> Multi-Release attribute has a value of “true" in order to be effective, 
> i.e. to assert the jar is a multi-release jar.
> issue: https://bugs.openjdk.java.net/browse/JDK-8153213 
> 
> webrev: http://cr.openjdk.java.net/~sdrach/8153213/webrev/index.html 
> 
> Thanks
> Steve
>>> 
>> 
> 

Re: RFR: 8153213: Jar manifest attribute "Multi-Release" accepts any value

[Steve Drach]

>> I’ve put up a new webrev that addresses the issue of having spaces before 
>> (and after) the value “true” in the Multi-Release attribute.
>> 
> 
> Is some or all of that really necessary? since the we can specify domain of 
> values.

I think it is.  The spec states that one can have an arbitrary amount of 
leading/trailing spaces in the value part of the attribute.  Apparently 
attribute values could also have “hidden” characters such as vertical tab or 
nak for example.  I wish the spec was tighter here.  I’m merely stating the the 
domain can be
“ *TRUE *” for upper/lower case letters.

> 
> For example, the Sealed attribute can take on a value of “true” or “false” 
> (case ignored), but there is no white space trimming.

Well then “Sealed:   true” won’t work, but the spec says it should work.

I am fine without doing this nonsense, but I think we need to change the spec 
to make it correct.  We could change the definition of "otherchar” to something 
like this

otherchar:=ALPHA EXTALPHANUM*
ALPHA:=[A-Z | a-z | _]
EXTALPHANUM:=[ALPHA | 0-9 | SPACE]

Even this will still allow trailing spaces, so after matching TRUE we’d need to 
make sure the next char is SPACE, \r, or \n.

Other ideas?

> 
> Paul.
> 
>>> Hi,
>>> Please review this simple fix to require that the jar manifest 
>>> Multi-Release attribute has a value of “true" in order to be effective, 
>>> i.e. to assert the jar is a multi-release jar.
>>> issue: https://bugs.openjdk.java.net/browse/JDK-8153213 
>>> 
>>> webrev: http://cr.openjdk.java.net/~sdrach/8153213/webrev/index.html 
>>> 
>>> Thanks
>>> Steve
> 

RFR: 8153213: Jar manifest attribute "Multi-Release" accepts any value

[Steve Drach]

I’ve put up a new webrev that addresses the issue of having spaces before (and 
after) the value “true” in the Multi-Release attribute.

> Hi,
> Please review this simple fix to require that the jar manifest Multi-Release 
> attribute has a value of “true" in order to be effective, i.e. to assert the 
> jar is a multi-release jar.
> issue: https://bugs.openjdk.java.net/browse/JDK-8153213 
> 
> webrev: http://cr.openjdk.java.net/~sdrach/8153213/webrev/index.html 
> 
> Thanks
> Steve

RFR: 8153213: Jar manifest attribute "Multi-Release" accepts any value

[Steve Drach]

Hi,

Please review this simple fix to require that the jar manifest Multi-Release 
attribute has a value of “true" in order to be effective, i.e. to assert the 
jar is a multi-release jar.

issue: https://bugs.openjdk.java.net/browse/JDK-8153213 

webrev: http://cr.openjdk.java.net/~sdrach/8153213/webrev/index.html 


Thanks
Steve

Re: RFR: 8152733: Avoid creating Manifest when checking for Multi-Release attribute

[Steve Drach]

Hi,

A minor nit on the comment

> + * Since there are no repeated substrings in our search strings,
> + * the good character shifts can be replaced with a comparison.

Probably should be “good suffix shifts”.

Steve

Re: 8151339 Adding fragment to JAR URLs breaks ant

[Steve Drach]

Hi Uwe,

> Thanks for the quick fix!  I am not able to test this on the short term, but 
> I trust you that Lucene builds now. I am a bit nervous, because it does not 
> explain the Ivy issues, but I will try to create some test cases with 
> relative jar:-URL resolving tomorrow. This may help with resolving the 
> problems in build 110.

If you can come up with small, easily reproducible test cases for any errors 
your find, that would help a lot.
> 
> I just want to make sure, that the following also works:
> - Get URL from classloader to a resource file
> - resolve a relative file against this URL and load it by URL
> (this is common pattern for parsing XML resources from JAR files that refer 
> relatively to other resources in same JAR file by href)

Please try everything.

> 
> Keep me informed when build 109 is downloadable.

I’ll try.

Steve

> 
> Uwe
> 
> -
> Uwe Schindler
> uschind...@apache.org 
> ASF Member, Apache Lucene PMC / Committer
> Bremen, Germany
> http://lucene.apache.org/
> 
>> -Original Message-
>> From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On
>> Behalf Of Steve Drach
>> Sent: Monday, March 07, 2016 8:07 PM
>> To: core-libs-dev ; paul Sandoz
>> ; Alan Bateman ;
>> Xueming Shen 
>> Subject: RFR: 8151339 Adding fragment to JAR URLs breaks ant
>> 
>> Hi,
>> 
>> Please review the following changeset.  We’d like to get this into build 109,
>> which means by noon today.  This is essentially a temporary fix, but it’s 
>> been
>> tested and Lucene has been built against it.  We will follow up with a more
>> comprehensive fix by build 110.
>> 
>> webrev: http://cr.openjdk.java.net/~sdrach/8151339/webrev/
>> <http://cr.openjdk.java.net/~sdrach/8151339/webrev/>
>> issue: https://bugs.openjdk.java.net/browse/JDK-8151339
>> <https://bugs.openjdk.java.net/browse/JDK-8151339>
>> 
>> Thanks
>> Steve
>> 
> 
> 

RFR: 8151339 Adding fragment to JAR URLs breaks ant

[Steve Drach]

Hi,

Please review the following changeset.  We’d like to get this into build 109, 
which means by noon today.  This is essentially a temporary fix, but it’s been 
tested and Lucene has been built against it.  We will follow up with a more 
comprehensive fix by build 110.

webrev: http://cr.openjdk.java.net/~sdrach/8151339/webrev/ 

issue: https://bugs.openjdk.java.net/browse/JDK-8151339 


Thanks
Steve

Re: RFR 8150679: closed/javax/crypto/CryptoPermission/CallerIdentification.sh fails after fix for JDK-8132734

[Steve Drach]

> On Mar 3, 2016, at 3:26 AM, Paul Sandoz  wrote:
> 
> 
>> On 2 Mar 2016, at 20:12, Steve Drach  wrote:
>> 
>> Please review the following fix for JDK-8150679
>> 
>> webrev: http://cr.openjdk.java.net/~sdrach/8150679/webrev/ 
>> <http://cr.openjdk.java.net/~sdrach/8150679/webrev/>
>> issue: https://bugs.openjdk.java.net/browse/JDK-8150679 
>> <https://bugs.openjdk.java.net/browse/JDK-8150679>
>> 
>> The test was modified to demonstrate the problem.
> 
> You are essentially bombing out of MR-JAR functionality if the JarEntry is 
> not an instance of JarFileEntry.

If it’s not a JarFileEntry, none of the MR functionality has been invoked.

> That might be ok for a short-term solution, but it might require some further 
> deeper investigation on things that extend JarEntry and how it is is used by 
> VerifierStream [*].
> 
> JarFile:
> 
> 895 private JarEntry verifiableEntry(ZipEntry ze) {
> 896 if (ze == null) return null;
> 
> You don’t need this. The code will anyway throw an NPE elsewhere, and the 
> original code threw an NPE when obtaining the name:

Ok.  I’ll take this out.  Feels a bit uncomfortable though.

> 
>return new JarVerifier.VerifierStream(
>getManifestFromReference(),
>ze instanceof JarFileEntry ?
>(JarEntry) ze : getJarEntry(ze.getName()),
>super.getInputStream(ze),
>jv);
> 
> 
> 897 if (ze instanceof JarFileEntry) {
> 898 // assure the name and entry match for verification
> 899 return ((JarFileEntry)ze).reifiedEntry();
> 900 }
> 901 ze = getJarEntry(ze.getName());
> 902 assert ze instanceof JarEntry;
> 
> This assertion is redundant as the method signature of getJarEntry returns 
> JarEntry.

I know it’s redundant.  It was a statement of fact. but the method signature 
does the same thing.

> 
> 
> 903 if (ze instanceof JarFileEntry) {
> 904 return ((JarFileEntry)ze).reifiedEntry();
> 905 }
> 906 return (JarEntry)ze;
> 907 }
> 
> 
> MultiReleaseJarURLConnection
> —
> 
> Given your changes above i am confused how your test passes for instances of 
> URLJarFileEntry since they cannot be reified.

I suspect that it works for regular jar files but not for MR jar files.  That’s 
another bug in URLJarFile — it gets a versioned entry that can’t be verified.  
I mentioned this yesterday.  I’ll write a test and if warranted submit a bug on 
this.

> 
> Paul.
> 
> [*] AFAICT JarVerifier directly accesses the fields JarEntry.signers/certs.

Yes, but the overriden fields are the ones of interest.  

> 

RFR 8150679: closed/javax/crypto/CryptoPermission/CallerIdentification.sh fails after fix for JDK-8132734

[Steve Drach]

Please review the following fix for JDK-8150679

webrev: http://cr.openjdk.java.net/~sdrach/8150679/webrev/ 

issue: https://bugs.openjdk.java.net/browse/JDK-8150679 


The test was modified to demonstrate the problem.

Re: RFR - 8132734: java.util.jar.* changes to support multi-release jar files

[Steve Drach]

>> Thank you Alan.  I’ll address the issues you bring up before integration.
> Thanks. Are you planning to update the webrev too as it would be nice to see 
> the final javadoc?

http://cr.openjdk.java.net/~sdrach/8132734/webrev.07/index.html 

Re: RFR - 8132734: java.util.jar.* changes to support multi-release jar files

[Steve Drach]

Thank you Alan.  I’ll address the issues you bring up before integration.

> On Feb 15, 2016, at 4:30 AM, Alan Bateman  wrote:
> 
> 
> On 10/02/2016 01:04, Steve Drach wrote:
>> Hi,
>> 
>> Yet another webrev, http://cr.openjdk.java.net/~sdrach/8132734/webrev.06/ 
>> <http://cr.openjdk.java.net/%7Esdrach/8132734/webrev.06/>, with a change to 
>> JarEntryIterator to fix a problem discovered by performance tests — calling 
>> hasNext() twice as often as needed.  I also removed the @since 9 tags from 
>> the methods entries() and stream(), and added an additional sentence to the 
>> spec for each of those methods that clarifies what a base entry is (actually 
>> is not).
>> 
> I went through the latest webrev and it looks quite good.
> 
> A few comments on the javadoc:
> 
> "... partitioned by the major version of Java platform releases" - this might 
> be better as "... partitioned by the major version of the Java platform".
> 
> In JarFile.Release then it uses the phrase "top-most (base) directory". I 
> thought we had purged "top-*" from the javadoc in previous iterations because 
> it hints of classes or resources in the top most directory (which isn't the 
> case with classes in a named package).
> 
> "... will not be accessible by this JarFile" hints of access control or even 
> security manager. Would it clearer to re-word this to something like "will 
> not be located by methods such as getEntry" ?
> 
> "returned depends whether" -> "returned depends on whether".
> 
> In the javadoc for entries() and stream() then it mentions "the constructor" 
> many times. I would be tempted to replace many of these - for example "all 
> entries are returned, regardless of the constructor" might be better as "all 
> entries of returned, regards of how the JarFile is created".
> 
> A couple of nits on the implementation:
> 
> vze = JarFile.super.getEntry(META_INF_VERSIONS + i-- + sname);
> - it would be more readable if you move the decrement to its own line. Also I 
> assume that JarFile is not needed here.
> 
> L942-943 looks messy too, I assume that can be cleaned up.
> 
> JarFileFactory - "earl" will confuse readers, needs a comment or a better 
> name.
> 
> I think this is all that I have for now.
> 
> -Alan.

Re: RFR - 8132734: java.util.jar.* changes to support multi-release jar files

[Steve Drach]

Sherman,

I like your suggestions and will open an issue to work on the performance after 
integration.  I’ll fix the minor “bug” you pointed out at the same time.  

Steve

> On Feb 12, 2016, at 5:19 PM, Xueming Shen  wrote:
> 
> 
> Steve,
> 
> I would assume the difference of the webrev.04 "old" iterator and the 
> webrev.06 "new" iterator
> that might trigger a performance is how you create the JarFileEntry. The one 
> parameter constructor
> invokes isMultiRelease(), which might be relatively expensive, when the "mr" 
> is enabled. There
> are couple "if" checks involved. 
> 
> For the "new" iterator is slower than the current jdk9 one. It might be 
> desired to have two JarEntryIterator
> classes defined, one for "notVersioned", one for the "versioned". Of course 
> keep the two parameter
> constructor for the "notVersioned". This might bring performance back for the 
> normal "notVersioned"
> usage, which I would assume is the majority.
> 
> There is also a "bug" in the new iterator. The iterator/Enumeration returned 
> from ZipFile.entries()
> checks "ensureOpen() in both hasNext() and next(). So close() the ZipFile 
> after itr.hasNext() will
> fails the next next() invocation in the old implementation. The latest itr 
> will returns the cached 
> "ze". Not a big issue for sure, but a kind of "regression". Defining two 
> iterator classes as suggested
> above will also workaround this issue, as the "notVersioned" one will work 
> just as expected, no
> regression.
> 
> -Sherman
> 
> On 2/9/16 5:04 PM, Steve Drach wrote:
>> Hi,
>> 
>> Yet another webrev, http://cr.openjdk.java.net/~sdrach/8132734/webrev.06/ 
>> <http://cr.openjdk.java.net/%7Esdrach/8132734/webrev.06/>, with a change to 
>> JarEntryIterator to fix a problem discovered by performance tests — calling 
>> hasNext() twice as often as needed.  I also removed the @since 9 tags from 
>> the methods entries() and stream(), and added an additional sentence to the 
>> spec for each of those methods that clarifies what a base entry is (actually 
>> is not).
>> 
>>> I think having stream and entries do this is right although I would like to 
>>> see some performance data if possible.
>> 
>> See http://cr.openjdk.java.net/~sdrach/8132734/JarFile%20Performance.pdf 
>> <http://cr.openjdk.java.net/%7Esdrach/8132734/JarFile%20Performance.pdf>
>> 
>> I used JMH to run the benchmark.  See 
>> http://cr.openjdk.java.net/~sdrach/8132734/MyBenchmark.java 
>> <http://cr.openjdk.java.net/%7Esdrach/8132734/MyBenchmark.java>.  I used two 
>> jar files, the rt.jar file from JDK 8 that has 20653 entries and the 
>> multi-release.jar found in the test directory with 14 entries.  Obviously 
>> rt.jar is not a multi-release jar file.
>> 
>> The first two tables (1 and 2) are comparable and the second two tables are 
>> somewhat comparable (3 and 4).
>> 
>> Tables 1 and 2 have 4 sections that show the results of tests on the two jar 
>> files in 4 configurations of JarFile.  The tests were done with a JarFile 
>> object constructed without the Release object argument, essentially the 
>> legacy constructor.  The section labeled "JDK 8 JarFile” was done with JDK 
>> 8u66.  The section labeled “JDK 9 JarFile” was done with the latest build of 
>> openjdk/jdk9/dev without any changes in my 8132734 changeset.  I chose this 
>> section as the reference, so the last column shows the values normalized to 
>> 1 micro/milli second per operation (rt.jar times are in milliseconds and 
>> multi-release.jar times are in microseconds).  It should be obvious that JDK 
>> 9 is much faster than JDK 8, somewhere on the order of 5-6 times faster.  I 
>> think that is because ZipFile doesn’t use JNI in JDK 9.
>> 
>> Of the two remaining sections in Tables 1 and 2, the section labeled 
>> “MultiRelease JarFile” differs from the section labeled “MultiRelease 
>> JarFile, new iterator” only in the JarEntryIterator class.  The first one 
>> uses the original iterator in JarFile.java that can be seen starting with 
>> line 551 of webrev.04 
>> <http://cr.openjdk.java.net/%7Esdrach/8132734/webrev.04/>, and the new 
>> iterator starts with line 553 of webrev.06 
>> <http://cr.openjdk.java.net/%7Esdrach/8132734/webrev.06/>.  The results are 
>> strange.  The new, more complex, iterator appears to be faster than the old, 
>> simpler, iterator.  I double, and triple, checked it, but it was always 
>> faster.  I used jitwa

Re: RFR JDK-8149769: Null pointer exception in ZipFileSystemProvider

[Steve Drach]

> What's the issue? The bug description only includes the fix. If the env is 
> null, shouldn't
> it trigger a NPE?
> 
> The java.nio.file.spi package does have the note that "NPE, unless otherwise 
> noted ..."
> The api for FilesystemProvider.newFileSystem(..., env) says "env" can be 
> empty, means
> NPE for "null".
> 
> Maybe I miss something here?

No, you are not missing anything, I was.  I see ZipFileSystem will throw the 
NPE instead of ZipFileSystemProvider if my “fix” goes in, so it’ll still 
happen.  I rescind my request and will close the bug report with appropriate 
comment.


> 
> -Sherman
> 
> On 2/12/16 1:11 PM, Steve Drach wrote:
>> Hi,
>> 
>> Please review this simple fix to ZipFileSystemProvider.  The issue is 
>> JDK-8149769 <https://bugs.openjdk.java.net/browse/JDK-8149769>.  I didn’t do 
>> a webrev but instead provide the following patch.
>> 
>> Thanks
>> Steve
>> 
>> diff -r 2d6c2c75f338 
>> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystemProvider.java
>> --- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystemProvider.java   
>> Tue Feb 09 14:07:28 2016 -0800
>> +++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystemProvider.java   
>> Fri Feb 12 12:59:46 2016 -0800
>> @@ -100,7 +100,7 @@
>>  }
>>  ZipFileSystem zipfs = null;
>>  try {
>> -if (env.containsKey("multi-release")) {
>> +if (env != null && env.containsKey("multi-release")) {
>>  zipfs = new JarFileSystem(this, path, env);
>>  } else {
>>  zipfs = new ZipFileSystem(this, path, env);
> 

Re: RFR JDK-8149769: Null pointer exception in ZipFileSystemProvider

[Steve Drach]

>> Please review this simple fix to ZipFileSystemProvider.  The issue is 
>> JDK-8149769 .  I didn’t do 
>> a webrev but instead provide the following patch.
>> 
>> 
> This looks okay. Can one of the existing tests be updated to cover this case?

Yes, almost any of them can be updated.  There is one, ZipFSTester.java that 
seems to be associated with a lot of bugs, so maybe that one.  Or I can put it 
in MultiReleaseJarTest, which makes sense because the code it tests was the 
code that caused the bug.  Any preferences?

RFR JDK-8149769: Null pointer exception in ZipFileSystemProvider

[Steve Drach]

Hi,

Please review this simple fix to ZipFileSystemProvider.  The issue is 
JDK-8149769 .  I didn’t do a 
webrev but instead provide the following patch.

Thanks
Steve

diff -r 2d6c2c75f338 
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystemProvider.java
--- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystemProvider.java  
Tue Feb 09 14:07:28 2016 -0800
+++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystemProvider.java  
Fri Feb 12 12:59:46 2016 -0800
@@ -100,7 +100,7 @@
 }
 ZipFileSystem zipfs = null;
 try {
-if (env.containsKey("multi-release")) {
+if (env != null && env.containsKey("multi-release")) {
 zipfs = new JarFileSystem(this, path, env);
 } else {
 zipfs = new ZipFileSystem(this, path, env);

Re: RFR - 8132734: java.util.jar.* changes to support multi-release jar files

[Steve Drach]

Hi,

Yet another webrev, http://cr.openjdk.java.net/~sdrach/8132734/webrev.06/ 
, with a change to 
JarEntryIterator to fix a problem discovered by performance tests — calling 
hasNext() twice as often as needed.  I also removed the @since 9 tags from the 
methods entries() and stream(), and added an additional sentence to the spec 
for each of those methods that clarifies what a base entry is (actually is not).

> I think having stream and entries do this is right although I would like to 
> see some performance data if possible.

See http://cr.openjdk.java.net/~sdrach/8132734/JarFile%20Performance.pdf 


I used JMH to run the benchmark.  See 
http://cr.openjdk.java.net/~sdrach/8132734/MyBenchmark.java 
.  I used two jar 
files, the rt.jar file from JDK 8 that has 20653 entries and the 
multi-release.jar found in the test directory with 14 entries.  Obviously 
rt.jar is not a multi-release jar file.

The first two tables (1 and 2) are comparable and the second two tables are 
somewhat comparable (3 and 4).

Tables 1 and 2 have 4 sections that show the results of tests on the two jar 
files in 4 configurations of JarFile.  The tests were done with a JarFile 
object constructed without the Release object argument, essentially the legacy 
constructor.  The section labeled "JDK 8 JarFile” was done with JDK 8u66.  The 
section labeled “JDK 9 JarFile” was done with the latest build of 
openjdk/jdk9/dev without any changes in my 8132734 changeset.  I chose this 
section as the reference, so the last column shows the values normalized to 1 
micro/milli second per operation (rt.jar times are in milliseconds and 
multi-release.jar times are in microseconds).  It should be obvious that JDK 9 
is much faster than JDK 8, somewhere on the order of 5-6 times faster.  I think 
that is because ZipFile doesn’t use JNI in JDK 9.

Of the two remaining sections in Tables 1 and 2, the section labeled 
“MultiRelease JarFile” differs from the section labeled “MultiRelease JarFile, 
new iterator” only in the JarEntryIterator class.  The first one uses the 
original iterator in JarFile.java that can be seen starting with line 551 of 
webrev.04 , and the new 
iterator starts with line 553 of webrev.06 
.  The results are 
strange.  The new, more complex, iterator appears to be faster than the old, 
simpler, iterator.  I double, and triple, checked it, but it was always faster. 
 I used jitwatch to look at the hotspot logs generated during compilation and 
neither method was compiled.  I suppose I could dig into it further but decided 
not to.  Consider it good news.  The results do show that the multi-release 
enhancement slows JarFile entries/stream down by 2-18% depending on the size of 
the jar file.  But they are still much better than the JDK 8 values.

> Also I would expect that if a JAR file is not mult-release but the library 
> opens it with Release.RUNTIME to perform the same as opening the JAR file 
> with the Release-less constructors.

The results in Table 3 attempts to answer this question since rt.jar is not a 
multi-release jar file.  This tells me that if one opens the JarFile with 
Release.RUNTIME, that there is a performance penalty of 2-6% on this very large 
jar file.

Finally, the results in Table 4 tell me that processing a true multi-release 
jar file takes about 80% more time per entries() or stream() operation.  I’ve 
looked at this in a profiler and there is no particular area that stands out to 
me, it’s just more complicated to process a multi-release jar file, as would be 
expected.

> I think the javadoc will need to also need to make it clear whether entries 
> with names starting with META-INF/versions/ are returned.

Fixed.

> 
> I see you've added @since 9 to the existing methods, I assume you didn't mean 
> to do this.

Fixed.

> 
> At some point then we need to discuss how RUNTIME_VERSION is computed. Iris 
> (via Mandy) has pushed jdk.Version to jdk9/dev but having it exported by 
> java.base conflicts with the design principles in JEP 200. Moving it to 
> another module means that code in java.base cannot use it and thus the JAR 
> file can't use it.

Left as is.

Re: RFR - 8132734: java.util.jar.* changes to support multi-release jar files

[Steve Drach]

Thanks for the comments Alan.  Responses in-line.

>> I created a new webrev, 
>> http://cr.openjdk.java.net/~sdrach/8132734/webrev.05/ 
>> , that implements 
>> what I outlined above.  In particular I enhanced the JarEntryIterator class 
>> and I added additional commentary to the entries() and stream() methods.  I 
>> also added a new test, MultiReleaseJarIterators, to test entries() and 
>> stream().
>> 
> I think having stream and entries do this is right although I would like to 
> see some performance data if possible.

I’ll see what I can do.  I suspect the non-multi-release jar will be very 
comparable since there’s just a couple boolean tests that were added for this 
case.

> Also I would expect that if a JAR file is not mult-release but the library 
> opens it with Release.RUNTIME to perform the same as opening the JAR file 
> with the Release-less constructors.

Perhaps.  There is a slightly different path with an additional method call and 
boolean test in this case, but I’ll try to get some metrics here too.

> 
> I think the javadoc will need to also need to make it clear whether entries 
> with names starting with META-INF/versions/ are returned.

It was a bit difficult to explain in a succinct way, but the careful reader 
should be able to infer that the META-INF/versions/ entries are not returned 
when the constructor with the Release argument is invoked.  I’ll try to add 
some additional detail.

> 
> I see you've added @since 9 to the existing methods, I assume you didn't mean 
> to do this.

I did mean to do it, but now that you mention it, I see it was a mistake.  I’ll 
fix that.

> 
> At some point then we need to discuss how RUNTIME_VERSION is computed. Iris 
> (via Mandy) has pushed jdk.Version to jdk9/dev but having it exported by 
> java.base conflicts with the design principles in JEP 200. Moving it to 
> another module means that code in java.base cannot use it and thus the JAR 
> file can't use it.

I guess I need to wait until that settles down a bit.

Re: RFR - 8132734: java.util.jar.* changes to support multi-release jar files

[Steve Drach]

> I think the following is a reasonable solution.
> 
> If the JarFile is constructed with any of the 5 constructors that do not 
> contain a Release argument, then entries()/stream() returns the set of all 
> entries in the jar file including those under the 
> META-INF/versions/directory.  The base entries are “raw”, they are not 
> aliases for versioned entries.
> 
> If the JarFile is constructed with the 1 constructor that does include a 
> Release argument, then entries()/stream() returns the set of appropriately 
> versioned entries that would result from invoking getEntry()/getJarEntry() 
> with the name of each base entry.  The entries in the tree rooted at the 
> directory META-INF/versions/ are not returned.

I created a new webrev, http://cr.openjdk.java.net/~sdrach/8132734/webrev.05/ 
<http://cr.openjdk.java.net/~sdrach/8132734/webrev.05/>, that implements what I 
outlined above.  In particular I enhanced the JarEntryIterator class and I 
added additional commentary to the entries() and stream() methods.  I also 
added a new test, MultiReleaseJarIterators, to test entries() and stream().

 

> 
>> On Feb 1, 2016, at 12:29 PM, Steve Drach  wrote:
>> 
>> I’m sorry, I didn’t look at the code close enough before I started talking 
>> ;-)  Right now entries()/stream() returns all entries and if the JarFile is 
>> constructed with a Release object != Release.BASE, the base entries that are 
>> returned are the versioned entries.  I think this behavior is a bit 
>> confusing and we should just return all entries without regard to 
>> versioning.  Then create the two new methods for specific versioned entries.
>> 
>>> On Feb 1, 2016, at 12:18 PM, Steve Drach  wrote:
>>> 
>>>>>>> Alan’s point is that traversing using entries()/stream() always returns 
>>>>>>> the versioned entries (if any) rather than all entries, thus in a sense 
>>>>>>> filters.
>>>>>>> 
>>>>>>> My assumption was the traversal should by default be consistent with a 
>>>>>>> calls to getEntry, thus:
>>>>>>> 
>>>>>>> jarFile.stream().forEach(e ->  {
>>>>>>>  JarEntry je = jarFile.getJarEntry(e.getName());
>>>>>>>  assert e.equals(je);
>>>>>>> });
>>>>>>> 
>>>>>>> There might need to be another stream method that returns all entries.
>>>>>>> 
>>>>>> Right, I'm mostly just wondering if entries()/streams() should override 
>>>>>> the entries in the stream with versioned entries and filter out the 
>>>>>> META-INF/versions/ tree.
>>>>> I don’t think so.  That kind of behavior might be difficult to 
>>>>> understand.  Returning all the entries provides some flexibility.  One 
>>>>> can write code like this:
>>>>> 
>>>>> jarfile.stream().map(JarEntry::getName).filter(s ->  
>>>>> !s.startsWith(“META-INF”)).map(JarFile::getJarEntry).etc
>>>>> 
>>>>> to get the versioned results for any version you specify when 
>>>>> constructing the JarFile.
>>>> 
>>>> The current specification treats those class files under meta-inf/releases 
>>>> like
>>>> kind of "metadata" of those base entries. Ideally those files should not 
>>>> even
>>>> be individual "files", but part of their corresponding entries. The 
>>>> consumer of
>>>> the MR-Jar should not need to be aware of these version-ed entries at all 
>>>> to use
>>>> this MR-jar file to load classes/resources. From this point of view, these 
>>>> entries
>>>> probably should be "invisible" from entries()/stream(), when the jar file 
>>>> is opened
>>>> with "version-enabled". And all returned entries should have all their 
>>>> "data"
>>>> (size, csize, timestamps, crc ...) pointed to the corresponding version-ed 
>>>> entries,
>>>> withe the only exception is the "name".
>>>> 
>>>> On the other hand it might be desired to keep JarFile.entries()/stream() 
>>>> asis to
>>>> match their "zip file" perspective, to return "all" raw entries. Then it 
>>>> might also
>>>> be desired to have an alternative "versioned streamVersion()" …
>>> 
>>> It seems to that we have two reasonable a