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
