cli: add rustup self uninstall --no-modify-path

[cyqsimon]

Closes #4529

TODO

• Tests

[cyqsimon]

Not really sure how to properly add the tests that check for environment variable configuration. There doesn't seem to be existing infrastructure for testing this sort of thing.

Also I don't really understand how the current tests are avoiding modifying the user's actual rc files. I imagine the provisions for it must be hidden away somewhere in the test harness but I just couldn't find it.

[rami3l]

Not really sure how to properly add the tests that check for environment variable configuration. There doesn't seem to be existing infrastructure for testing this sort of thing.

Also I don't really understand how the current tests are avoiding modifying the user's actual rc files. I imagine the provisions for it must be hidden away somewhere in the test harness but I just couldn't find it.

@cyqsimon That's a completely fine question to ask! I think you can find some inspiration from:

rustup/tests/suite/cli_paths.rs

Lines 239 to 267 in 2bd6356

	#[tokio::test]
	async fn uninstall_removes_source_from_rcs() {
	let cx = CliTestContext::new(Scenario::Empty).await;
	let rcs: Vec<PathBuf> = [
	".bashrc",
	".bash_profile",
	".bash_login",
	".profile",
	".zshenv",
	]
	.iter()
	.map(|rc| cx.config.homedir.join(rc))
	.collect();
	for rc in &rcs {
	raw::write_file(rc, FAKE_RC).unwrap();
	}
	cx.config.expect(&INIT_NONE).await.is_ok();
	cx.config
	.expect(&["rustup", "self", "uninstall", "-y"])
	.await
	.is_ok();
	for rc in &rcs {
	let new_rc = fs::read_to_string(rc).unwrap();
	assert_eq!(new_rc, FAKE_RC);
	}
	}

Basically we use CliTestContext to create a sandboxed installation for rustup to operate in TempDir, so copying-pasting and modifying neighboring tests should do the trick for you.

[cyqsimon]

Alright I've added the tests.

When I initially went over the existing tests I was fixated on tests/suite/cli_self_upd.rs and found nothing there 😅. But yeah within cli_paths.rs it's largely a copy-paste job.

How would you like me to resolve the conflict with #4536? Merge or rebase?

[djc]

Rebase please!

[cyqsimon]

I have having a very difficult time reproducing the CI error locally (or in fact, making sense of it at all). Will keep trying for the time being.

Edit: seems like there's some shenanigans influencing whether rustup-init sets up .zshenv or not, which is different between the CI and my local environment. That's all I can say with any degree of confidence. Zsh is not my daily shell so I'm not that familiar with it.

[rami3l]

@cyqsimon Many thanks all the same! I think I can have a look at that... I'll let you know if I've found anything 🙏

[rami3l]

@cyqsimon That's quite easy actually: in our Ubuntu CI environment there's no zsh so the ~/.zshenv predicate is never triggered. Maybe just remove that from the test case?

[cyqsimon]

@cyqsimon That's quite easy actually: in our Ubuntu CI environment there's no zsh so the ~/.zshenv predicate is never triggered. Maybe just remove that from the test case?

I see. But shouldn't the fix be to install zsh in the CI, so that the relevant code paths are actually tested?

[rami3l]

I see. But shouldn't the fix be to install zsh in the CI, so that the relevant code paths are actually tested?

@cyqsimon I see your point here; however I don't think a typical Linux environment assumes zsh, so this might confuse more users than it should when they are developing rustup on Linux; we want the test suite to have as few surprising external dependencies as possible.

Also I don't think placing a dynamic predicate for both cases is a good choice either, the point being you are almost always testing one specific path in the CI and the opposite path almost never gets tested and could rust over time. This has happened before in this repo's history for a few times, unfortunately...

As a compromise I think you can put a cfg for darwin on that specific array item. Don't put it for linux otherwise you might break CI for other Unices (BSDs, Solaris, ...)!

[cyqsimon]

As a compromise I think you can put a cfg for darwin on that specific array item. Don't put it for linux otherwise you might break CI for other Unices (BSDs, Solaris, ...)!

Yep that sounds reasonable. Will do that.

And what about uninstall_removes_source_from_rcs (the test you referenced)? That test is only passing because the lack of an intermediate assert! between init and uninstall.

[cyqsimon]

Rebased to resolve merge conflicts.

[rami3l]

And what about uninstall_removes_source_from_rcs (the test you referenced)? That test is only passing because the lack of an intermediate assert! between init and uninstall.

@cyqsimon You can mirror that in a separate commit if you want.