JDK-8232092 (fs) Files::isWritable returns false on a writeable root directory (win) #287

nhat-nguyen · 2020-09-21T19:30:44Z

I'm following up on the reviewing thread here: https://mail.openjdk.java.net/pipermail/nio-dev/2020-August/007456.html

All the tests that create symlinks are now properly checked against using TestUtil.supportsLinks

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

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

Reviewers

Alan Bateman (@AlanBateman - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/287/head:pull/287
$ git checkout pull/287

…y (win)

bridgekeeper · 2020-09-21T19:32:35Z

Hi @nhat-nguyen, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user nhat-nguyen" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

nhat-nguyen · 2020-09-21T19:32:52Z

/covered

openjdk · 2020-09-21T19:34:21Z

@nhat-nguyen The following label will be automatically applied to this pull request: nio.

When this pull request is ready to be reviewed, an RFR email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label (add|remove) "label" command.

bridgekeeper · 2020-09-21T19:34:47Z

Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

mlbridge · 2020-09-21T22:58:36Z

Webrevs

AlanBateman

The update to WindowsFileStore looks okay.

The test has good coverage but I worry that creating X: will not be reliable in some CI environments, esp. if X: exists already or permissions are needed. I'll try your test in our CI and would be good if others did the same.

One terminology nit is that the file system API uses the term "directory". The test uses a mix of "directory" and "folder" would be good to keep it consistent if possible.

I don't think the addition to TestUtil is needed, the createRootTempDirectory method can call Files.createTmpDirectory.

Many of the existing TestNG tests import statically Asserts and could be done here too.

substList should return Stream, we try to avoid raw types.

No need to import java.lang.ProcessBuilder, also would be good to clean import the imports, no need for a space between each java.* package.

mlbridge · 2020-09-24T12:27:08Z

Mailing list message from Alan Bateman on nio-dev:

On 24/09/2020 10:52, Alan Bateman wrote:

:
The test has good coverage but I worry that creating X: will not be reliable in some CI environments, esp. if X: exists
already or permissions are needed. I'll try your test in our CI and would be good if others did the same.

I tried it in our CI and testRealPath fails consistently on all Windows
clients.? This is JDK-8213216 rather than JDK-8232092. I know Brian was
looking at JDK-8213216 at one point but we didn't come to a conclusion
on supporting the scenario without creating inconsistencies. So maybe
this test should be dropped from SubstDrive.

-Alan

nhat-nguyen · 2020-10-05T18:59:10Z

Mailing list message from Alan Bateman on nio-dev:

On 24/09/2020 10:52, Alan Bateman wrote:

:
The test has good coverage but I worry that creating X: will not be reliable in some CI environments, esp. if X: exists
already or permissions are needed. I'll try your test in our CI and would be good if others did the same.

I tried it in our CI and testRealPath fails consistently on all Windows
clients.? This is JDK-8213216 rather than JDK-8232092. I know Brian was
looking at JDK-8213216 at one point but we didn't come to a conclusion
on supporting the scenario without creating inconsistencies. So maybe
this test should be dropped from SubstDrive.

-Alan

Thank you Alan for catching this. This is an oversight on my end. I wrote testRealPath with the intention that if Brian ever fixes jdk-8213216, he can use this test; unfortunately I forgot to disable it. I have dropped this test case from the test altogether to prevent any future confusion.

One terminology nit is that the file system API uses the term "directory". The test uses a mix of "directory" and "folder" would be good to keep it consistent if possible.

Fixed!

I don't think the addition to TestUtil is needed, the createRootTempDirectory method can call Files.createTmpDirectory.

Fixed!

Many of the existing TestNG tests import statically Asserts and could be done here too.

Fixed!

No need to import java.lang.ProcessBuilder, also would be good to clean import the imports, no need for a space between each java.* package.

Fixed!

substList should return Stream, we try to avoid raw types.

I'm sorry I'm a bit confused by this. Did you mean to say "should not return stream"? substList returns Stream and not raw Stream. If the convention is try not to return Stream<> at all, I have converted the method to return List<> instead (I made substList return Stream in the first place because I wanted to call anyMatch() on the return result). Please let me know if this is what you expected. 😄

mlbridge · 2020-10-06T06:00:39Z

Mailing list message from Alan Bateman on nio-dev:

On 05/10/2020 20:02, Nhat Nguyen wrote:

:
I'm sorry I'm a bit confused by this. Did you mean to say "should not return stream"? substList returns Stream<String>
and not raw Stream. If the convention is try not to return Stream<> at all, I have converted the method to return
List<> instead (I made substList return Stream in the first place because I wanted to call anyMatch() on the return
result). Please let me know if this is what you expected. ??

My comment wasn't clear. I was pointing to the return type of substList.
It was a Stream in the initial patch and I was just pointing out that it
should be Stream<String>.

-Alan

AlanBateman · 2020-10-06T14:17:41Z

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?

substList can be changed to return List.

This reverts commit 43cd87b.

nhat-nguyen · 2020-10-06T19:16:38Z

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 like you mentioned before? I think it is perhaps cleaner than List because I would call .anyMatch() which is a stream API on the return value of substList.

AlanBateman · 2020-10-07T07:42:55Z

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

nhat-nguyen · 2020-10-07T17:29:56Z

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 webrevs are 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>. It also seems the comments on Github may also be incorrectly escaped, as <whatever> doesn't render unless I wrap it in backticks.

AlanBateman · 2020-10-07T18:21:38Z

Indeed, it seems to be interpreting the tags as if they are HTML so looked to me that you were using raw types everyone. Sorry I didn't notice this.

The updated changes look good and happy to sponsor.

openjdk · 2020-10-07T18:23:44Z

@nhat-nguyen This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8232092: (fs) Files::isWritable returns false on a writeable root directory (win)

Reviewed-by: alanb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 321 new commits pushed to the master branch:

38159d5: 8253876: jdk/test/lib/hexdump/ASN1FormatterTest.java fails with "AssertionError: Lines expected [126] but found [202]"
db3053d: 8067127: Tags cleanup
739347f: 8254168: Remove TemplateTable::count_calls
4fe07cc: 8252324: Signal related code should be shared among POSIX platforms
1e8e543: 8216497: javadoc should auto-link to platform classes
04ca660: 8253874: [JVMCI] added test omitted in 8252881
49128a1: 8253475: Javadoc clean up in HttpExchange and HttpServer
9543d76: 8253000: Remove redundant MAKE_SUBDIR argument
4dfa411: 8252997: Null-proofing for linker_md.c
97ff38c: 8254141: Typo in copyright year
... and 311 more: https://git.openjdk.java.net/jdk/compare/b1b0f0b2ccfa199b00a4e2398f4a228a9f467e98...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@AlanBateman) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

nhat-nguyen · 2020-10-07T18:34:00Z

Indeed, it seems to be interpreting the tags as if they are HTML so looked to me that you were using raw types everyone. Sorry I didn't notice this.

The updated changes look good and happy to sponsor.

Thank you for all the suggestions!

nhat-nguyen · 2020-10-07T18:36:50Z

/integrate

openjdk · 2020-10-07T18:38:35Z

@nhat-nguyen
Your change (at version 2c8844c) is now ready to be sponsored by a Committer.

AlanBateman · 2020-10-07T19:43:41Z

/sponsor

openjdk · 2020-10-07T19:45:45Z

@AlanBateman @nhat-nguyen Since your change was applied there have been 322 commits pushed to the master branch:

5a9bd41: 8254102: use ProcessHandle::pid instead of ManagementFactory::getRuntimeMXBean to get pid in tests
38159d5: 8253876: jdk/test/lib/hexdump/ASN1FormatterTest.java fails with "AssertionError: Lines expected [126] but found [202]"
db3053d: 8067127: Tags cleanup
739347f: 8254168: Remove TemplateTable::count_calls
4fe07cc: 8252324: Signal related code should be shared among POSIX platforms
1e8e543: 8216497: javadoc should auto-link to platform classes
04ca660: 8253874: [JVMCI] added test omitted in 8252881
49128a1: 8253475: Javadoc clean up in HttpExchange and HttpServer
9543d76: 8253000: Remove redundant MAKE_SUBDIR argument
4dfa411: 8252997: Null-proofing for linker_md.c
... and 312 more: https://git.openjdk.java.net/jdk/compare/b1b0f0b2ccfa199b00a4e2398f4a228a9f467e98...master

Your commit was automatically rebased without conflicts.

Pushed as commit abe2593.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

sormuras · 2020-10-07T20:06:35Z

Wohoo! Next Windows-related bug down.

mlbridge · 2020-10-08T08:51:42Z

Mailing list message from Alan Bateman on nio-dev:

Nhat,

The test is failing intermittently, tracked as JDK-8254195. I didn't see
this when testing your change before sponsoring.

I think the issue is that there are several tests running at the same
time so the unallocated and unusable values won't be stable. I think we
can drop those from the test.

-Alan

[1] https://bugs.openjdk.java.net/browse/JDK-8254195

8232092: Files::isWritable returns false on a writeable root director…

0bfe917

…y (win)

bridgekeeper bot added the oca Needs verification of OCA signatory status label Sep 21, 2020

openjdk bot added the nio nio-dev@openjdk.org label Sep 21, 2020

bridgekeeper bot added the oca-verify Needs verification of OCA signatory status label Sep 21, 2020

Fix typo

0b89766

bridgekeeper bot removed oca Needs verification of OCA signatory status oca-verify Needs verification of OCA signatory status labels Sep 21, 2020

openjdk bot added the rfr Pull request is ready for review label Sep 21, 2020

AlanBateman reviewed Sep 24, 2020

View reviewed changes

nhat-nguyen added 9 commits October 5, 2020 10:56

Change usages of "folder" to "directory"

c6b539a

Revert TestUtil changes and use Files.createTempDirectory directly

ac1049e

Drop testRealPath (JDK-8213216)

ef2e1d5

Use static import for testng.Assert methods

a0394a0

Remove import of ProcessBuilder

241856c

Remove redundant new lines between java imports

772b858

Remove duplicate import

0c8da58

Remove unused import

c6e1804

Remove usage of stream

43cd87b

nhat-nguyen added 3 commits October 6, 2020 10:40

Revert "Remove usage of stream"

beb7e90

This reverts commit 43cd87b.

Remove import of List

3aa87ef

Add mechanism to find available drive and skip test if none is available

d9888e2

Update method name so that it's clearer

2c8844c

AlanBateman approved these changes Oct 7, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 7, 2020

openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 7, 2020

openjdk bot closed this Oct 7, 2020

openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 7, 2020

JDK-8232092 (fs) Files::isWritable returns false on a writeable root directory (win) #287

JDK-8232092 (fs) Files::isWritable returns false on a writeable root directory (win) #287

nhat-nguyen commented Sep 21, 2020 •

edited by openjdk bot

bridgekeeper bot commented Sep 21, 2020

nhat-nguyen commented Sep 21, 2020

openjdk bot commented Sep 21, 2020

bridgekeeper bot commented Sep 21, 2020

mlbridge bot commented Sep 21, 2020 •

edited

AlanBateman left a comment

mlbridge bot commented Sep 24, 2020

nhat-nguyen commented Oct 5, 2020 •

edited

mlbridge bot commented Oct 6, 2020

AlanBateman commented Oct 6, 2020

nhat-nguyen commented Oct 6, 2020

AlanBateman commented Oct 7, 2020 •

edited

nhat-nguyen commented Oct 7, 2020 •

edited

AlanBateman commented Oct 7, 2020

openjdk bot commented Oct 7, 2020 •

edited

nhat-nguyen commented Oct 7, 2020

nhat-nguyen commented Oct 7, 2020

openjdk bot commented Oct 7, 2020

AlanBateman commented Oct 7, 2020

openjdk bot commented Oct 7, 2020

sormuras commented Oct 7, 2020

mlbridge bot commented Oct 8, 2020

JDK-8232092 (fs) Files::isWritable returns false on a writeable root directory (win) #287

JDK-8232092 (fs) Files::isWritable returns false on a writeable root directory (win) #287

Conversation

nhat-nguyen commented Sep 21, 2020 • edited by openjdk bot

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Sep 21, 2020

nhat-nguyen commented Sep 21, 2020

openjdk bot commented Sep 21, 2020

bridgekeeper bot commented Sep 21, 2020

mlbridge bot commented Sep 21, 2020 • edited

Webrevs

AlanBateman left a comment

Choose a reason for hiding this comment

mlbridge bot commented Sep 24, 2020

nhat-nguyen commented Oct 5, 2020 • edited

mlbridge bot commented Oct 6, 2020

AlanBateman commented Oct 6, 2020

nhat-nguyen commented Oct 6, 2020

AlanBateman commented Oct 7, 2020 • edited

nhat-nguyen commented Oct 7, 2020 • edited

AlanBateman commented Oct 7, 2020

openjdk bot commented Oct 7, 2020 • edited

nhat-nguyen commented Oct 7, 2020

nhat-nguyen commented Oct 7, 2020

openjdk bot commented Oct 7, 2020

AlanBateman commented Oct 7, 2020

openjdk bot commented Oct 7, 2020

sormuras commented Oct 7, 2020

mlbridge bot commented Oct 8, 2020

nhat-nguyen commented Sep 21, 2020 •

edited by openjdk bot

mlbridge bot commented Sep 21, 2020 •

edited

nhat-nguyen commented Oct 5, 2020 •

edited

AlanBateman commented Oct 7, 2020 •

edited

nhat-nguyen commented Oct 7, 2020 •

edited

openjdk bot commented Oct 7, 2020 •

edited