[modules-dev] Review request for 6559067, 6559086, and 6605100
Stanley M. Ho
Stanley.Ho at Sun.COM
Tue Oct 9 19:23:15 PDT 2007
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.
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.
261 "Cannot overwrite " + f.getName() + " in repository
directory " + sourceDir);
"in repository's source directory" instead of "in repository directory".
278 public synchronized boolean uninstall(ModuleArchiveInfo m)
throws IOException {
Should check if repository is read-only.
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.
295 if (f2.isDirectory()) {
What happens if f2 is a file?
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);
}
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.
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();
495 private void initializeDirs() throws IOException {
Perhaps "constructDirs", "constructCache", or "initializeCache".
517 private void resetDirs() throws IOException {
Perhaps "cleanupDirs" or "cleanupCache", or "removeCache".
547 private void uninstallJams() throws IOException {
I don't seem to find where this method get used. Could it be removed?
- src/share/classes/sun/module/repository/RepositoryConfig.java
489 + ": " + msg);
Missing "'" before ": ".
- 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?
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.
1087 private void initializeDirs() throws IOException {
1111 private void resetDirs() throws IOException {
See above.
1122 metadataDir = null;
1123 downloadDir = null;
Shouldn't baseDir also be reset to null?
- 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.
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.
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.
- 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
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