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

[Steve Drach]

>>> I’ve put out another webrev, 
>>> http://cr.openjdk.java.net/~sdrach/8156499/webrev.05/, that addresses 
>>> Mandy’s concerns.  In particular I demonstrate that the resultant image is 
>>> “runnable” and that a Main class in the image can/cannot find the 
>>> java.logging module when the module-info.class is changed to require 
>>> java.logging in one case but not in the other.
>> 
>> Looks okay in general.  The jlink change looks correct.  Thanks for updating 
>> the test.
> 
> In fact - does this test run on windows?
> 
> 239 Path java = Paths.get(image, "bin", "java”);
> 
> On windows, shouldn’t this need to be “java.exe”?

It turns out it runs just fine on the jprt windows test.

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

[Steve Drach]

>>> I’ve put out another webrev, 
>>> http://cr.openjdk.java.net/~sdrach/8156499/webrev.05/, that addresses 
>>> Mandy’s concerns.  In particular I demonstrate that the resultant image is 
>>> “runnable” and that a Main class in the image can/cannot find the 
>>> java.logging module when the module-info.class is changed to require 
>>> java.logging in one case but not in the other.
>> 
>> Looks okay in general.  The jlink change looks correct.  Thanks for updating 
>> the test.
> 
> In fact - does this test run on windows?
> 
> 239 Path java = Paths.get(image, "bin", "java”);
> 
> On windows, shouldn’t this need to be “java.exe”?

I haven’t run it on jprt yet, but I’ll note that 
tools/jlink/plugins/SystemModuleDescriptors/UserModuleTest does the same thing 
(finds java, not java.exe) and has apparently been run on windows many times.  
If jprt turns up an error here, I’ll fix it.

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

[Steve Drach]

I’ve put out another webrev, 
http://cr.openjdk.java.net/~sdrach/8156499/webrev.05/ 
<http://cr.openjdk.java.net/~sdrach/8156499/webrev.05/>, that addresses Mandy’s 
concerns.  In particular I demonstrate that the resultant image is “runnable” 
and that a Main class in the image can/cannot find the java.logging module when 
the module-info.class is changed to require java.logging in one case but not in 
the other.

> On Oct 20, 2016, at 12:02 AM, Alan Bateman  wrote:
> 
> 
> 
> On 19/10/2016 23:35, Steve Drach wrote:
>> :
>> 
>> Argh!  Sorry about that.  Here’s the correct link 
>> http://cr.openjdk.java.net/~sdrach/8156499/webrev.04/ 
>> <http://cr.openjdk.java.net/%7Esdrach/8156499/webrev.04/>
>> 
>> 
> Thanks for the updates, the changes to jlink look correct in this version.
> 
> -Alan.
> 

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

[Steve Drach]

> On Oct 19, 2016, at 1:59 PM, Steve Drach  wrote:
> 
>>>> In JarArchive::entries then you filter out META-INF/MANIFEST.MF and I'm 
>>>> not sure that that is right (think modular JAR on the module path with a 
>>>> manifest, it's just a resource file).
>>> I took that out and it still works as expected.
>>> 
>>>> Are directories the only case where toEntry can return null, in which case 
>>>> would it simpler to filter out directories here.
>>> I’ve done that too.
>>> 
>>>> There is quite a bit of clean-up needed in this area (pre-dates your patch 
>>>> of course). Not clear why Archive isn't a Closeable for example, or why 
>>>> covariant returns aren't used by the more specialized 
>>>> JarArchive/JarEntry/etc. I'm sure you don't want to get into that but 
>>>> maybe we could at least make the JarFile available via a protected method 
>>>> rather than a field.
>>> I’ve made JarFile available via a protected method
>> This sounds good. Do you have an updated patch? (the current patch is 
>> webrev.03 and I couldn't find a webrev.04).
>> 
>> -Alan.
> 
> http://cr.openjdk.java.net/~sdrach/8164805/webrev.00/ 
> <http://cr.openjdk.java.net/~sdrach/8164805/webrev.00/>
Argh!  Sorry about that.  Here’s the correct link 
http://cr.openjdk.java.net/~sdrach/8156499/webrev.04/ 
<http://cr.openjdk.java.net/~sdrach/8156499/webrev.04/>

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

[Steve Drach]

>>> In JarArchive::entries then you filter out META-INF/MANIFEST.MF and I'm not 
>>> sure that that is right (think modular JAR on the module path with a 
>>> manifest, it's just a resource file).
>> I took that out and it still works as expected.
>> 
>>> Are directories the only case where toEntry can return null, in which case 
>>> would it simpler to filter out directories here.
>> I’ve done that too.
>> 
>>> There is quite a bit of clean-up needed in this area (pre-dates your patch 
>>> of course). Not clear why Archive isn't a Closeable for example, or why 
>>> covariant returns aren't used by the more specialized 
>>> JarArchive/JarEntry/etc. I'm sure you don't want to get into that but maybe 
>>> we could at least make the JarFile available via a protected method rather 
>>> than a field.
>> I’ve made JarFile available via a protected method
> This sounds good. Do you have an updated patch? (the current patch is 
> webrev.03 and I couldn't find a webrev.04).
> 
> -Alan.

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

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

[Steve Drach]

>>> In JarArchive::entries then you filter out META-INF/MANIFEST.MF and I'm not 
>>> sure that that is right (think modular JAR on the module path with a 
>>> manifest, it's just a resource file).
>> I took that out and it still works as expected.
>> 
>>> Are directories the only case where toEntry can return null, in which case 
>>> would it simpler to filter out directories here.
>> I’ve done that too.
>> 
>>> There is quite a bit of clean-up needed in this area (pre-dates your patch 
>>> of course). Not clear why Archive isn't a Closeable for example, or why 
>>> covariant returns aren't used by the more specialized 
>>> JarArchive/JarEntry/etc. I'm sure you don't want to get into that but maybe 
>>> we could at least make the JarFile available via a protected method rather 
>>> than a field.
>> I’ve made JarFile available via a protected method
> This sounds good. Do you have an updated patch? (the current patch is 
> webrev.03 and I couldn't find a webrev.04).
> 
> -Alan.

I’m going to update the test to use the new ToolProvider SPI, then I’ll put a 
new webrev out today.

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

[Steve Drach]

>> Hi,
>> 
>> Please review the latest webrev for this issue.
>> 
>> issue: https://bugs.openjdk.java.net/browse/JDK-8156499 
>> 
>> webrev: http://cr.openjdk.java.net/~sdrach/8156499/webrev.03/ 
>> 
>> 
>> I believe this changeset resolves all the issues Alan and Mandy had with the 
>> previous changesets.  They are:
>> 
>> 1. the test silently passes if java.base.jmod can not be found on the 
>> module-path
>> 2. the test assures that
>> a. the correct module-info class is in the module image
>> b. the correct resource file is in the module image
>> c. the correct class file is in the module image
>> 3. As before, the image module created is for the Runtime.Version of jlink.  
>> This demonstrates that MMR jar files
>> are handled correctly, but will have to be updated to create an image 
>> based on the Runtime.Version of java.base
>> when the appropriate code is migrated from jake.
>> 
> In JarArchive::entries then you filter out META-INF/MANIFEST.MF and I'm not 
> sure that that is right (think modular JAR on the module path with a 
> manifest, it's just a resource file).

I took that out and it still works as expected.

> Are directories the only case where toEntry can return null, in which case 
> would it simpler to filter out directories here.

I’ve done that too.

> 
> There is quite a bit of clean-up needed in this area (pre-dates your patch of 
> course). Not clear why Archive isn't a Closeable for example, or why 
> covariant returns aren't used by the more specialized 
> JarArchive/JarEntry/etc. I'm sure you don't want to get into that but maybe 
> we could at least make the JarFile available via a protected method rather 
> than a field.

I’ve made JarFile available via a protected method

> 
> I don't have time just now to go through the test in detail but having it 
> pass silently when on exploded build (no packaged modules) looks right.
> 
> -Alan

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

[Steve Drach]

Hi,

Please review the latest webrev for this issue.

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

webrev: http://cr.openjdk.java.net/~sdrach/8156499/webrev.03/ 


I believe this changeset resolves all the issues Alan and Mandy had with the 
previous changesets.  They are:

1. the test silently passes if java.base.jmod can not be found on the 
module-path
2. the test assures that
a. the correct module-info class is in the module image
b. the correct resource file is in the module image
c. the correct class file is in the module image
3. As before, the image module created is for the Runtime.Version of jlink.  
This demonstrates that MMR jar files
are handled correctly, but will have to be updated to create an image based 
on the Runtime.Version of java.base
when the appropriate code is migrated from jake.

Thanks,
Steve

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

[Steve Drach]

 Er, I thought the plan was for the set of concealed packages to be the 
 same. It's okay for the ConcealedPackages in the base section to include 
 "empty" packages.
>>> 
>>> I wasn’t involved with that decision.  Chris wrote that code, perhaps he 
>>> can comment.
>> 
>> This surprises me, as I have the same recollection as Alan; no additional
>> concealed packages are allowable in the versioned section.
>> 
>> I just checked the jar tool, and it does NOT add any versioned specific
>> concealed packages.
> 
> 
> If a JAR file contains an empty directory, should `q` be considered as a 
> concealed package in the base section?
> 
> $ jar -t --file hi.jar
> META-INF/
> META-INF/MANIFEST.MF
> module-info.class
> p/Hi.class
> q/
> 
> currently not:
> 
> $ jar -d --file mr.jar
> hi
>  requires mandated java.base
>  conceals p

Is a jar tool that takes a -d option lurking somewhere?  The latest one I have 
does not have that option.

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

[Steve Drach]

 It exists purely because another class in the same versioned directory 
 depends on it.  If we are creating a versionedStream for the version that 
 the non-public class is in, it will be in finalNames, otherwise it won’t 
 be.  I believe the code is correct here.
 
 New concealed packages can be added in a versioned section of the jar file 
 created by jar tool.  Should classes in concealed packages be added to 
 finalNames or not?  Or stated differently, for jlink, should a 
 versionedStream contain entries in concealed packages?
 
>>> Er, I thought the plan was for the set of concealed packages to be the 
>>> same. It's okay for the ConcealedPackages in the base section to include 
>>> "empty" packages.
>> 
>> I wasn’t involved with that decision.  Chris wrote that code, perhaps he can 
>> comment.
> 
> This surprises me, as I have the same recollection as Alan; no additional
> concealed packages are allowable in the versioned section.
> 
> I just checked the jar tool, and it does NOT add any versioned specific
> concealed packages.

You’re right.  Apparently when I read the code this morning, I hadn’t had 
enough coffee yet ;-)

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

[Steve Drach]

>> If there is a class in a versioned directory that is not in the base 
>> directory, then that class must not be public or protected.  It’s not part 
>> of the public interface of the multi-release jar.
> It's also a modular JAR and so if the class is not in an exported package 
> then it's okay for it to be public.

So, if a class is not in an exported package, it’s in a concealed package, is 
that right?  So, should any classes in a concealed package, public or not, be 
included in the versionedStream used in jlink?

> 
> 
>> It exists purely because another class in the same versioned directory 
>> depends on it.  If we are creating a versionedStream for the version that 
>> the non-public class is in, it will be in finalNames, otherwise it won’t be. 
>>  I believe the code is correct here.
>> 
>> New concealed packages can be added in a versioned section of the jar file 
>> created by jar tool.  Should classes in concealed packages be added to 
>> finalNames or not?  Or stated differently, for jlink, should a 
>> versionedStream contain entries in concealed packages?
>> 
> Er, I thought the plan was for the set of concealed packages to be the same. 
> It's okay for the ConcealedPackages in the base section to include "empty" 
> packages.

I wasn’t involved with that decision.  Chris wrote that code, perhaps he can 
comment.

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

[Steve Drach]

>> I didn’t have it right ;-)  It turns out a JarFile stream of versioned 
>> entries was more interesting than I initially thought.  Here’s another 
>> webrev.  It’s not clear to me if I should include the change to JarFile in 
>> this changeset or if it should be in a stand alone changeset.  Advice 
>> appreciated.
>> 
>> webrev: http://cr.openjdk.java.net/~sdrach/8156499/webrev.02/ 
>> 
> 
> 
> I think it’s time to enumerate several test cases of a multi-release Jar 
> content, e.g. a MRJAR with a new concealed package in versioned entries but 
> not in the base entry (I believe that’s allowed?? version 9 & 10 entries that 
> makes sure versioned 10 entries are skipped.  It should also check the 
> module-info.class with different requires and ensure that the image jlink 
> created contains the module-info.class of the right version.  This would help 
> the review of this change.

Okay, I can do that.

> 
> 
> I think ModularJarArchive is the right place to add the versioned entries 
> support.

Okay.

>  JarArchive should probably be renamed to ZipArchive (that’s an existing 
> code).

Just curious, why?  Note the processing of the module-info.class.  Isn’t that 
more appropriate for a JarArchive rather than a ZipArchive?  Seems to me we 
should just change the internal instances of ZipFile to JarFile and change the 
name zipFile to jarFile for consistency.

> 
> Version of JarFile
> 
> 221 jarFile = new JarFile(file.toFile(), true, ZipFile.OPEN_READ, 
> JarFile.runtimeVersion());
> 
> JarFile::runtimeVersion is the version of the jlink tool.  You should not use 
> the version of the jlink runtime.  Instead, this should use the version of 
> java.base which will be the runtime version of the image being created.
> 
> ImageHelper::newArchive is one place where it creates JarArchive. You can 
> find java.base module from the Configuration passed to ImageHelper 
> constructor via Configuration.findModule(“java.base”) and from its module 
> descriptor.  Jlink implementation is rather over engineering at the moment.  
> You will have to find if there are other places to pass the right versoin 
> when creating ModularJarArchive.

As Alan noted, this is intended for phase 2 after he adds some apparently 
necessary code.

> 
> 
> 179 // a legal multi-release jar always has a base entry
> 
> I thought that a new class or a new concealed package can be added in a 
> versioned entry in MRJAR.  If so, those cases will not be included in the 
> finalNames.

If there is a class in a versioned directory that is not in the base directory, 
then that class must not be public or protected.  It’s not part of the public 
interface of the multi-release jar.  It exists purely because another class in 
the same versioned directory depends on it.  If we are creating a 
versionedStream for the version that the non-public class is in, it will be in 
finalNames, otherwise it won’t be.  I believe the code is correct here.

New concealed packages can be added in a versioned section of the jar file 
created by jar tool.  Should classes in concealed packages be added to 
finalNames or not?  Or stated differently, for jlink, should a versionedStream 
contain entries in concealed packages?

> 
> Have you considered adding a new method in JarFile to return the versioned 
> entry name and/or the version?  If it’s the base version, it will return the 
> same value as JarFile::getName.

There is a package-private method in JarFile that allows one to get the real 
name of a JarEntry.  It’s accessed externally by using SharedSecrets.  As far 
as i know it’s only used in two places.  The JEP-238 team decided not to make 
it a public method, although I think we could be persuaded to change our minds.

>  As we discussed the jdeps support for MRJAR offline, a tool would be 
> interested in getting the base entry name, versioned entry name, or even 
> version.

JarFile::getVersion exists.  And, as mentioned above, it possible to get a base 
name and a real name for a JarEntry.

> 
> Mandy

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

[Steve Drach]

I didn’t have it right ;-)  It turns out a JarFile stream of versioned entries 
was more interesting than I initially thought.  Here’s another webrev.  It’s 
not clear to me if I should include the change to JarFile in this changeset or 
if it should be in a stand alone changeset.  Advice appreciated.

webrev: http://cr.openjdk.java.net/~sdrach/8156499/webrev.02/ 
<http://cr.openjdk.java.net/~sdrach/8156499/webrev.02/>

> On Aug 9, 2016, at 5:41 PM, Steve Drach  wrote:
> 
> 
>> On Aug 8, 2016, at 12:46 PM, Alan Bateman > <mailto:alan.bate...@oracle.com>> wrote:
>>> 
>> I think we can do this in phases, starting out with jlink picking up the 
>> runtime version and using that to select the resources from the modular JAR 
>> would be a good start. At some point we will have an update module path 
>> implementation that better supports link time, in which case you can find 
>> java.base and then use its version (as opposed to the runtime version) when 
>> finding modules.
> 
> I think I have it right this time.  It’s much more complex unfortunately.
> 
> webrev: http://cr.openjdk.java.net/~sdrach/8156499/webrev.01/ 
> <http://cr.openjdk.java.net/~sdrach/8156499/webrev.01/>
> issue: https://bugs.openjdk.java.net/browse/JDK-8156499 
> <https://bugs.openjdk.java.net/browse/JDK-8156499>
> 
>>> The issue has to do with multi-release jar files.  How do exploded images 
>>> relate to multi-release jar files.
>>> 
>>> 
>> If you look at the other jlink tests then you'll see that they skip silently 
>> when on an exploded image (no packaged modules, meaning no JMOD files).
> 
> I couldn’t really find what you were referring to, other than a possibility 
> in BascTest, so what I did is assure the module path only has the Mr. Jar 
> file and jmods.  It’s possible that isn’t what you are looking for
> 

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

[Steve Drach]

> On Aug 8, 2016, at 12:46 PM, Alan Bateman  wrote:
>> 
> I think we can do this in phases, starting out with jlink picking up the 
> runtime version and using that to select the resources from the modular JAR 
> would be a good start. At some point we will have an update module path 
> implementation that better supports link time, in which case you can find 
> java.base and then use its version (as opposed to the runtime version) when 
> finding modules.

I think I have it right this time.  It’s much more complex unfortunately.

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

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


>> The issue has to do with multi-release jar files.  How do exploded images 
>> relate to multi-release jar files.
>> 
>> 
> If you look at the other jlink tests then you'll see that they skip silently 
> when on an exploded image (no packaged modules, meaning no JMOD files).

I couldn’t really find what you were referring to, other than a possibility in 
BascTest, so what I did is assure the module path only has the Mr. Jar file and 
jmods.  It’s possible that isn’t what you are looking for

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

[Steve Drach]

> On Aug 8, 2016, at 1:57 AM, Alan Bateman  wrote:
> 
> On 06/08/2016 00:43, Steve Drach wrote:
> 
>> Hi,
>> 
>> Please review this changset that makes jlink multi-release jar aware.
>> 
>> issue: https://bugs.openjdk.java.net/browse/JDK-8156499 
>> <https://bugs.openjdk.java.net/browse/JDK-8156499>
>> webrev: http://cr.openjdk.java.net/~sdrach/8156499/webrev.00/index.html 
>> <http://cr.openjdk.java.net/~sdrach/8156499/webrev.00/index.html>
>> 
> Are you sure that this is right? I ask because it looks like it appears to 
> ignore all resources in the JAR file that don't start with 
> META-INF/versions/.

Back to the old drawing board ;-)

> The other thing is that it is uses jarFile.version() which isn't right. At 
> some point (maybe a future patch) then it need to be using the version of 
> java.base.

Is there a way to determine the version of java.base?

> 
> Also I assume the test needs work so that it passes when run on an exploded 
> image.

The issue has to do with multi-release jar files.  How do exploded images 
relate to multi-release jar files.

> 
> -Alan

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: Multi release jars on boot classpath

[Steve Drach]

There are no plans to support multi-release jars on the boot classpath.

> On May 31, 2016, at 2:12 PM, Alan Bateman  wrote:
> 
> On 31/05/2016 20:53, Michael Rasmussen wrote:
>> Hi
>> 
>> I was looking over the specs for the multi release jars, and couldn't find
>> any information if this applied to jars on boot classpath as well (for
>> instance javaagents that add themselves there). All the docs and tasks
>> seems to focus on URLClassLoader, JarFile, etc.
>> 
>> So will multi release jars pick the right class if on boot classloader (ie
>> loaded from null classloader/jvm)?
>> 
> This is question for JEP 238 so including Steve Drach for the definite 
> answer. To my knowledge, there is no support for this. In general then it 
> wouldn't be common to deploy libraries with -Xbootclasspath/a. Yes, agents 
> might want to do this but this is somewhat niche area.
> 
> -Alan

Re: Paths in Mutli Release Jar

[Steve Drach]

> On May 27, 2016, at 2:16 AM, Alan Bateman  wrote:
> 
> On 27/05/2016 09:06, Robert Scholte wrote:
>> I noticed that the path is still META-INF/*versions*/ even though the JEP 
>> has been renamed from Multi Version jar to Multi Release jar. Is that 
>> intended or should/will that be changed as well?
> I'm not aware of any proposal to rename this. Steve Drach (cc'ed) is JEP 238 
> and should be able to confirm.

There is no proposal to rename this.

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
>