[modules-dev] Review request for 6559067, 6559086, and 6605100
Dave Bristor
David.Bristor at Sun.COM
Thu Oct 11 14:58:06 PDT 2007
Hi Stanley, thanks for all the feedback...it was a lot to review! Comments
inline.
Stanley M. Ho wrote:
> Hi Dave,
>
> Looks good. Several comments:
>
> - src/share/classes/sun/module/repository/LocalRepository.java
>
> 253 public synchronized ModuleArchiveInfo install(URL u) throws
> IOException {
>
> Should check if repository is read-only. Local repository is read-only
> if the source location is not writable by the user.
Fixed. See also changes to isReadOnly().
> 260 throw new IOException(
> 261 "Cannot overwrite " + f.getName() + " in repository
> directory " + sourceDir);
>
> Should throw IllegalStateException instead because a module definition
> with the same name, version, and platform binding has already been
> installed.
Fixed.
> 261 "Cannot overwrite " + f.getName() + " in repository
> directory " + sourceDir);
>
> "in repository's source directory" instead of "in repository directory".
Fixed.
> 278 public synchronized boolean uninstall(ModuleArchiveInfo m)
> throws IOException {
>
> Should check if repository is read-only.
Fixed; FWIW I don't see it as a requirement in the spec nor javadoc (not to
discount it as a good idea!). I changed install() and uninstall().
I also added a check to disallow installation of modules whose names begin
with "java." (No test for this latter, though.)
> 287 if (!f.exists()) {
> 288 throw new IOException("Could not find module file for "
> + m.getName()
> 289 + "in the repository");
> 290 }
>
> If module does not exist, should simply return false. See spec.
Fixed.
> 295 if (f2.isDirectory()) {
>
> What happens if f2 is a file?
Then somehow a file of that name got created, perhaps by a user; we shouldn't
do anything with it. The non-directory situation now throws
IllegalStateException.
>
> 328 ArrayList<ModuleDefinition> rc = new
> ArrayList<ModuleDefinition>();
> 329 if (constraint == Query.ANY) {
> 330 for (ModuleDefinition md :
> contents.getModuleDefinitions()) {
> 331 rc.add(md);
> 332 }
> 333 } else {
> 334 for (ModuleDefinition md :
> contents.getModuleDefinitions()) {
> 335 if (constraint.match(md)) {
> 336 rc.add(md);
> 337 }
> 338 }
> 339 }
>
> The result should be read-only, and it could also be cached for
> subsequent query as an optimization.
>
> List<ModuleDefinition> rc;
> if (constraint == Query.ANY) {
> rc = Collections.unmodifiableList(contents.getModuleDefinitions());
> } else {
> rc = new ArrayList<ModuleDefinition>();
> for (ModuleDefinition md : contents.getModuleDefinitions()) {
> if (constraint.match(md)) {
> rc.add(md);
> }
> }
> rc = Collections.unmodifiableList(rc);
> }
I made the result unmodifiable, and introduced some caching; see modDefCache.
As an aside: findModuleDefinitions has to take the platform binding into
account, so I fixed this too; see platformMatches(). For now that returns
true but that will change once platform binding support gets added (see 6601899).
>
> 444 throw new NullPointerException("LocalRepository: config
> cannot be null.");
> 582 "LocalRepository: specified " + type + " directory "
>
> I notice that some exceptions' messages are prefixed with
> "LocalRespository:" while some aren't. Should be consistent.
Fixed by introducing new method msg(String) which returns given String arg
with a prefix. I changed all (hopefully!) throws to use this.
> 496 if (expansionDirName != null) {
> 497 expansionDir = new File(expansionDirName,
> 498 "expansion" + "-" + timestamp);
> 499 expansionDir.mkdir();
> 500 } else {
> 501 File tmp = File.createTempFile("jamtmp" +
> System.currentTimeMillis(), "tmp");
> 502 expansionDir = new File(tmp.getParent(),
> 503 "LocalRepositoryExpansion-"
> 504 + getName() + "-" + timestamp);
> 505 tmp.delete();
> 506 expansionDir.mkdirs();
> 507 }
>
> Could be simplified somewhat:
>
> if (expansionDirName == null) {
> File tmp = File.createTempFile("jamtmp", System.currentTimeMillis(),
> "tmp");
> expansionDirName = tmp.getParent().getCanonicalPath();
> tmp.delete();
> }
> expansionDir = new File(expansionDirName, "LocalRepositoryExpansion-" +
> getName() + "-" + timestamp);
> expansionDir.mkdirs();
Done!
> 495 private void initializeDirs() throws IOException {
>
> Perhaps "constructDirs", "constructCache", or "initializeCache".
Now called initializeCache().
> 517 private void resetDirs() throws IOException {
>
> Perhaps "cleanupDirs" or "cleanupCache", or "removeCache".
Now called removeCache().
> 547 private void uninstallJams() throws IOException {
>
> I don't seem to find where this method get used. Could it be removed?
Definitely! It was used at one point, and I forgot to remove it. Done.
> - src/share/classes/sun/module/repository/RepositoryConfig.java
>
> 489 + ": " + msg);
>
> Missing "'" before ": ".
You're eyesight is much better than mine: Fixed.
> - src/share/classes/sun/module/repository/URLRepository.java
>
> 675 public synchronized List<ModuleDefinition>
> findModuleDefinitions(Query constraint) {
>
> Same comment as in LocalRepository.
>
> 774 String getSourcePath() {
>
> Don't see it being used. Can it be removed?
It's used in URLModuleDefinitionContent.
> 812 throw new NullPointerException("URLRepository: config
> cannot be null.");
> 846 "URLRepository: In repository " + getSourceLocation()
> 877 throw new IOException("URLRepository: Cannot open
> connection to " + moduleMD, ex);
> 885 throw new IOException("URLRepository: Cannot load
> MODULE.METADATA for " + moduleMD, ex);
>
> Prefix issue in exception's message.
Fixed similarly to LocalRepository.
> 1087 private void initializeDirs() throws IOException {
> 1111 private void resetDirs() throws IOException {
>
> See above.
Fixed as in LocalRepository.
> 1122 metadataDir = null;
> 1123 downloadDir = null;
>
> Shouldn't baseDir also be reset to null?
Fixed.
> - src/share/classes/sun/module/repository/ExtensionRepositoryFactory.java
>
> I think you might mean global repository instead of extension repository.
>
> 54 locations.put("linux", "/usr/jdk/packages/lib/ext");
> 55 locations.put("solaris", "/usr/java/packages/lib/ext");
> 56 locations.put("windows", "%SystemRoot%\\Sun\\Java\\lib\\ext");
>
> These locations are where the global legacy extension JARs are stored,
> but they're not for modules. For the extension repository, the location
> should be {java.home}/lib/module/ext on all platforms.
I changed repository.properties to accomplish this.
> If you want to add the support for the global repository, the location
> would be the following for now:
>
> Linux: /usr/jdk/packages/lib/module
> Solaris: /usr/java/packages/lib/module
> Windows: %SystemRoot%\\Sun\\Java\\lib\\module
>
> That said, the location should be specified in the repository.properties
> rather than hardcoded. See below.
I have the locations both hardcoded and in repository.properties; the latter
take precedence over the former.
> 71 return Modules.newLocalRepository(
>
> The extension repository should be a file-based URL repository for
> performance reason, while the global repository should be a local
> repository.
Done, via classname in repository.properties
> - src/share/lib/module/repository.properties
>
> 32 extension.parent=bootstrap
> 33 extension.factoryname=sun.module.repository.ExtensionRepositoryFactory
>
> 34
> 35 user.parent=extension
> 36 user.factoryname=sun.module.repository.UserRepositoryFactory
>
> extension.parent=bootstrap
> extension.factoryname=sun.module.repository.ExtensionRepositoryFactory
> extension.source.location=file:///{java.home}/lib/module/ext
> global.parent=extension
> global.factoryname=sun.module.repository.GlobalRepositoryFactory
> global.source.location=/usr/jdk/packages/lib/module (note: varies
> between platforms)
> user.parent=global
> user.factoryname=sun.module.repository.UserRepositoryFactory
> user.source.location=file:///{user.home}/.java/module
Fixed in principle, though the result is a little different. We explicitly
don't use the form of the source given for the user repository because that
results in the error situation we discussed with Alan a while back.
Webrev is updated:
http://analemma.sfbay.sun.com/java/jdk/ws/libs/rev/6559067+6559086+6605100/
Thanks again for the extensive feedback,
Dave
> That's it for now.
>
> - Stanley
>
>
> Dave Bristor wrote:
>> Hi folks,
>>
>> This fixes 3 bugs:
>> (repo) implement LocalRepository.shutdown() and reload()
>> http://monaco.sfbay/detail.jsp?cr=6559067
>>
>> (repo) URLRepository.shutdown() and reload()
>> http://monaco.sfbay/detail.jsp?cr=6559086
>>
>> (repo) Allow factories in repository configuration
>> http://monaco.sfbay/detail.jsp?cr=6605100
>>
>> The webrev is at:
>>
>> http://analemma.sfbay.sun.com/java/jdk/ws/libs/rev/6559067+6559086+6605100/
>>
>>
>> There are lots of changes, so a description of them is in order:
>>
>> * LocalRepository and URLRepository suffered from entropy, so I moved
>> code around and brought them up to coding standards (I'm sure some
>> things are still missed in that regard).
>>
>> * In discussion, Kumar, Stanley, and I decided that the configuration
>> of those 2 repositories should be via system properties and the
>> Map<String, String> given when they're constructed. The
>> repository-specific system propertes are read once, when the class is
>> loaded. Their values *override* any that could be given at
>> construction. BasicLauncherTests was changed to use overrides via
>> system properties; Local and URL repository tests use overrides via
>> the Map<String, String>.
>>
>> * When one of those repositories is created, it is OK if its source
>> location does not exist. In this case, when find() or list()
>> operations are performed, they'll return empty lists. If reload() is
>> performed, and at that time the source location *does* exist, then
>> subsequent find() or list() operations will return modules now
>> available. This is verified in the Local and URL repository tests.
>> Checks for the prior, opposite behavior were removed from these and
>> from RepositoryTest.
>>
>> * However, we provide a system property for each of these repository
>> types that can cause construction/initialization to require that the
>> source location *does* exist at that time; if not an IOException gets
>> thrown. LocalRepositoryTest verifies this.
>>
>> * shutdown() and reload() are implemented for both repositories, and
>> are verified in Local and URL repository tests.
>>
>> * For these repository implementations: once an instance has been
>> initialized, subsequent initialize() invocations are a no-op. Once an
>> instance has been initialized and then shutdown, subsequent shutdown()
>> invocations are a no-op. But if an instance is initialized, then
>> shutdown, a subsequent initialize() invocation throws
>> IllegalStateException. Local and URL repository tests were updated to
>> check this.
>>
>> * RepositoryConfig now supports configuration via RepositoryFactory
>> objects. See src/share/lib/module/repository.properties and
>> src/share/classes/sun/module/repository/ExtensionRepositoryFactory for
>> an example. A new test was added for factories.
>>
>> Thanks,
>> Dave
>> _______________________________________________
>> modules-dev mailing list
>> modules-dev at openjdk.java.net
>> http://mail.openjdk.java.net/mailman/listinfo/modules-dev
More information about the modules-dev
mailing list