RFR 8168925: MODULES property should be topologically ordered and space-separated list
sundararajan.athijegannathan at oracle.com
Sat Dec 10 03:32:45 UTC 2016
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
> 550 String arr = mods.split(" ");
> I think you will need to remove the double-quotes before doing the split.
Will fix that.
> - 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
> 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.
> It’s one option to disable the releaseinfo plugin. An alternative
> way to fix this test is .
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
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.
>> 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. I will take it out if Sundar pushes this changeset before JDK-8169925.
>  http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8169925/webrev.00/jdk/test/tools/jlink/CustomPluginTest.java.udiff.html
More information about the jigsaw-dev