[modules-dev] Review please for launcher bugs

[Kumar Srinivasan]

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.


-------------------------------------------------------------------

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 ?



> 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
>   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.
> 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
>   


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