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

[Dave Bristor]

Hi folks,

Any further feedback on 6560271?

bugster: http://monaco.sfbay/detail.jsp?cr=6560271
webrev: http://analemma.sfbay.sun.com/java/jdk/ws/libs/rev/6560271/

I heard from Kumar earlier that it was OK.

I ask because, in a separate workspace, I have changes for

6601899: URLRepository.find*() must not return modules for other than the 
appropriate platform/arch

which take care of using platform/arch.  Presuming that the changes for 
6560271 are "roughly correct", I've made these more recent changes essentially 
on top of those for 6560271.  Since webrev diffs against checked-in files, I 
don't think it's possible to generate one between these two changes.  I'll 
make one for 6601899 once I've checked in 6560271.

But I won't check that in, until I hear that it's OK.  Any takers?

Thanks,
	Dave

-------- Original Message --------
Subject: Re: [modules-dev] Review request for 6560271 (embedded JAR files)
Date: Thu, 18 Oct 2007 14:48:04 -0700
From: Dave Bristor <David.Bristor at Sun.COM>
To: Dave Bristor <David.Bristor at Sun.COM>
CC: Kumar Srinivasan <Kumar.Srinivasan at Sun.COM>, modules-dev at openjdk.java.net
References: <47153E42.2010103 at sun.com> <47155718.4010706 at Sun.COM> 
<4715609B.4090005 at sun.com> <47156325.9080101 at sun.com>

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
>