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

[Dave Bristor]

Thanks for the feedback; all fixed.

Andreas Sterbenz wrote:
> Dave Bristor wrote:
>>
>> bugster: http://monaco.sfbay/detail.jsp?cr=6560271
>> webrev: http://analemma.sfbay.sun.com/java/jdk/ws/libs/rev/6560271/
> 
> Looks fine, some minor comments:
> 
>  . JamUtils.copyFile(): creating a BufferedInputStream is not really 
> useful here since the reads are all done in BUFFER_SIZEd blocks anyway

I removed the BufferedInputStream.

>  . LocalRepository.installJams(): not sure why you deleted the comment 
> about .pack.gz here, but if you don't need it, that is fine with me

I wanted to clean out the XXX commentary where we have bugs filed, and in this
case we've got 6566022.

>  . LocalRepository.assertValidDirs(): why did you change !isDirectory() 
> to isFile() ?

I'd changed the use of expansionDir such that it isn't created in the 
filesystem unless there are any actual legacy JAR files; if it is not null but 
it's a plain file that would be bad.  But your comment prompted this 
clarification/improvement:

         if (expansionDir != null
             && (expansionDir.exists() && !expansionDir.isDirectory())) {

>  . LocalJamDefinitionContent.createModuleDefinition(): is that string 
> really intended to be "URLModuleDefinitionContent" ?

Whoops; changed to LocalJamDefinitionContent and another similar URL* mention
in a comment.

>  . RepositoryUtils.extractJarFiles(): is that always 
> "URLModuleDefinitionContent" ?

Whoops again.  I added a private static final String NAME to
{URLModule,LocalJam}DefinitionContent, and pass that where appropriate.

>  . RepositoryUtils.extractJarFiles:223: I may be missing something, but 
> are you creating a file and then immediately deleting it in some cases?

Unfortunately so.  Section 5.2.4 is explicit that legacy JARs must not contain
MODULE.METADATA.  The only way I know to determine that is to examine the
embedded JAR's contents, and to do that we (currently) have to extract the JAR
into its own file.

I updated the webrev after seeing all tests pass.

http://analemma.sfbay.sun.com/java/jdk/ws/libs/rev/6560271/

Thanks,
	Dave

> 
> 
> Andreas.
>