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

Fix file path separator decoded from manifest is not correct on Windows #3077

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rhysd
Copy link
Contributor

@rhysd rhysd commented Oct 4, 2022

While installing rust-doc component on Windows, I noticed the following log messages in the output.

info: retrying renaming 'C:\Users\rhysd\.rustup\tmp\d648r0b_lyhd2svq_dir\rust-docs\share/doc/rust/html' to 'C:\Users\rhysd\.rustup\toolchains\stable-x86_64-pc-windows-msvc\share/doc/rust/html'

This log message itself was not a problem, but some path separators were not correct ...\stable-x86_64-pc-windows-msvc\share/doc/rust/html.

After taking a look at sources of rustup, I understood the cause was that the share/doc/rust/html part was decoded from manifest.in directly without converting path separators. This PR fixes it.

@rhysd
Copy link
Contributor Author

rhysd commented Oct 6, 2022

Can I have some review from maintainer? If something is blocking review, I'll happily try to resolve it. So please let me know.

@Rustin170506 Rustin170506 self-requested a review October 7, 2022 13:13
@Rustin170506
Copy link
Member

Thanks for your patch. I'll take a look tomorrow.

@Rustin170506
Copy link
Member

Does C:\Users\rhysd\.rustup\toolchains\stable-x86_64-pc-windows-msvc\share/doc/rust/html is valid path on Windows? Because I have no Windows PC so I can't test it.

@rhysd
Copy link
Contributor Author

rhysd commented Oct 8, 2022

File path separator on Windows is \. So the path is valid but it does not represent the indented path C:\Users\rhysd\.rustup\toolchains\stable-x86_64-pc-windows-msvc\share\doc\rust\html.

I don't know why it works currently, but I think it would be canonicalized in some layer.

@rbtcollins
Copy link
Contributor

It currently works because the Win32 APIs are flexible. The NT api that underlies it is not, and if we pass in \?\ style paths to the Win32 API it will also not be flexible, from memory. Canonicalising it is the right thing to do. I haven't read the patch at this point, just wanted to say thanks for taking this on and its clearly the right outcome to aim for.

@rhysd
Copy link
Contributor Author

rhysd commented Oct 11, 2022

rustup uses fs::copy recursively so I checked the Windows implementation and understood CopyFileExW is used. But I could not find any document which handles / as \.

https://github.com/rust-lang/rust/blob/1e926f06528ecb2503f026e2fd53cb735d487b10/library/std/src/sys/windows/fs.rs#L1343-L1350

As @rbtcollins kindly shared the thought, I think the current behavior is depending on some implementation details of platform-specific API.

line.find(':').map(|pos| {
Self(
line[0..pos].to_owned(),
PathBuf::from_slash(&line[(pos + 1)..]),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I've had a look at this. I think the change should be localised to the rename path, rather than the manifest: the change you've made will result in different manifests in the rustup dir depending on OS - that will interact badly with rustup dirs shared on NFS and the like.

I'm not entirely sure that the extra library is worth it - since we're only doing a relatively small number of these operations the overhead of a deconstruct to segments and reconstruct will be quite inivisble.

    42 /home/robertc/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/manifest-cargo-x86_64-unknown-linux-gnu
     5 /home/robertc/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/manifest-clippy-preview-x86_64-unknown-linux-gnu
    26 /home/robertc/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/manifest-rust-analysis-x86_64-unknown-linux-gnu
    25 /home/robertc/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/manifest-rustc-x86_64-unknown-linux-gnu
     1 /home/robertc/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/manifest-rust-docs-x86_64-unknown-linux-gnu
     5 /home/robertc/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/manifest-rustfmt-preview-x86_64-unknown-linux-gnu
  1466 /home/robertc/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/manifest-rust-src
    32 /home/robertc/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/manifest-rust-std-x86_64-unknown-linux-gnu
  1602 total

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants