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

Mandy Chung mandy.chung at oracle.com
Sat Dec 10 05:57:17 UTC 2016


> On Dec 9, 2016, at 7:32 PM, Sundararajan Athijegannathan <sundararajan.athijegannathan at 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.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!]
> 

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


More information about the jigsaw-dev mailing list