RFR: jmod remove

[Chris Hegarty]

just to add...

I extended the test to cover the case of content installed outside the 
module library, and there some issues with newline/whitespace in the 
output of 'jmod ls' causing problem on Cygwin. The test should be bullet 
proof now!

-Chris.

On 09/05/12 15:31, Chris Hegarty wrote:
> Alan,
>
> I finally got back to this. Comments inline...
>
> Updated webrev:
> http://cr.openjdk.java.net/~chegar/jigsaw/jmodrm.05/webrev/
>
>> comment on moduleTrashDir is that it duplicates the logic for temporary
>> directories and maybe it would be simper to use use
>> Files.createTempDirectory to create a directory in the trash directory.
>
> Right, there was an error in my initial change, we only want to generate
> a unique name here not actually create the directory. If the directory
> is created there could be a problem when doing the move. I encountered
> this on some Windows systems.
>
> I retained the logic ( same as createTempDirectory ) to generate the
> name, but it could use UUID or similar.
>
>> A few minor comments:
>>
>> Library.java defines the mids parameter, not not the dry parameter.
>
> Got it, thanks.
>
>> SimpleLibrary.remove, you can use try-with-resources at L1501.
>
> Done.
>
>> checkRootsRequire - I wonder if we can find a better name for this
>> method, the logic seems right and should work for service usage too but
>> the name of the method seems a bit odd. Maybe
>> ensureNotPresentInConfiguration or something short?
>
> I went with ensureNotInConfiguration.
>
>> Just on consistency of naming, then with the change then Files defines
>> both deleteTree and deleteAllUnchecked to do a recursive delete which
>> seems a bit inconsistent.
>
> I renamed this to deleteTreeUnchecked. I kept deleteTreeUnchecked as it
> makes more of an effort to actually delete all the files. Eventually, I
> think we should remove deleteTree and replace it with
> deleteTreeUnchecked, but this could be done separately as there may be
> more discussion around it.
>
>> Shouldn't Files. deleteUnchecked just return any exception thrown by
>> delete rather than dropping the useful exception that delete throws?
>
> Reverted to Files.delete and catch the exception.
>
>> Nothing really to do with this patch but ModuleFile.remove(File) could
>> use Files.readAllLines which would remove much of the code.
>
> Thanks, done.
>
> -Chris.
>
>> Aside from a bit of clean-up then I think this will be great to have
>> (and urgently needed now because %mids means that we can't rm modules
>> anymore).
>>
>> -Alan
>>