[modules-dev] Review request for 6608500

[Dave Bristor]

Stanley M. Ho wrote:
> Hi Dave,
> 
> Looks good. A few comments:
> 
> - src/share/classes/sun/module/repository/LocalRepository.java
> - src/share/classes/sun/module/repository/URLRepository.java
> 
> The value of the system property should be retrieved within a 
> doPrivileged block, otherwise it will fail when it runs inside the sandbox.

Good point; I made this change.

> Also, this should be done once, possibly in a static initializer, so we 
> don't need to keep checking the system property whenever a new 
> repository is initialized.

Done, and since system properties are mutable, I'll also added javadoc to the 
initialize() methods noting the values are fixed once the classes are loaded..

> 228         expansionDirectory = value == null ? sourceDirectory : new 
> File(value);
> 
> It is unclear why the path of the expansion directory is derived from 
> the source directory. I would expect that the expansion directory is 
> somewhere in temp.

This is the prior in behavior, unchanged.  I changed code so it's now in temp. 
  But then I observed that, since we don't set the property in all our tests, 
an expansion directory is created for each test in temp!  Kelly / JPRT won't 
be pleased, and that's contrary to the point of this bugfix.  I propose to 
revert the code for now, and cause the expansion directory to be created on an 
as-needed basis, during install(), as we discussed yesterday w.r.t. the other 
bug on my plate, 6605100.  An alternative would be to modify all of our tests 
to set the extension directory's path as part of this fix.  The webrev has the 
code reverted to using source directory.

webrev updated, and thanks!

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

	Dave

> - Stanley
> 
> 
> Dave Bristor wrote:
>> Hi folks,
>>
>> This cleans up some tests.  It includes the changes we mentioned 
>> yesterday w.r.t. *Repository implementations: they can now be 
>> configured from system properties, and those values override values in 
>> the Map<String, String> that's provided when the instance is created.  
>> These changes should keep old test files from filling JPRT's temp 
>> directories.
>>
>> bugster: http://monaco.sfbay/detail.jsf?cr=6608500
>> webrev: http://analemma.sfbay.sun.com/java/jdk/ws/libs/rev/6608500/
>>
>> Please note that, with respect to configuring repositories use of the 
>> Map is still in LocalRepositoryTest and URLRepositoryTest.  Testing 
>> via System.getProperty is in BasicLauncherTests.
>>
>> Thanks,
>>     Dave
>> _______________________________________________
>> modules-dev mailing list
>> modules-dev at openjdk.java.net
>> http://mail.openjdk.java.net/mailman/listinfo/modules-dev