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

Support OUT_DIR located in \\?\ path on Windows #13914

Closed
wants to merge 1 commit into from

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented May 14, 2024

What does this PR try to resolve?

See rust-lang/rust#75075. If cargo.exe is compiled on Windows with a target dir that begins with "\\?\", the build fails with an error similar to the one seen in https://github.com/tokio-rs/tokio/actions/runs/9075609753/job/24936610110.

error: couldn't read \\?\D:\a\tokio\tokio\target\debug\build\rustversion-2d44b26e828f2c8d\out/version.expr: The filename, directory name, or volume label syntax is incorrect. (os error 123)
   --> C:\Users\runneradmin/.cargo\registry\src\index.crates.io-6f17d22bba15001f\rustversion-1.0.16\src/lib.rs:186:30
    |
186 | const RUSTVERSION: Version = include!(concat!(env!("OUT_DIR"), "/version.expr"));
    |                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Normally in Windows paths / works just as well as \, but from Maximum Path Length Limitation: File I/O functions in the Windows API convert "/" to "\" as part of converting the name to an NT-style name, except when using the "\\?\" prefix as detailed in the following sections.

How should we test and review this PR?

This should be "if it compiles, it works" and Cargo's CI already has coverage on Windows. We can look into what Tokio's CI was doing to trigger the "\\?\" case in GitHub Actions and try to reproduce it in Cargo's CI if desired.

Additional information

Future work: I hope that someone can make a successful proposal for something like include_bytes!(concat!(env!("OUT_DIR") / "man.tgz")) that works in a cross-platform manner by using the correct path separator for rustc's host OS.

@rustbot
Copy link
Collaborator

rustbot commented May 14, 2024

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-cli Area: Command-line interface, option parsing, etc. A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 14, 2024
@epage
Copy link
Contributor

epage commented May 15, 2024

Internally to Cargo, we exclusively work with regular paths and not verbatim paths. It sounds like the problem is that we accept paths from users in a couple of places which may be verbatim paths.

Should we instead run dunce on those paths so they are no longer verbatim? This would solve it for Cargo and all other cases.

@dtolnay
Copy link
Member Author

dtolnay commented May 15, 2024

Good call, if Cargo can make sure env!("OUT_DIR") is never prefixed with "\\?\" that would be great. (And maybe the other filepath variables set by Cargo: CARGO_MANIFEST_DIR, CARGO_BIN_EXE_<name>, CARGO_TARGET_TMPDIR, CARGO_RUSTC_CURRENT_DIR, …)

I looked more into what happened in Tokio and my current guess about the failure is it's connected to one or more of 3 uses of Path::canonicalize by the cargo-check-external-types crate, but I am not sure which one. (@jdisanti)

canonicalize is documented as: "On Windows, this converts the path to use extended length path syntax, which allows your program to use longer path names, but means you can only join backslash-delimited paths to it, and it may be incompatible with other applications (if passed to the application on the command-line, or written to a file another application may read)."

If the canonicalized path is making it to Cargo through something like --manifest-path, let's find that and apply dunce.

@dtolnay
Copy link
Member Author

dtolnay commented May 16, 2024

I wrote up a repro in #13919. Multiple things appear to be going wrong. The issue occurs whether \\?\… is in Cargo's working directory or in a --manifest-path argument. Rustc's current directory ends up being \\?\…, and Cargo is also passing \\?\… paths in some rustc flags including --out-dir and -Ldependency.

@dtolnay dtolnay closed this May 16, 2024
@dtolnay dtolnay deleted the windows branch May 16, 2024 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli Area: Command-line interface, option parsing, etc. A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants