RFR: jmod remove

[Paul Sandoz]

On Apr 26, 2012, at 5:18 PM, Jonathan Gibbons wrote:

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

Yes, that would fit better since this is an exceptional case, rather than the caller having to check for a null for a non-exceptional case.

It's annoying that potentially one reason why this bubbles out at the API is that installation can result in stuff being installed out of the scope of the library itself, which is not the common case.

For the common case, at least with this implementation, the internal directory for the module can be moved to a trash directory [*], and if deletion then fails then it can be retried later when another operation on the library is performed (kind of like how weak hash map cleans up keys).

Paul.

[*] assuming move is an atomic operation

> -- 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.
>>>> 
>>>> 
>