[modules-dev] Review request: 6560281

[Dave Bristor]

Stanley M. Ho wrote:
> Hi Dave,
> 
> Looks good. A few comments inline:
> 
> - src/share/lib/module/repository.properties
> 
>   32 extension.PARENT=BOOTSTRAP
>   33 extension.SOURCE=file:${java.home}/lib/module/ext
>   34 extension.CLASS=sun.module.repository.LocalRepository
>   35
>   36 user.PARENT=extension
>   37 user.SOURCE=file:${user.home}/.java/module
>   38 user.CLASS=sun.module.repository.LocalRepository
> 
> The upper case attribute looks a bit funny. Perhaps lower case? CLASS 
> should be classname instead - this is the convention was also used in 
> module.properties.

They are uppercase so that they stand out: the intent is that they be treated 
as reserved words, and that other respository implementation specific 
properties can be in lower case.  Perhaps make them lower case and/or use a 
case-insensitive comparison?

Related: the default jre extension repository directory is
     ${java.home}/lib/module/ext
Do you know into which Makefile I should cause that directory to be created? 
I've modified LocalRepository to create the sourceLocation if it does not 
exist (which will presumably suffice for the user repository).

> - src/share/lib/module/module.properties
> 
>   99 #     -Drepository.properties.file=somefile.
> 
> The system property (not the property in the module.properties file) 
> should be "java.module.repository.properties.file". See 
> "java.module.import.override.policy.file" and 
> "Djava.module.visibility.policy.file".

Fixed.  In line with your suggestion of "classname" above, shouldn't these all 
be "filename"?

> - src/share/classes/sun/module/repository/RepositoryConfig.java
> 
> Upper case issue described above.
> 
> 151             File f = new 
> File(replacePropertyNamesWithValues(location));
> 237             String repoValue = replacePropertyNamesWithValues(
> 370     private static String replacePropertyNamesWithValues(String s) {
> 
> You could simply use sun.security.util.PropertyExpander rather than 
> writing your own replacePropertyNamesWithValues(). This is what I used 
> for expanding the property names in other files. See 
> src/share/classessun/module/config/DefaultPolicy.java

Thanks, I didn't know about PropertyExpander: fixed.  Well, maybe not: if you 
give PropertyExpander.expand this string:
	${user.home/foo/bar
it doesn't throw an exception, which doesn't seem reasonable (IMHO).  At your 
suggestion, the code currently uses PropertyExpander, but I disabled the test 
which checked for the condition above.  Please let me know if I should restore 
the code I had before.


>  153                 BufferedInputStream is = new BufferedInputStream(
>  154                     new FileInputStream(f));
> 
> Should have finally block to close the stream afterwards.

Fixed.

> 206     public static Repository configRepositories(Properties 
> configProps) {
> 
> Should be private.

It's public so that it can be accessed from RepositoryConfigTest.

webrev updated!

Thanks,
	Dave

> 
> - Stanley