[modules-dev] Review request for 6608500
David.Bristor at Sun.COM
Wed Sep 26 13:34:58 PDT 2007
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
> 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!
> - 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
>> 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.
>> modules-dev mailing list
>> modules-dev at openjdk.java.net
More information about the modules-dev