RFR: jmod remove

[Chris Hegarty]

sorry, clicked send too quick...

In the webrev, remove still throws IOException if there is a problem 
when accessing the library to determining if removal would break the 
configuration. This is why it may (or may not) look funny that the 
result value is a list of IOExceptions as well as throwing IOException. 
I think it is best to differentiate between these cases.

-Chris.

On 26/04/2012 16:38, Chris Hegarty wrote:
> 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.
>>
>>