consistently add context with file path when parsing fails

[Skgland]

fixes #3851

[rami3l]

@Skgland Thanks for making this PR!

Would you mind explain in detail what is the motivation of this PR and what has been improved with a simple before/after comparison, for example?

PS: To make sure we are on the same page, regarding #3851 I would like to underline that the server incident causing bad manifests being downloaded is probably out of the scope of the Rustup project (so it possibly cannot be "fixed"), although improvements regarding error handling are welcomed.

[rami3l]

Your CI failure is #3852, we will be investigating this on our side.

[Skgland]

Would you mind explain in detail what is the motivation of this PR and what has been improved with a simple before/after comparison, for example?

Having a manifest parsing error while using cargo I initially assumed a mistake in the Cargo.toml,
though due to the problematic line in the error message that was quickly ruled out.
Next I tried to figure out where cargo was trying and failing to parse a manifest.
I assumed that this was maybe a manifest from the crates.io package registry cache, but searching for the line in ~/.cargo/ didn't bring up any results.
Only when I noticed rust-analyzer and cargo --help fail the same way did I come to the conclusion that the source is likely from the rustup wrapper of those binaries and not cargo itself and as such began searching in rustup.
This search could have been shorten had the manifest path been part of the error message, thus me not having to search for which file it was that had the error.

To recreate the error condition I inserted a NUL byte in a valid manifest file, resulting in the following error before/after my change.

Before:

$> RUSTUP_HOME=home CARGO_HOME=home home/bin/cargo --help
error: error parsing manifest: TOML parse error at line 1286, column 24
     |
1286 | [[pkg.rust.target.aarch64-apple-darwin.components]]
     |                        ^
invalid table header
expected `.`, `]]`

After:

$> RUSTUP_HOME=home CARGO_HOME=home home/bin/cargo --help
error: could not parse manifest file: '/home/bennet/git/rustup/home/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/multirust-channel-manifest.toml': error parsing manifest: TOML parse error at line 1286, column 24
     |
1286 | [[pkg.rust.target.aarch64-apple-darwin.components]]
     |                        ^
invalid table header
expected `.`, `]]`

PS: To make sure we are on the same page, regarding #3851 I would like to underline that the server incident causing bad manifests being downloaded is probably out of the scope of the Rustup project (so it possibly cannot be "fixed"), although improvements regarding error handling are welcomed.

Yes, I agree that the source of the manifest corruption is outside of rustup and as such out of scope. My goal with the Issue and this PR was to improve the error reporting. Also, I am pretty sure that there was no server incident involved, but I believe that the export and re-import of the WSL 2 distribution somehow corrupted the disk image file, resulting in the already installed manifest getting corrupted.

[rami3l]

@Skgland Thanks for the clarification!

At Rustup the word "manifest" is used specifically to refer to a certain kind of file downloaded from the Rustup release server, and, as you've found out, named multirust-channel-manifest.toml.

This file is specifically used to determine what are available in this channel, however I have no idea how you got that wrong (since we have shasum checks), sorry about that...

rustup/tests/suite/cli_v2.rs

Lines 280 to 300 in 3ba08da

	#[test]
	fn bad_sha_on_manifest() {
	setup(&|config| {
	// Corrupt the sha
	let sha_file = config
	.distdir
	.as_ref()
	.unwrap()
	.join("dist/channel-rust-nightly.toml.sha256");
	let sha_str = fs::read_to_string(&sha_file).unwrap();
	let mut sha_bytes = sha_str.into_bytes();
	sha_bytes[..10].clone_from_slice(b"aaaaaaaaaa");
	let sha_str = String::from_utf8(sha_bytes).unwrap();
	rustup::utils::raw::write_file(&sha_file, &sha_str).unwrap();
	// We fail because the sha is bad, but we should emit the special message to that effect.
	config.expect_err(
	&["rustup", "default", "nightly"],
	"update not yet available",
	);
	});
	}

This PR might have to wait for #3852 before we get our merge CI green again. If you'd like, in the meantime, you can add a regression test in the file above.

[Skgland]

Ok, just pushed a commit with a regression test

[Skgland]

Hm, looks like my regression test fails on windows due to \ vs. / in the path in the error message.

[Skgland]

Do you have a suggestion for how to deal with the path-separator discrepancy on windows or should I skip the test on windows with e.g. #[cfg(not(target_os = "windows"))] the test?

[rami3l]

Do you have a suggestion for how to deal with the path-separator discrepancy on windows or should I skip the test on windows with e.g. #[cfg(not(target_os = "windows"))] the test?

@Skgland Please feel free to use what you consider appropriate for the problem. For example you can use two conditionally compiled assertions for Windows and Unix. Or you can compare the path components.