[modules-dev] Initial repository management tool

[Dave Bristor]

Andreas Sterbenz wrote:
> Dave Bristor wrote:
> 
>>
>> Webrev updated, includes test
>>
>> http://javaweb.sfbay/java/jdk/ws/libs/rev/6559117/
> 
> 
> Looks fine. Some minor comments:
> 
>  . ToolBase should be declared abstract for clarity

Fixed.

> 
> JRepo:
> 
>  . DEBUG is true in the webrev

Fixed.

>  . the Boolean.getBoolean() code in the comments can fail if there is a 
> security manager

Hmm, I had the security manager at one point, anyway it's back now: thanks.

>  . commands and JRepo constructor: it would seem clearer to me if the 
> code to initialize the static 'commands' Map was in a static initializer 
> rather than under a check in the constructor

Since Command is not static, a "this" is required for their construction, so 
they can't be done statically.  Command is not static because it makes use of 
ToolBase.usageError, etc.  Perhaps it would be better to have "ToolHelper" 
(name?), and have JRepo create an instance of it.  It's not much of a class 
and is only valuable *if* we convert Jam.java to also use it.  What do you 
think?  I'm starting to tend that way...

>  . it seems a little confusing for the Command constructor to have a 
> side effect of registering the class. Maybe there should be a static 
> register method somewhere that takes a Command object?

Fixed.

>  . getRepository(): wouldn't the fix for the XXX be to do the "new 
> URL()", and if that succeeds, construct a URLRepository; if it throws an 
> Exception, use a LocalRepository? If those constructors fail, propagate 
> the exceptions.

I think that's what it does now.  If "new URL" fails (e.g. 
MalformedURLException), we'll try a LocalRepository; similarly if 
Modules.newURLRepository fails.  But I did add a check: if repositoryLocation 
is not a well-formed URL, and it's not an accessible file, throw an I/O 
exception immediately.

>  . getMAIText(): should this really say "platform?" if platform is null, 
> etc? Rather than an empty string, or "generic", or "n/a"?

I like "generic" for platform and arch, if for no other reason than I'm used 
to it in bugster.  Do you have a suggestion for time and file?  I kind of want 
to convey what it is that's unknown.  I left them as-is for now, but am open 
to suggestions.  Perhaps it would help if there were column headings?

>  . the distinction between "list" and "find" (and "query") seems rather 
> subtle. Do we need those as separate commands? Or could module name and 
> version merely be optional parameters?

I used ruby "gems" as a model, as suggested by Stanley and the EG:
     http://rubygems.org/read/chapter/2#page3
     http://docs.rubygems.org/read/book/2
Gem has separate commands for list vs. query.  Unix, etc. have directory 
listing sepate from directory searching.  That's the rationale.  Conceptually 
listing and findind seem separate to me (my $0.02).


>  . I assume you are updating make/sun/Makefile to build make/sun/jrepo?

I'd missed that, thanks for noting it.

Webrev updated, thanks for the feedback!

	Dave

> 
> Andreas.