[modules-dev] Review request: 6560281

[Stanley M. Ho]

Hi Dave,

Looks good. Several comments inline:

- src/share/classes/sun/module/repository/RepositoryConfig.java

   87      * <li>user.home/.java/repository/repository.properties</li>
   88      * <li>$java.home/lib/repository/repository.properties</li>

As we discussed before, because the user-specific configuration would be 
picked up by different versions of the JRE the user is running, it is 
unclear to me if merging the settings in these configurations files is 
really what we want, especially if the settings in these files could be 
in conflict.

If we want to support multiple files, I think it is more reasonable for 
user to provide a custom repository.properties file that tells the 
module system to construct additional repositories from the furthest 
repository created in the JRE's repository.properties file, and set the 
resulting furthest repository as the system repository.

It might also be reasonable for user to specify a specific 
repository.properties that completely overrides the JRE's 
repository.properties. You will find similar logic implemented in 
src/share/classes/sun/module/config/ModuleSystemConfig.

For the JRE specific configuration, it should live under 
{java.home}/lib/module/repository.properties. I suggest you should also 
have a default repository.properties file checked into the workspace, 
see src/share/lib/module/*.

100     static {

The calls to System.getProperty() should be in doPrivileged() block. 
Because of the nature of class initialization, this could fail if 
untrusted code is on the stack.

174     public static Repository configRepositories() throws 
RuntimeException {

Should be private.

177             // XXX Should repeated configRepositories throw exception?
178             // XXX Log subsequent setup attempts.

Why would this being called more than once? Should throw exception.

191                         // XXX nonexistent/unreadable config file: 
log message? throw exception?

The {java.home}/lib/module/repository.properties file should always 
exists. If not, exception should be thrown.

  231      * If no CLASS is given, then if the SOURCE value is a URL, a
  232      * URLRepository will be created; else if the SOURCE is an 
existing,
  233      * readable file, a LocalRepository will be created.

The content of the JRE's default repository configuration file will be 
pretty much the same under all platforms, hence the source location will 
  be in the form of URL. This implies that only URLRepository could be 
created in our default configuration. While URLRepository has 
advantages, there are several repositories we definitely need to create 
using LocalRepository, e.g. the global repository and the user 
repository. #344 - #353 demonstrates this issue clearly. I think we need 
a better way to indicate whether it's LocalRepository or URLRepository 
that we want to create. Perhaps we need to support something like TYPE.

374             // XXX Throw exception, or ignore and presume "user 
knows best"?

Throw exception.

- src/share/classes/sun/module/repository/LocalRepository.java

  185     public synchronized void initialize(Map<String, String> 
config) throws IOException {

Should be private.

- src/share/classes/sun/module/repository/URLRepository.java

283     public synchronized void initialize(Map<String, String> config) 
throws IOException {

Should be private.

- src/share/classes/java/module/Modules.java

The javadoc needs to be updated to describe the exceptions. I will take 
care of this when I make other API changes later.

For the test cases, you might want to check out the tests for the 
visibility policy and import override policy in 
test/java/module/config/*. Because the repository.properties file is 
basically a regular property file, you probably don't really need a lot 
of tests around the file format. The focus should be to make sure the 
code handles the repository chain construction properly in various 
situations, including:

1. Well-formed repository chain
2. Repository that has unknown parent
3. Dangling repository that is not under the main repository chain
4. Multiple furthest repositories

That's it for now.

- Stanley