[modules-dev] Review request: 6560281

[Dave Bristor]

Stanley M. Ho wrote:
> 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/*.

We discussed this in my office, so just to summarize the changes I've made:

repository.properties is now loaded as a single file; there is no merging of 
them as was previously done.

The location of the file is specified in ModuleSystemConfig.  The default 
location can be overridden, but only if another property is set to true; this 
is also controlled by ModuleSystemConfig.  I updated
     src/share/lib/module/module.properties
with two new properties.

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

This got removed as part of the above work.

> 174     public static Repository configRepositories() throws 
> RuntimeException {
> 
> Should be private.

Fixed.

> 177             // XXX Should repeated configRepositories throw exception?
> 178             // XXX Log subsequent setup attempts.
> 
> Why would this being called more than once? Should throw exception.

Fixed to throw IllegalStateException.

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

What if it's a user-specified file?  For now it throws a RuntimeException in 
either case.

>   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 

Why must the source location be a URL?  In repository.properties it given as a 
file.

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

CLASS could be sun.module.repository.LocalRepository.

Or, we could check the URL: if it's a file: URL, create a LocalRepository.

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

Fixed.

> 
> - src/share/classes/sun/module/repository/LocalRepository.java
> 
>   185     public synchronized void initialize(Map<String, String> 
> config) throws IOException {
> 
> Should be private.

Fixed.

> - src/share/classes/sun/module/repository/URLRepository.java
> 
> 283     public synchronized void initialize(Map<String, String> config) 
> throws IOException {
> 
> Should be private.

Fixed.

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

Thanks, I left them alone.

> 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

Already there.

> 2. Repository that has unknown parent

Already there.

> 3. Dangling repository that is not under the main repository chain

Added.

> 4. Multiple furthest repositories

I'll work on that one.  For the moment I've made substantial enough changes in 
other regards that I wanted to send this re-review out despite not having 
those yet.

In the process of running tests, I found a failure, fixed it, but wonder about 
a couple things.  Take a look at the changes to ModuleLauncher.  It seems odd 
to me that we don't have a method Query.version(Version).  Is that intentional?

Second, repository.install returns a ModuleArchiveInfo, but we need a 
ModuleDefinition, so we find() it.  Would it make sense for a method
     ModuleDefinition Repository.get(ModuleArchiveInfo)
based on the identity of the ModuleArchiveInfo?

Kumar, can you explain the logic behind this, in ModuleLauncher?

             // Get the first one not a bootstrap! mind you.
             for (ModuleDefinition md : mlist) {
                 if (md.getRepository() != repository.getParent()) {
                     definition = md;
                     break;
                 }

With the RepositoryConfig changes in place, definition was getting set to 
java.se; probably because there are more repositories configured.

Webrev updated: http://javaweb.sfbay/java/jdk/ws/libs/rev/6560281/

Thanks,
	Dave

> 
> That's it for now.
> 
> - Stanley
> _______________________________________________
> modules-dev mailing list
> modules-dev at openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/modules-dev