[modules-dev] Review request for 6560271 (embedded JAR files)

[Dave Bristor]

I fixed the issues w.r.t. uninstallation, and updated the webrev:
http://analemma.sfbay.sun.com/java/jdk/ws/libs/rev/6560271/

For the curious, these changes have to do with making the location of legacy
jars be consistent between Local and URL repositories, and ensuring that
uninstall() removes only those for the particular module being uninstalled.
Also, expansion directories are now not created unless required.

A minor downside of these changes is that ShutdownOnExitTest doesn't work on 
Windows, so it's been changed to pass on that platform.  We can't do better 
until disableModuleDefinition is implemented.

Thanks,
	Dave


Dave Bristor wrote:
> Even more followup: I just noticed in issue regarding uninstallation of 
> legacy jars: please hold off on any review until I get to look at it 
> tomorrow morning...
>     Dave
> 
> Dave Bristor wrote:
>> Thanks for the feedback, my followup inline.
>>
>> Kumar Srinivasan wrote:
>>> Hi Dave,
>>>
>>> Generally looks good. I am quite sure Stanley will do a more
>>> thorough review, here are some comments:
>>>
>>> 1. I noticed you were using buffers of 4096 and other places
>>> 8192, I think we should put a static in JamUtils.BUFFER_SIZE = 8192
>>> and simply use this everywhere or if we want different sizes for
>>> whatever reason we could do this:
>>> JamUtils.LARGE_BUFFER_SIZE = 8192
>>> JamUtils.MEDIUM_BUFFER_SIZE = 4096
>>> JamUtils.SMALL_BUFFER_SIZE = 2048
>>
>> I added a single BUFFER_SIZE = 8192 in JamUtils, and made use of it in 
>> files that are already part of this webrev.  I updated 6566724 to 
>> explicitly mention this for other code too:
>>
>> 6566724 Add constants for oft-used strings such as MODULE.METADATA
>>
>> That'll make a nice cleanup task for a rainy day ;-)
>>
>>> 2. LocalJamDefinitionContent.java s/cpde/code/g
>>
>> Webrev updated:
>> http://analemma.sfbay.sun.com/java/jdk/ws/libs/rev/6560271
>>
>> Fixed, thanks.
>>
>>     Dave
>>>
>>>
>>>
>>>> Hi folks,
>>>>
>>>> This set of changes lets embedded legacy JAR files work, including 
>>>> with the JarLibraryPath annotation.  I plan to build on this in 
>>>> making changes to support native libraries.  Comments welcome!
>>>>
>>>> bugster: http://monaco.sfbay/detail.jsp?cr=6560271
>>>> webrev: http://analemma.sfbay.sun.com/java/jdk/ws/libs/rev/6560271
>>>>
>>>> Description of changes:
>>>>
>>>> Several bits of cleanup, such as for javadoc, consistency, etc.
>>>>
>>>> JamUtils: Added copyFile() which is therefore removed from 
>>>> URLRepository.
>>>>
>>>> LocalRepository: Improve location of expansionDir.
>>>>
>>>> URLRepository: Just cleanup.
>>>>
>>>> LocalJamDefinitionContent: Support for legacy/embedded JAR files.  
>>>> Code cleanup to make it more consistent with 
>>>> URLModuleDefinitionContent.  I think this should be renamed to 
>>>> LocalModuleDefinitionContent for consistency (or rename the other 
>>>> class); opinions?
>>>>
>>>> URLModuleDefinitionContent: Support for legacy/embedded JAR files.
>>>>
>>>> RepositoryUtils: This new class has static methods which were 
>>>> formerly duplicated in others, especially {Local,URL}Repository.  In 
>>>> theory, this could be a pair of base classes, one for the 
>>>> *Repository implementations, and one for the *ModuleDefinition 
>>>> implementations.  The former is particularly likely.
>>>>
>>>> JamBuilder: now public constructor and can add annotations to the 
>>>> JAM that's to be built.
>>>>
>>>> LegacyJarTest: makes sure the above works ;-)
>>>>
>>>> Thanks,
>>>>     Dave
>>>> _______________________________________________
>>>> modules-dev mailing list
>>>> modules-dev at openjdk.java.net
>>>> http://mail.openjdk.java.net/mailman/listinfo/modules-dev
>>>>   
>>>
>> _______________________________________________
>> modules-dev mailing list
>> modules-dev at openjdk.java.net
>> http://mail.openjdk.java.net/mailman/listinfo/modules-dev
>