Allow selecting toolchains to uninstall with glob pattern by camelid · Pull Request #2540 · rust-lang/rustup

camelid · 2020-10-27T20:22:29Z

Examples

Correct usage

$ rustup toolchain liststable-x86_64-apple-darwin (default)nightly-2020-01-01-x86_64-apple-darwinnightly-2020-06-01-x86_64-apple-darwinnightly-x86_64-apple-darwin
$ rustup toolchain uninstall 'nightly-20*'info: uninstalling toolchain 'nightly-2020-01-01-x86_64-apple-darwin'info: toolchain 'nightly-2020-01-01-x86_64-apple-darwin' uninstalledinfo: uninstalling toolchain 'nightly-2020-06-01-x86_64-apple-darwin'info: toolchain 'nightly-2020-06-01-x86_64-apple-darwin' uninstalled
$ rustup toolchain liststable-x86_64-apple-darwin (default)nightly-x86_64-apple-darwin

Invalid pattern

$ rustup toolchain uninstall '***'error: Pattern syntax error near position 2: wildcards are either regular `*` or recursive `**`

Work-in-progress:

Errors need improvement (instead of just panicking)
Needs test
Needs CLI docs

$ rustup toolchain list stable-x86_64-apple-darwin (default) nightly-2020-04-18-x86_64-apple-darwin nightly-2020-04-20-x86_64-apple-darwin nightly-x86_64-apple-darwin $ rustup toolchain uninstall --regex 'nightly-*' info: uninstalling toolchain 'nightly-2020-04-18-x86_64-apple-darwin' info: toolchain 'nightly-2020-04-18-x86_64-apple-darwin' uninstalled info: uninstalling toolchain 'nightly-2020-04-20-x86_64-apple-darwin' info: toolchain 'nightly-2020-04-20-x86_64-apple-darwin' uninstalled info: uninstalling toolchain 'nightly-x86_64-apple-darwin' info: toolchain 'nightly-x86_64-apple-darwin' uninstalled $ rustup toolchain list stable-x86_64-apple-darwin (default)

kinnison · 2020-10-28T20:20:13Z

One comment, before I even look at the code too deeply, I'm not sure regular expressions are ideal for this, instead perhaps it's best to advertise this as shell-glob style patterns. i.e. stable-* rather than stable-.* being needed?

camelid · 2020-10-28T20:25:32Z

I'm not fully sure what you mean by "advertise as shell-glob style patterns": It takes a regex, not a glob pattern. I realized that I confused myself with the example I tried because I used the regex nightly-* which actually matches the substring nightly, nightly-, nightly--, etc.

kinnison · 2020-10-28T20:27:24Z

Most "mortals" are not regex savvy, but they understand shell glob patterns (i.e. * not .*)

camelid · 2020-10-28T20:30:09Z

So are you suggesting that we only allow the metacharacter *, escape all other characters, replace the * with .*, and then pass it to regex? I think I agree that we should have it be a shell glob pattern, but I'm not sure what the best way to implement it is.

camelid · 2020-10-28T20:33:30Z

Oh, I guess we could use the glob crate to do this. Is that what you were thinking?

kinnison · 2020-10-28T20:44:05Z

If I were working in C I'd just use the fnmatch() function so if there's a similar crate then that might be best. Otherwise as you suggest, the transformation of escaping metacharacters other than * and then changing * for .* would work.

camelid · 2020-10-31T21:23:26Z

+ info!("no toolchains matched pattern '{}'", pattern_str);
+ return Ok(utils::ExitCode(0));


I think this should return a non-zero exit code, but I modelled this after rustup toolchain uninstall some-toolchain, which just shows a message with a successful exit code. Should I change both of them to return an error?

If it has always been "safe" in the shell scripting sense to "remove" a toolchain which isn't present, then we wouldn't want to break peoples' scripting by making them check first before trying. Better to be informational but consistent with past behaviour here I think.

I'm not entirely sure that this makes sense. Patterns are new; failure to match a pattern is a new situation. We can detect a toolchain string that has no pattern characters and do the prior behaviour, if we want to.
That means, we can take a first principles design here if appropriate.

First treat the toolchain as a pattern; if it doesn't match anything, then treat it as a partial toolchain specifier.

camelid

I think all this needs is some documentation, and then it's ready to go! Where should I put the docs?

camelid · 2020-11-04T21:54:06Z

+#[test]
+fn toolchain_uninstall_pattern() {
+ setup(&|config| {
+ expect_ok(config, &["rustup", "uninstall", "stable-*"]);
+ expect_ok(config, &["rustup", "uninstall", "nightly-*"]);
+ let mut cmd = clitools::cmd(config, "rustup", &["show"]);
+ clitools::env(config, &mut cmd);
+ let out = cmd.output().unwrap();
+ assert!(out.status.success());
+ let stdout = String::from_utf8(out.stdout).unwrap();
+ assert!(!stdout.contains(for_host!("'stable-{}'")));
+ assert!(!stdout.contains(for_host!("'nightly-2015-01-01-{}'")));
+ });
+}


I just copied this test code from other tests and modified it. Is the test correct?

You probably want to start the test by installing and asserting that stable and nightly are there. Otherwise I think this is fine.

camelid · 2020-11-18T22:57:11Z

@kinnison Could you review this again? Thanks!

kinnison · 2020-11-21T21:14:50Z

+#[test]
+fn toolchain_uninstall_pattern() {
+ setup(&|config| {
+ expect_ok(config, &["rustup", "uninstall", "stable-*"]);
+ expect_ok(config, &["rustup", "uninstall", "nightly-*"]);
+ let mut cmd = clitools::cmd(config, "rustup", &["show"]);
+ clitools::env(config, &mut cmd);
+ let out = cmd.output().unwrap();
+ assert!(out.status.success());
+ let stdout = String::from_utf8(out.stdout).unwrap();
+ assert!(!stdout.contains(for_host!("'stable-{}'")));
+ assert!(!stdout.contains(for_host!("'nightly-2015-01-01-{}'")));
+ });
+}


You probably want to start the test by installing and asserting that stable and nightly are there. Otherwise I think this is fine.

rami3l · 2024-07-10T12:43:02Z

Closing in favor of #3862, as discussed in #3519 (comment). Thanks a lot for helping out nonetheless!

djc · 2024-07-10T12:47:42Z

@rami3l can you elaborate on your rationale for linking this to #3862? Seems like this is still a reasonable request?

rami3l · 2024-07-10T12:52:09Z

@rami3l can you elaborate on your rationale for linking this to #3862? Seems like this is still a reasonable request?

@djc The idea is that one would be able to do rustup toolchain list --quiet | grep nightly-2023- | xargs rustup uninstall after #3862. Not sure if this addition is still worth it 🤔

If the demand is still there we can make another PR with @camelid as the co-author, I have no problem with that.

est31 · 2024-07-10T13:24:27Z

It might still make sense to have glob support. At the least one should print the command you suggested when someone tries to use globs with rustup: it's not trivial to come up with it, nor is it guaranteed to always work in the future.

rami3l · 2024-07-10T13:33:40Z

It might still make sense to have glob support. At the least one should print the command you suggested when someone tries to use globs with rustup: it's not trivial to come up with it, nor is it guaranteed to always work in the future.

@est31 Indeed, I've added #2530 back to our roadmap.

camelid added 2 commits October 27, 2020 13:20

Remove mistakenly-committed code
7962dfe

camelid commented Oct 27, 2020
View reviewed changes

Comment threadsrc/cli/rustup_mode.rs Outdated
Comment threadsrc/cli/rustup_mode.rs Outdated

camelid commented Oct 27, 2020
View reviewed changes

Comment threadsrc/cli/rustup_mode.rs Outdated

Add use regex::Regex instead of using full path
3c01125

camelid commented Oct 27, 2020
View reviewed changes

Comment threadsrc/cli/rustup_mode.rs Outdated

kinnison reviewed Oct 28, 2020
View reviewed changes

Comment threadsrc/cli/rustup_mode.rs Outdated
Comment threadsrc/cli/rustup_mode.rs Outdated
Comment threadsrc/cli/rustup_mode.rs Outdated
Comment threadsrc/cli/rustup_mode.rs Outdated
Comment threadsrc/config.rs Outdated
Comment threadsrc/config.rs

Return impl Iterator instead of Vec
7ce4203

camelid force-pushed the toolchain-uninstall-regex branch from c39c730 to 7ce4203Compare October 28, 2020 20:51

Accept glob pattern instead of regex
8c94393

camelid changed the title ~~Allow selecting toolchains to uninstall with regex~~Allow selecting toolchains to uninstall with glob patternOct 28, 2020

camelid commented Oct 28, 2020
View reviewed changes

Comment threadsrc/cli/rustup_mode.rs Outdated
Comment threadsrc/cli/rustup_mode.rs Outdated
Comment threadsrc/config.rs

Tell user when pattern did not match any toolchains
75a35b0

camelid commented Oct 31, 2020
View reviewed changes

camelid added 3 commits November 1, 2020 13:42

Default to treating toolchain as a pattern
693632c
First treat the toolchain as a pattern; if it doesn't match anything, then treat it as a partial toolchain specifier.

Add comments
63bc755

Add test
ed9f023

camelid commented Nov 4, 2020
View reviewed changes

kinnison reviewed Nov 21, 2020
View reviewed changes

camelid mentioned this pull request Nov 21, 2020
Somewhat confusing lifetime error rust-lang/rust#79281
Closed

rami3l closed this Jul 10, 2024

		info!("no toolchains matched pattern '{}'", pattern_str);
		return Ok(utils::ExitCode(0));

Uh oh!

Uh oh!

Conversation

camelid commented Oct 27, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Examples

Correct usage

Invalid pattern

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kinnison commented Oct 28, 2020

Uh oh!

camelid commented Oct 28, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kinnison commented Oct 28, 2020

Uh oh!

camelid commented Oct 28, 2020

Uh oh!

camelid commented Oct 28, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kinnison commented Oct 28, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

camelidOct 31, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kinnisonNov 1, 2020

Choose a reason for hiding this comment

Uh oh!

rbtcollinsNov 1, 2020

Choose a reason for hiding this comment

Uh oh!

camelid left a comment

Choose a reason for hiding this comment

Uh oh!

camelidNov 4, 2020

Choose a reason for hiding this comment

Uh oh!

kinnisonNov 21, 2020

Choose a reason for hiding this comment

Uh oh!

camelid commented Nov 18, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kinnisonNov 21, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rami3l commented Jul 10, 2024

Uh oh!

djc commented Jul 10, 2024

Uh oh!

rami3l commented Jul 10, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

est31 commented Jul 10, 2024

Uh oh!

rami3l commented Jul 10, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

camelid commented Oct 27, 2020•
edited
Loading

camelid commented Oct 28, 2020•
edited
Loading

camelidOct 31, 2020•
edited
Loading

rami3l commented Jul 10, 2024•
edited
Loading

rami3l commented Jul 10, 2024•
edited
Loading