> 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
