[modules-dev] Review please for launcher bugs

[Dave Bristor]

Kumar Srinivasan wrote:
> Thanks for all the comments, I have consolidated all the comments here.
> Please see my embedded responses.
> Thanks
> 
> 
> Andreas------
>>   . there should be a check for "definition == null" in 
>> getModuleDefinitionFromJam() that throws an appropriate exception if 
>> we don't find the module. It could be null if the JAM file is corrupt.
> Yes will add a check and throw an exception.
> 
> -------------------------------------------------------------------
> 
> Stanley----------
>> I think what is unclear is that Kumar's fix checks if the filename 
>> returned from ModuleArchiveInfo ends with JamUtils.EXPANDED_EXT, and 
>> this kinda implies that the repository implementation is returning an 
>> internal cached jam filename in this case.
> Hmm, I will assume the file is blah.jar-expanded for now, Dave can 
> remove the
> -expanded when he puts the fix into the repository.

I added a note about this to 6582244.

> -------------------------------------------------------------------
> 
> Dave Bristor------
>>
>> - src/share/classes/sun/module/ModuleLauncher.java
>>
>> 147     private static Repository repository = null;
>>
>> Please put this and all other field declarations at the top of the 
>> file, not in the middle.
>>   
> Ok
> 
>>   188         String fname = new File(jamFileName).getName();
>>   189         String moduleName = fname.substring(0, 
>> fname.lastIndexOf('.'));
>>   190         List<ModuleArchiveInfo> ilist =  repository.list();
>>   191         for (ModuleArchiveInfo i : ilist) {
>>   192             if (i.getFileName().endsWith(fname + 
>> JamUtils.EXPANDED_EXT)) {
>>   193                 definition = getModuleDefinition(repository, 
>> i.getName());
>>   194                 break;
>>   195             }
>>   196         }
>>
>> I remain somewhat bewildered by this: could you explain?  
> Yes, the directory containing the jam becomes the system repository as 
> the bug 6580159,
> therefore I create a LocalRepository, once I found that I need to get 
> the moduleDefinition
> of the given JAM-file. Does that make sense ?

I understand from earlier email what's going on, but using the jamFileName in 
expectation of it being part of the module name is not a 100% solution, since 
there's no required correspondence.

>> Also, line 188 could be
>>     String fname = baseDir.getName();
>> though perhaps baseDir should be renamed jamFile (since it's not a 
>> base directory).
>>   
> Yowsa, Its somewhat confusing here with the naming, I will clean it up.
> 
>>   207             if (!jamFileName.endsWith(".jam") && 
>> !jamFileName.endsWith(".jar")) {
>>   208                 throw new IllegalArgumentException("jam filename 
>> must have a .jam or .jar extension");
>>
>> Shouldn't .jam.pack.gz also be allowed?  Or is that just 
>> not-yet-implemented?   (I know it's not fully supported elsewhere...)
>>   
> Good question!. Is .jam.pack.gz allowed ? I did not see it in the 
> original specification.
> http://j2se.sfbay.sun.com/web/bin/view/J2SE/JamsProposedToolsSupport

What I see on that page is mention of options like "... -jam file ..." and I 
assumed that a .jam.pack.gz would also be acceptable.  Once I implement 
6566022, it should be possible to use them in local repositories.

>>   231                         throw new Exception("Repository not 
>> found at " + repFile.toString());
>>
>> Should this also be IllegalArgumentException?
>>   
> I was thinking about that. I will change them to IllegalArgumentException.
>> - test/java/module/basic/BasicLauncherTests.java
>>
>>   260     static String expRepository;
>>   261     static String jamRepository;
>>   262     static String urlRepository ;
>>
>> Extra space in line 262 and please put fields at beginning of file.
>>   
> Acknowledged.
> 
>> Several other places, why the change from expRepository to 
>> urlRepository?  I might have thought an addition would be appropriate; 
>> was removal of expRepository intentional?
>>   
> Nothing was removed instead of using expRepository I am using urlRepository
> to test, also I renamed httpRepository to the more generic one 
> urlRepository.
> To address CR: 6580122.

Ah, ok!

Thanks,
	Dave

>> Kumar Srinivasan wrote:
>>  
>>> Hi,
>>>
>>> I need a review of these fixes asap.
>>> http://javaweb.sfbay/~ksrini/webrevs/j2se-tools/jsr277-6/
>>> [The webrev contains the CRs].
>>>
>>> Built and tested on Solaris and Windows all the regresssion
>>> tests in test/java/module pass.
>>>
>>> Thanks
>>>
>>>     
>> _______________________________________________
>> modules-dev mailing list
>> modules-dev at openjdk.java.net
>> http://mail.openjdk.java.net/mailman/listinfo/modules-dev
>>   
> 
>