RFR 8168925: MODULES property should be topologically ordered and space-separated list

[Sundararajan Athijegannathan]

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