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

Add try_canonicalize and use it over std::fs::canonicalize #11866

Merged
merged 3 commits into from
Apr 8, 2023

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.

src/cargo/util/mod.rs Outdated Show resolved Hide resolved
@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