Hi,

Please review the updated webrev: http://cr.openjdk.java.net/~sundar/8168925/webrev.02/

Changes from the last review:

* Moved all release file property filling code from DefaultImageBuilder.java to ReleaseInfoPlugin.java * DefaultImageBuilder populates this.targetOsName directly from java.base module
* Added a comment for CATEGORIES_ORDER in ImagePluginConfiguration.java
* In test ModuleNamesOrderTest.java, changed moduleName to be output dir name ("image..." name)

Thanks,
-Sundar

On 10/12/16, 11:27 AM, Mandy Chung wrote:
On Dec 9, 2016, at 7:32 PM, Sundararajan 
Athijegannathan<sundararajan.athijegannat...@oracle.com>  wrote:

Hi,

Thanks for your review. Comments below..

On 10/12/16, 2:18 AM, Mandy Chung wrote:
On Dec 9, 2016, at 12:49 AM, Sundararajan 
Athijegannathan<sundararajan.athijegannat...@oracle.com>   wrote:

Please review http://cr.openjdk.java.net/~sundar/8168925/webrev.01/index.html 
for https://bugs.openjdk.java.net/browse/JDK-8168925

Is the order of Plugin.Category enums significant?  You moved COMPRESSOR down - 
is it necessary?
Yes. Without that ReleaseInfoPlugin will receive compressed resources and 
compressed module-info.class won't be parsed okay by ModuleDescriptor.read 
(called by ModuleSorter). Note that auto-decompression is done only by 
LastResourcePool - which is created after all plugins operate. [I kept getting 
test failures and debugged to find this is the cause of failures!]

OK.  If the order of the enums determines the plugin ordering, it’d be good 
adding a comment.

Can you look at DefaultImageBuilder::releaseProperties which I think this should be 
moved to ReleaseInfoPlugin?  The content of `release` should be written when the new 
entry "/java.base/release” is added to the resource pool.  DefaultImageBuilder 
does not need to add any more properties to this `release` file as all properties in 
`release` are known once the graph is resolved.
Okay, I'll check if I can move all stuff there. From initial communication 
[private email], I thought the suggestion was only about MODULES.
This has beeen my suggestion - move the whole thing out from 
DefaultImageBuilder.  I don’t see the reason why it has to be done in two 
separate places.  That’ll be a good clean up.


src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ResourcePoolManager.java
- is this change needed?
Yes. Helped me debugging to find out which module's module-info.class was not 
parsed fine. Exception translation puts the name of the module in the message - 
which will be missing in the exception thrown by 
ModuleDescriptor.read(ByteBuffer).
OK.

test/tools/jlink/CustomPluginTest.java
    It’s one option to disable the releaseinfo plugin.  An alternative
    way to fix this test is [2].
I could. But that test checks for the module dependency checks that happen 
after all plugins are exercised. That should happen regardless of release-info 
plugin is enabled. i.e., even when release-info plugin is disabled, that module 
missing error should be thrown and the test checks for that.

OK.  If I happen to push JDK-8169925 before this, you can revert my change to 
this test.

Thanks
Mandy

Reply via email to