[modules-dev] [Fwd: [Fwd: Review request: 6563535, URLRepository.install]]

[Dave Bristor]

6563535 was checked in last week after Kumar's review.  I made a webrev 
comparing changes based on your feedback with what's checked in:

http://analemma.sfbay.sun.com/java/jdk/ws/libs/rev/6563535-b

Tested on Solaris and Windows, and I'll start a JPRT run.

Please note that the review request I sent for 6589789 will have to be updated 
for these changes!

Thanks,
	Dave

Stanley M. Ho wrote:
> Hi Dave,
> 
> Looks good. Several comments:
> 
> - src/share/classes/sun/module/repository/URLRepository.java
> 
> 563         if (!(srcJamFileName.endsWith(".jam")  || 
> srcJamFileName.endsWith(".jam.pack.gz"))) {
> 
> Should also support .jar.
> 
>  630         // Refuse to install if comparable module is already 
> installed.
>  631         // XXX Use platform, arch when available from metadata.
>  632         List<ModuleDefinition> defns = findModuleDefinitions(
>  633             Query.and(Query.name(moduleName), 
> Query.version(moduleVersion)));
> 
> findModuleDefinitions() will not return platform specific module 
> definitions, and this logic will reject installation of platform 
> specific modules that do not match the running platform. What you want 
> to check is probably the list of ModuleArchiveInfo instead.

Fixed.

> 
> 657         FileOutputStream fos = new FileOutputStream(destMDFile);
> 
> Use BufferedOutputStream.

Fixed there, and when writing an otherwise non-existent 
repository-metadata.xml file.

> 
> 688             contents.put(mai, installedMD);
> 
> If exception is thrown in the try/catch block, the contents will be 
> updated incorrectly. Even contents is updated again in the catch block, 
> there is still a small window that contents does not represent the 
> correct information. Instead, this should be updated in a finally block.

If it's in the finally block, it will always get updated.  But if an exception 
is thrown, then we don't want it to get updated, which is what we need.

Or do you mean there should be a try/catch within the catch of the 
IOException, so that contents.remove(mai) in a finally?  I made this change; 
please let me know if it meets your intent.

>  717         ModuleDefinition md = contents.get(mai);
>  718         if (md == null) {
>  719             rc = false;
> 
> If the module definition is platform specific but does not match the 
> current platform, the above logic won't allow it to be removed from the 
> repository.

Fixed, I think: see if you agree.

> 
> 748             System.err.println("### jamFile: " + 
> jamFile.getCanonicalPath());
> 
> Remove.

Oops!  Removed.

>  752              * them.  If any rename or deletion fails, undo 
> whatever can be
>  753              * undone. */
> 
> If writeRepositoryMetadata in 773 throws exception, it is unclear if the 
> unwinding will be done properly.

Improved.  And for some of the subsequent unwinding also, re-renameing files.

>  805     private void writeRepositoryMetadata(ModuleArchiveInfo mai) 
> throws IOException {
> 
> It never uses specified mai when writing the metadata.

But it does when providing error messages in exceptions.

> 
> - Stanley
> 
> 
> 
> 
> 
>