RFR: JDK-8232092 (fs) Files::isWritable returns false on a writeable root directory (win) [v8]

Nhat Nguyen github.com+8022952+nhat-nguyen at openjdk.java.net
Wed Oct 7 17:33:11 UTC 2020


On Wed, 7 Oct 2020 07:42:55 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>>> The updated test looks much better and passes in our infrastructure since the remove of the toRealPath test. I'm still
>>> bothered by the hardcoded "X:". Does subst (and hence substCreate) fail if that volume already exists?
>> 
>> subst will fail if that volume already exists. I have added a method to find any available drive to be used with subst;
>> it simply tries calling substCreate with drive letters from Z down to A. If none is available, it will throw a
>> SkipException to skip the whole suite.
>>> substList can be changed to return List.
>> 
>> What is your opinion on substList returning Stream<String> like you mentioned before? I think it is perhaps cleaner
>> than List<String> because I would call .anyMatch() which is a stream API on the return value of substList.
>
> I see the latest version will attempt to find an unused driver letter. I'll try that out now in our infrastructure to
> make sure that it is stable (Update: It passes on our infrastructure as it always find an unused drive so I think this
> aspect of the patch is good).  substList returning a Stream is good but it should be Stream<String> rather than just
> "Stream". Also the method name and comment suggests is returns a List, maybe it should just be renamed to mappedDrives
> to be a bit clearer. The new findAvailableDrive is also missing the type variable, I think you want Optional<Path> here.

> substList returning a Stream is good but it should be Stream rather than just "Stream". Also the method name and
> comment suggests is returns a List, maybe it should just be renamed to mappedDrives to be a bit clearer. The new
> findAvailableDrive is also missing the type variable, I think you want Optional here.

Thanks Alan for the feedback. I think there's a problem with how webrev is being generated. I have been a bit confused
by your comments on the usage of raw types because I actually have them from the beginning. I just double checked the
webrevs and they are indeed missing. However, if you look at the Github diff, you will see that I'm using both
`Stream<String>` and `Optional<Path>`.

-------------

PR: https://git.openjdk.java.net/jdk/pull/287


More information about the nio-dev mailing list