[modules-dev] Review please for launcher bugs

[Kumar Srinivasan]

Based on all the feedback I have adjusted my previous webrevs,
pending putback on Dave's fix for the InvocationTargetEx.:

The latest webrev:
http://javaweb.sfbay/~ksrini/webrevs/j2se-tools/jsr277-7/

Diff to the first webrev:
http://javaweb.sfbay/~ksrini/webrevs/j2se-tools/jsr277-7-vs-6/

The first webrev:
http://javaweb.sfbay/~ksrini/webrevs/j2se-tools/jsr277-6/


> 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
>>>   
>>>       
>>     
> _______________________________________________
> modules-dev mailing list
> modules-dev at openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/modules-dev
>   


-- 
Kumar Srinivasan           
Sun Microsystems, Java Software.
408-276-7586