Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows implementation of feature path_try_exists #85060

Merged
merged 2 commits into from
May 21, 2021

Conversation

ChrisDenton
Copy link
Member

Draft of a Windows implementation of try_exists (#83186).

The first commit reorganizes the code so I would be interested to get some feedback on if this is a good idea or not. It moves the Path::try_exists function to fs::exists. leaving the former as a wrapper for the latter. This makes it easier to provide platform specific implementations and matches the fs::metadata function.

The other commit implements a Windows specific variant of exists. I'm still figuring out my approach so this is very much a first draft. Eventually this will need some more eyes from knowledgable Windows people.

@rust-highfive
Copy link
Collaborator

r? @yaahc

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 7, 2021
@yaahc
Copy link
Member

yaahc commented May 12, 2021

Hey @ChrisDenton, thank you for the PR and I'm sorry for the slow reply. I've taken a look, and admittedly I'm no Windows expert, but everything here looks good to me. I really appreciate the comments btw, they made reviewing this PR much easier. For a real Windows expert you could try asking on Zulip (in #t-libs or #t-compiler/windows) first. If that doesn't work, we can ping a few Windows experts on GitHub here.

@ChrisDenton
Copy link
Member Author

@yaahc No worries, I'm not in a rush! I've asked in the Windows group if someone wouldn't mind taking a look. I think there are a few changes I'd like to make but I'd prefer to wait for a second opinion first.

@ChrisDenton
Copy link
Member Author

A Microsoft engineer expressed the concern that using GetFileAttributesW as an optimization relied on OS implementation details that may change. So I've simplified the second commit to merely try opening the file. The first commit remains the same.

I think this is now ready for review.

@ChrisDenton ChrisDenton marked this pull request as ready for review May 19, 2021 16:38
@yaahc
Copy link
Member

yaahc commented May 19, 2021

Looks good, thank you.

@bors r+

@bors
Copy link
Contributor

bors commented May 19, 2021

📌 Commit 5d7c75a has been approved by yaahc

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 19, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 19, 2021
Windows implementation of feature `path_try_exists`

Draft of a Windows implementation of `try_exists` (rust-lang#83186).

The first commit reorganizes the code so I would be interested to get some feedback on if this is a good idea or not. It moves the `Path::try_exists` function to `fs::exists`. leaving the former as a wrapper for the latter. This makes it easier to provide platform specific implementations and matches the `fs::metadata` function.

The other commit implements a Windows specific variant of `exists`. I'm still figuring out my approach so this is very much a first draft. Eventually this will need some more eyes from knowledgable Windows people.
@RalfJung
Copy link
Member

Failed in rollup:
#85483 (comment)
@bors r- rollup=iffy

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 19, 2021
@ChrisDenton
Copy link
Member Author

Oh, sorry. I made a silly mistake there. I didn't catch it locally because I wasn't testing a target with an unsupported std::fs. I'll push a fix soon.

@yaahc
Copy link
Member

yaahc commented May 20, 2021

@bors r+

@bors
Copy link
Contributor

bors commented May 20, 2021

📌 Commit 86dbc06 has been approved by yaahc

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 20, 2021
@bors
Copy link
Contributor

bors commented May 21, 2021

⌛ Testing commit 86dbc06 with merge f36b137...

@bors
Copy link
Contributor

bors commented May 21, 2021

☀️ Test successful - checks-actions
Approved by: yaahc
Pushing f36b137 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 21, 2021
@bors bors merged commit f36b137 into rust-lang:master May 21, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 21, 2021
@ChrisDenton ChrisDenton deleted the win-file-exists branch May 21, 2021 13:25
@ijackson
Copy link
Contributor

@ChrisDenton Hi. I'm doing some work on io::ErrorKind in #79965 which involved adding many more Windows error codes. My bulk import (from mingw) discovered that your definition of ERROR_SHARING_VIOLATION specified u32 rather than DWORD.

I think this was just a slip on your part, so I adjust that in my branch. If that was deliberate then please come over to #79965 and comment on e2bd0d0

Thanks :-).

@ChrisDenton
Copy link
Member Author

@ijackson That was indeed a slip on my part and I'm glad you caught it!

@ijackson
Copy link
Contributor

@ijackson That was indeed a slip on my part and I'm glad you caught it!

No problem. They're the same type underneath anyway. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants