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

[rhysd]

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]

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

[0xPoe]

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

[0xPoe]

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]

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]

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]

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.

[rbtcollins]

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

[rami3l]

@rbtcollins One of our tests is also plagued with this issue, so I doubt whether it's really reasonable to localize the change to the rename path. The snippet below has nothing to do with renaming I assume:

rustup/tests/suite/cli_v2.rs

Lines 389 to 425 in 6fa21c2

	async fn bad_manifest() {
	// issue #3851
	let mut cx = CliTestContext::new(Scenario::SimpleV2).await;
	// install some toolchain
	cx.config.expect_ok(&["rustup", "update", "nightly"]).await;
	#[cfg(not(target_os = "windows"))]
	let path = format!(
	"toolchains/nightly-{}/lib/rustlib/multirust-channel-manifest.toml",
	this_host_triple(),
	);
	#[cfg(target_os = "windows")]
	let path = format!(
	r"toolchains\nightly-{}\lib/rustlib\multirust-channel-manifest.toml",
	this_host_triple(),
	);
	assert!(cx.config.rustupdir.has(&path));
	let path = cx.config.rustupdir.join(&path);
	// corrupt the manifest file by inserting a NUL byte at some position
	let old = fs::read_to_string(&path).unwrap();
	let pattern = "[[pkg.rust.targ";
	let (prefix, suffix) = old.split_once(pattern).unwrap();
	let new = format!("{prefix}{pattern}\u{0}{suffix}");
	fs::write(&path, new).unwrap();
	// run some commands that try to load the manifest and
	// check that the manifest parsing error includes the manifest file path
	cx.config
	.expect_err(
	&["rustup", "check"],
	&format!("error: could not parse manifest file: '{}'", path.display()),
	)
	.await;
	}

... also, so far the only call site to .decode() is in DirectoryPackage::install().

[rami3l]

different manifests in the rustup dir depending on OS

@rbtcollins I think that can be resolved by putting from_slash() in .decode() and to_slash() in .encode()... Performance-wise it won't make that much of a difference, right?

[rami3l]

I'd like to take another look at this... The underlying problem definitely needs more attention anyways.

[rami3l]

Sorry, I've been having relatively low bandwidth in the past month due to the holiday season... But I'm trying to keep up.

I think one proper way out would probably be making an extension to Path so that when any path-related stuff gets displayed it will be normalized first. One possibility is taking inspiration from something like https://docs.rs/crate/normalize-path which comes from cargo. After all, the crux with the original issue was really unsatisfactory error output such as r"toolchains\nightly-{}\lib/rustlib\multirust-channel-manifest.toml" and nothing else. However, that might not be worth the effort if we are really not looking for maximal performance here (I guess we aren't).

Thus, I think it might be better to do from_slash() in .decode() and to_slash() in .encode() and call it a day, as previously mentioned in #3077 (comment).

[rami3l]

I'm closing this now in favor of #4179.

Thanks again for helping out, @rhysd!
I have made you the co-author of 0f67a1f (#4179) as a token of my appreciation.