-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add try_canonicalize
and use it over std::fs::canonicalize
#11866
Conversation
r? @ehuss (rustbot has picked a reviewer for you, use r? to override) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am happy to approve this if Windows expert like @ChrisDenton nod on the Rust side. It will be better if std exposes a better API for this.
use windows_sys::Win32::Storage::FileSystem::GetFullPathNameW; | ||
|
||
// On Windows `canonicalize` may fail, so we fall back to getting an absolute path. | ||
std::fs::canonicalize(&path).or_else(|_| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we wait for the stabilization of std::path::aboslute
? We should also have a TODO here and call out where this piece of code come from. (Looks like fn absolute
on windows to me.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would probably take a while.
Is there any issue discussion this? This sounds like a lot larger of a pitfall and it seems like this should either be fixed in the stdlib or that there should be a general crate for solving this problem (similar to |
Also cc @ehuss who I think has seen a number of bug reports on this over the years.
Oh my yes, This is precisely why I created the My current view is that, once it's stable,
I've been experimenting with path handling in the |
Thanks, this seems reasonable to me. There is more long-term work to do here, but this should be fine to unblock the immediate needs. We have been using I pushed a small spelling fix. @bors r+ |
☀️ Test successful - checks-actions |
1 similar comment
☀️ Test successful - checks-actions |
👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request. |
Update cargo 17 commits in 0e474cfd7b16b018cf46e95da3f6a5b2f1f6a9e7..7bf43f028ba5eb1f4d70d271c2546c38512c9875 2023-03-31 23:15:58 +0000 to 2023-04-10 16:01:41 +0000 - docs(pkgid): Consistently use @ (rust-lang/cargo#11956) - Fix credential token format validation. (rust-lang/cargo#11951) - Validate token on publish. (rust-lang/cargo#11952) - Clarify docs on `-C` that it appears before the command. (rust-lang/cargo#11947) - Add `try_canonicalize` and use it over `std::fs::canonicalize` (rust-lang/cargo#11866) - Fix typo (rust-lang/cargo#11944) - docs(changelog): Change wording about auto-fix message (rust-lang/cargo#11943) - Update home repo URL (rust-lang/cargo#11941) - doc(changelog): `[env]` is a table, not a stable (rust-lang/cargo#11942) - Stop using UncanonicalizedIter for QueryKind::Exact (rust-lang/cargo#11937) - Don't query permutations of the path prefix. (rust-lang/cargo#11936) - Fix typo in variable name (rust-lang/cargo#11931) - Fix Cargo warning about unused sparse configuration key (rust-lang/cargo#11930) - Switch benchsuite to the index archive. (rust-lang/cargo#11933) - Update git2 (rust-lang/cargo#11928) - Publish docs: Clarify requirements about the state of the index after publish. (rust-lang/cargo#11926) - Call out the differences between the index JSON and the API or metadata. (rust-lang/cargo#11927)
This adds a
try_canonicalize
function that callsstd::fs::canonicalize
and on Windows falls back to getting an absolute path. Uses ofcanonicalize
have been replaced withstd::fs::canonicalize
. On Windowsstd::fs::canonicalize
may fail due to incomplete drivers. In particularImDisk
does not support it.Combined with rust-lang/rust#109231 this allows compiling crates on an
ImDisk
RAM disk and I've tested that it works with various configuration using rcb.