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


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.


  550                 String[] arr = mods.split(" ");
I think you will need to remove the double-quotes before doing the split.
Will fix that.

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


src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/ReleaseInfoPlugin.java
  137         release.put("MODULES", "\"" + new ModuleSorter(in.moduleView())
  138                 
.sorted().map(ResourcePoolModule::name).collect(Collectors.joining(" "))
  139                 + "\"”);

Since there is always at least one module, you can use Collectors.joining(“ “, "\””, 
"\””) instead of the prepending and appending.

Will fix that.


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.


test/tools/jlink/ModuleNamesOrderTest.java

  57         String moduleName = "bug8168925”;

Is it the output dir name?  This is not a module name.  @bug has the bug id.  I 
suggest to rename this to “image” or some other name.


Will fix it.

One suggestion to the validation is to remove the double quotes from MODULES 
property value and split it into an array.  That may be a better way to verify 
the dependences.
Hmm.. I'll look into that.

Thanks,
-Sundar

PS. Mandy Chung wrote ModuleSorter for another fix (yet to be pushed).  I'm 
using it for this fix after discussion with her (private email).
ModuleSorter is part of the patch for JDK-8169925[1].  I will take it out if 
Sundar pushes this changeset before JDK-8169925.

Mandy
[1]
http://mail.openjdk.java.net/pipermail/jigsaw-dev/2016-December/010416.html
[2] 
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8169925/webrev.00/jdk/test/tools/jlink/CustomPluginTest.java.udiff.html

Reply via email to