Skip to content

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Mar 17, 2023

This adds a try_canonicalize function that calls std::fs::canonicalize and on Windows falls back to getting an absolute path. Uses of canonicalize have been replaced with std::fs::canonicalize. On Windows std::fs::canonicalize may fail due to incomplete drivers. In particular ImDisk 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.

@rustbot
Copy link
Collaborator

rustbot commented Mar 17, 2023

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-configuration Area: cargo config files and env vars A-rebuild-detection Area: rebuild detection and fingerprinting Command-vendor S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 17, 2023
Copy link
Member

@weihanglo weihanglo left a 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(|_| {
Copy link
Member

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

Copy link
Contributor Author

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.

@epage
Copy link
Contributor

epage commented Mar 20, 2023

On Windows std::fs::canonicalize may fail due to incomplete drivers. In particular ImDisk does not support it.

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 dunce or maybe a contribution to it)

@ChrisDenton
Copy link
Member

ChrisDenton commented Mar 20, 2023

Also cc @ehuss who I think has seen a number of bug reports on this over the years.

Is there any issue discussion this?

Oh my yes, This is precisely why I created the path::absolute function (see rust-lang/rust#92750). I also attempted to make a workaround for fs::canonicalize (rust-lang/rust#59117) but people were wary of the trade-offs. See also: rust-lang/rust#59117 (and various other issues over the years).

My current view is that, once it's stable, path::absolute should be used unless resolving symlinks is actually necessary. The downside of that is .. components aren't resolved on POSIX systems (because .. is a kind of link),

there should be a general crate for solving this problem

I've been experimenting with path handling in the omnipath crate. I can definitely add a try_canonicalize function if that would be useful.

@ehuss
Copy link
Contributor

ehuss commented Apr 8, 2023

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 normalize_path instead of canonicalize in many places, usually for this very same problem. We should probably eventually remove normalize_path and either use this or something else. However, normalize_path is used in many places, and it would be a pretty significant time commitment to understand the impact on all of them.

I pushed a small spelling fix.

@bors r+

@bors
Copy link
Contributor

bors commented Apr 8, 2023

📌 Commit 5430956 has been approved by ehuss

It is now in the queue for this repository.

@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 Apr 8, 2023
@bors
Copy link
Contributor

bors commented Apr 8, 2023

⌛ Testing commit 5430956 with merge 1b2de21...

@bors
Copy link
Contributor

bors commented Apr 8, 2023

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 1b2de21 to master...

1 similar comment
@bors
Copy link
Contributor

bors commented Apr 8, 2023

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 1b2de21 to master...

@bors
Copy link
Contributor

bors commented Apr 8, 2023

👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request.

@bors bors merged commit 1b2de21 into rust-lang:master Apr 8, 2023
@Zoxc Zoxc deleted the fs-non-canon branch April 8, 2023 20:16
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 11, 2023
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)
@ehuss ehuss added this to the 1.70.0 milestone Apr 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-configuration Area: cargo config files and env vars A-rebuild-detection Area: rebuild detection and fingerprinting Command-vendor 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.

8 participants