RFR: jmod remove

[Jonathan Gibbons]

How about throwing an exception containing a List<IOException> for that 
case?

-- Jon


On 04/26/2012 08:54 AM, Chris Hegarty wrote:
> 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.
>>>
>>>