Re: Review request: 8157068 ExceptionInInitializerError if images build patched to use exploded version of java.lang.module.SystemModules

2016-05-19 Thread Mandy Chung

> On May 19, 2016, at 9:19 AM, Alan Bateman  wrote:
> 
> 
> On 19/05/2016 15:48, Mandy Chung wrote:
>> Updated webrev:
>>
>> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8157068/webrev.01/index.html
>> 
>> Mandy
>> 
> This looks okay except the naming of the sub-directories (basic and 
> SystemModules) is inconsistent. Maybe it needs to be 
> systemmodules/PatchSystemModules.java.

I’m okay to rename them.

Mandy



Re: Review request: 8157068 ExceptionInInitializerError if images build patched to use exploded version of java.lang.module.SystemModules

2016-05-19 Thread Alan Bateman


On 19/05/2016 15:48, Mandy Chung wrote:

Updated webrev:
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8157068/webrev.01/index.html

Mandy

This looks okay except the naming of the sub-directories (basic and 
SystemModules) is inconsistent. Maybe it needs to be 
systemmodules/PatchSystemModules.java.


-Alan.


Re: Review request: 8157068 ExceptionInInitializerError if images build patched to use exploded version of java.lang.module.SystemModules

2016-05-19 Thread Mandy Chung
Updated webrev:
   http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8157068/webrev.01/index.html

Mandy



Re: Review request: 8157068 ExceptionInInitializerError if images build patched to use exploded version of java.lang.module.SystemModules

2016-05-19 Thread Mandy Chung

> On May 18, 2016, at 11:27 PM, Alan Bateman  wrote:
> 
> On 19/05/2016 02:41, Mandy Chung wrote:
>> Webrev at:
>>   
>> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8157068/webrev.00/index.html
>> 
>> This is to allow to patch java.base with an exploded image for JDK 
>> development purpose like this:
>> $ images/jdk/bin/java -Xpatch:java.base=jdk/modules/java.base -version
>> 
>> jdk.internal.module.SystemModules class is generated at link time to allow 
>> fast reconstitution of ModuleDescriptor.  If an image is patched with 
>> java.base of an exploded image, it will fall back to read and parse 
>> module-info.class from the jimage.  Hashes of tied modules are recorded in 
>> jdk.internal.module.SystemModules class in the fast path.  If patched, this 
>> fix will use the hashes recorded in the Hashes attribute for integrity check 
>> (that already validated at link time).
>> 
> A complicated scenario but the approach looks okay. No need to use 
> JavaLangModuleAccess to get at the hashes, just use descriptor.hashes().
> 

Ah, of course, SystemModuleFinder is in java.lang.module (I got mixed up with 
jdk.internal.module.SystemModules)

> The patch test directory already has a basic test for -Xpatch and might be 
> confusing to have the modules for both tests in the same tree. I wonder if we 
> should move each both tests in their own sub-directory to keep it easier to 
> understand.

That’s good.  I was looking for your opinion on this.  I considered putting 
them in a separate sub-directory and think it makes it clearer.  

Will move them before I push.

Mandy

Re: Review request: 8157068 ExceptionInInitializerError if images build patched to use exploded version of java.lang.module.SystemModules

2016-05-18 Thread Alan Bateman

On 19/05/2016 02:41, Mandy Chung wrote:

Webrev at:
   http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8157068/webrev.00/index.html

This is to allow to patch java.base with an exploded image for JDK development 
purpose like this:
$ images/jdk/bin/java -Xpatch:java.base=jdk/modules/java.base -version

jdk.internal.module.SystemModules class is generated at link time to allow fast 
reconstitution of ModuleDescriptor.  If an image is patched with java.base of 
an exploded image, it will fall back to read and parse module-info.class from 
the jimage.  Hashes of tied modules are recorded in 
jdk.internal.module.SystemModules class in the fast path.  If patched, this fix 
will use the hashes recorded in the Hashes attribute for integrity check (that 
already validated at link time).

A complicated scenario but the approach looks okay. No need to use 
JavaLangModuleAccess to get at the hashes, just use descriptor.hashes().


The patch test directory already has a basic test for -Xpatch and might 
be confusing to have the modules for both tests in the same tree. I 
wonder if we should move each both tests in their own sub-directory to 
keep it easier to understand.


-Alan.