RFR: jmod remove

[Chris Hegarty]

Alan, thanks for you comments,

Updated webrev:
   http://cr.openjdk.java.net/~chegar/jigsaw/jmodrm.03/webrev/

You may be able to come up with better wording for the Library spec 
changes. Also, please correct me if I've used incorrect terms here.

I addressed all of your specific comments. The implementation now 
captures all IOExceptions thrown during removal and will return a 
List<IOException> (so tools/etc can handle as required). For jmod I 
simply output a warning, but more importantly the library should be kept 
in the most consistent state (as possible) during removal. Note: I did 
consider suppressed exceptions here, but felt returning a list was more 
appropriate for tools to handle as appropriate.

-Chris.


On 19/04/2012 12:52, Alan Bateman wrote:
> On 19/04/2012 09:47, Chris Hegarty wrote:
>> I heard the following expression a few times, 'rm -rf has reached the
>> limit of its usefulness'! So it's about time we had a 'jmod remove'.
>>
>> The webrev is based upon Mandy's alias support patch [1]. I figured it
>> would most likely be going in after the alias support.
>>
>> http://cr.openjdk.java.net/~chegar/jigsaw/jmodrm.02/webrev/
>>
>> Essentially, you can remove any module not required by a
>> configuration. Fails if you try to remove a module which will break a
>> configuration, unless you specify --force, or remove both the root
>> module along with the dependent module.
>>
>> I added a comment about the parent/child relationship. That is, a
>> module library has no notion of its children so we could remove a
>> module in a parent library that is not part of any configuration in
>> that library, but may be part of a configuration in a child library. I
>> think this is fine for now, but we may have to revisit the
>> parent/child relationship in the future ( beyond the scope of jmod
>> remove ).
> Thanks for taking this one, I agree it's very useful to have remove
> implemented even with the issue of child repositories.
>
> I've looked through the changes and overall the approach seems okay. A
> couple of comments on the webrev:
>
> - I think we should consider having Library define the remove method (as
> an abstract method of course).
>
> - On the javadoc then adding @throws ConfigurationException and
> IOException would be useful (it wasn't obvious to me initially that a
> ConfigurationException is thrown when module can't be removed because it
> exists in some configuration). Also it javadoc should make it clear that
> IOException may be thrown after some, but not all, modules have been
> removed.
>
> - In Librarian's static initializer then it would be great to have "rm"
> map to Remove.class too, my preference is "jmod rm" over "jmod remove".
>
> - Suppose that there is an error after one or more modules have been
> removed, shouldn't we still refresh and update the module directory?
> We'll need to make the refresh very robust as bad things will happen in
> the wild.
>
> - removeWhenLocked doesn't check the return from delete so it may
> silently fail (Files.delete is probably called for here as it will give
> the right exception).
>
> - Should the dry run print a message so that it's clear what will be
> removed? Also having the dry run recover from bad input would be useful
> too.
>
> - SimpleLibrary L1487, typo "forceably" -> "forcibly".
>
> - SimpleLibrary L1507, "FileChannel lc" -> "FileChannel fc".
>
> - It might be better to rename the test to jmod-rm.sh to be consistent
> with the other tests. Also would be possible to include a test that uses
> views so that it works as expected.
>
> Otherwise thanks again for taking this one.
>
> -Alan.
>
>