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

[Mandy Chung]

> On Nov 1, 2016, at 2:44 PM, Steve Drach  wrote:
> 
> 
 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.

Thanks. So good to go.
Mandy

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

[Mandy Chung]

> On Oct 31, 2016, at 8:26 PM, Mandy Chung  wrote:
> 
> 
>> On Oct 27, 2016, at 5:22 PM, Steve Drach  wrote:
>> 
>> 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”?

Mandy

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

[Mandy Chung]

> On Oct 27, 2016, at 5:22 PM, Steve Drach  wrote:
> 
> 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.

Mandy

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

[Alan Bateman]

On 28/10/2016 01:22, Steve Drach wrote:
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.
The changes to jlink look okay (same as previous round). I briefly 
looked at the updated test and it looks like it does all the right 
checking. Mandy had detailed comments on the tests so I'll leave that to 
her.


Once this is in then the next step will be have jlink find the java.base 
module and then use its version as runtime version for the module finder 
that finds the modules to link into the image. The changes to jlink 
should be straight-forward but testing will be complicated. I only 
mention now in case it you are thinking of re-using the current test for 
the second phase.


-Alan

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.

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

[Alan Bateman]

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/ 





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/ 
> 
Argh!  Sorry about that.  Here’s the correct link 
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

[Mandy Chung]

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

This webrev is not for this issue?

Mandy

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

[Alan Bateman]

On 19/10/2016 20:15, 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.

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

[Alan Bateman]

On 13/10/2016 00:00, Steve Drach wrote:


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). Are directories the only case 
where toEntry can return null, in which case would it simpler to filter 
out directories here.


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

[Alan Bateman]

On 22/08/2016 21:43, Steve Drach wrote:

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


One thing we need to do is capture all the details on how modular JAR 
should work as multi-release JARs and get this written down in JEP 235 
or JEP 261. I'm not sure yet which JEP is the most appropriate.


-Alan.

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

[Mandy Chung]

> On Aug 22, 2016, at 2:56 PM, Steve Drach  wrote:
> 
> Is a jar tool that takes a -d option lurking somewhere?  The latest one I 
> have does not have that option.

It’s in jdk-9+132 and has been in jdk9/dev for 1.5 weeks (see JDK-8136930).  It 
was -p previously.

$ jar -h   
Usage: jar [OPTION...] [ [--release VERSION] [-C dir] files] …
:
:

 Main operation mode:

  -c, --create   Create the archive
  -i, --generate-index=FILE  Generate index information for the specified jar
 archives
  -t, --list List the table of contents for the archive
  -u, --update   Update an existing jar archive
  -x, --extract  Extract named (or all) files from the archive
  -d, --print-module-descriptor  Print the module descriptor

:

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

[Alan Bateman]

On 22/08/2016 22:04, Mandy Chung wrote:

:


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


which is correct. I think the configuration to try is a MR JAR containing:

module-info.class
p/Hi.class
META-INF/versions/9/p/Hi.class
META-INF/versions/9/p/internal/Hi5.class

Then repeat with a META-INF/versions/9/module-info.class.

In all cases then I would assume p.internal should be in the set of 
concealed packages.


-Alan

-Alan

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

[Mandy Chung]

> On Aug 22, 2016, at 1:35 PM, Chris Hegarty  wrote:
> 
> 
>> On 22 Aug 2016, at 19:24, Steve Drach  wrote:
>>> 
>>> 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

Mandy

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

[Chris Hegarty]

> On 22 Aug 2016, at 19:24, Steve Drach  wrote:
>> ...
>>> 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.

-Chris.

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

[Alan Bateman]

On 22/08/2016 18:47, Steve Drach wrote:


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




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.


-Alan

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

[Alan Bateman]

On 21/08/2016 03:49, Mandy Chung wrote:



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.

I agree on the test scenarios as it's tricky to get them all right.

On concealed packages then the intention is that they don't vary by JDK 
version, this means the versioned section can't add packages. This isn't 
a real limitation of course, it just means that the set of concealed 
packages in the base version might need to list additional packages. The 
jar tool has support for generating this but I don't know how many 
scenarios are tested.





I think ModularJarArchive is the right place to add the versioned entries 
support.  JarArchive should probably be renamed to ZipArchive (that’s an 
existing code).

I agree.




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.
Right, in the previous mails then I think we agreed to leave this to a 
second phase. The reason is that it requires refactoring in other areas. 
I have changes that Steve will need to do this second phase but for now 
I think it would be good to get the first phase done as it's the phase 
with all the tricky scenarios.


-Alan

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

[Mandy Chung]

> On Aug 12, 2016, at 5:33 PM, Steve Drach  wrote:
> 
> 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.


I think ModularJarArchive is the right place to add the versioned entries 
support.  JarArchive should probably be renamed to ZipArchive (that’s an 
existing code).

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.


 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.

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

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/ 


> On Aug 9, 2016, at 5:41 PM, Steve Drach  wrote:
> 
> 
>> 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 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

[Alan Bateman]

On 08/08/2016 17:34, Steve Drach wrote:

:
Back to the old drawing board ;-)
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.




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


-Alan

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 
>> 
>> webrev: 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

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

[Alan Bateman]

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 

webrev: 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/. 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.


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


-Alan