Allow installing alongside existing Rust (with warning) #2214

sollyucko · 2020-01-23T16:01:43Z

FYI, I haven't been able to test this because I run out of disk space, which is why telling users who want to install alongside an existing install to modify the source and recompile doesn't work. (Specificially, I am trying to run nightly Rust on repl.it.)

See rust-lang#953

kinnison · 2020-01-25T09:09:50Z

Hi @sollyucko

Thanks for your wanting to contribute to Rustup. I'm sorry that you're having difficulty with repl.it and rustup. The change as you propose it is not acceptable because it removes a sanity check which we want to remain present for the most part. Better would be to make a change similar to the one in #705 - specifically the env var RUSTUP_INIT_SKIP_PATH_CHECK and the use of the no_prompt flag from the -y command like argument. I think, perhaps, introducing RUSTUP_INIT_SKIP_INSTALLED_CHECK might be a good name and you could alter the path check and the manifest sanity check to honour that (and the latter to honour the no_prompt).

If you want to work on this, let me know, otherwise it'd be better to open a more descriptive issue detailing how this is necessary for repl.it and then close this PR to allow someone else to have a go at fixing it.

I hope you're up for the work, because I think it'd be useful, but I understand if you don't have the space to do it, rustup is quite big to build :D

sollyucko · 2020-01-28T03:15:18Z

What do you think of my latest commit (46cf331)? It currently requires either replying to a confirmation prompt or specifying the -y flag, and produces warnings no matter what.

sollyucko · 2020-01-28T03:17:20Z

Oops, fixed formatting. (c11150c)

sollyucko · 2020-01-28T11:18:48Z

Need to fix test cases...

kinnison

As you say, you'd need to deal with test hiccoughs (I imagine there's plenty of installer tests which will need tweaking, and I'll want to see at least some hint of testing the bypassing of the error if possible), and there're some bits to tweak, but this is making progress.

src/cli/common.rs

src/cli/self_update.rs

src/cli/common.rs

src/cli/self_update.rs

sollyucko · 2020-01-29T11:14:20Z

Assuming there aren't any more issues, I think all I have left to do is to add some more test cases.

sollyucko · 2020-01-31T02:40:15Z

@kinnison This now has a working implementation and tests for each method of skipping or ignoring the check. Is this ready to merge, or are there any issues I should fix? Thanks for the suggestions, BTW.

kinnison · 2020-02-01T17:38:09Z

Hi @sollyucko thanks for your efforts in improving this work. I shall review the over-all diff since you have so many commits on this branch. Assuming I'm happy with the work, we'll need it rebasing into a smaller number of commits which logically express the work you did; rather than the actual process by which you achieved the final result. That way, the history is more useful if we need to work out why a line of code is the way it is.

I shall review your work tonight.

kinnison

Your testing looks excellent. A few comments are inline, and as previously stated, once things look good we'll need the commit sequence rebasing to clean it up.

I am very excited by this work.

src/cli/self_update.rs

kinnison · 2020-02-01T17:46:05Z

tests/cli-inst-interactive.rs

+        let mut cmd = clitools::cmd(config, "rustup-init", &["-y"]);
+        clitools::env(config, &mut cmd);
+        cmd.env("RUSTUP_INIT_SKIP_EXISTENCE_CHECKS", "yes");
+
+        cmd.stdin(Stdio::piped());
+        cmd.stdout(Stdio::piped());
+        cmd.stderr(Stdio::piped());
+        let _out = cmd.spawn().unwrap().wait_with_output().unwrap();
+        let out = SanitizedOutput {
+            ok: _out.status.success(),
+            stdout: String::from_utf8(_out.stdout).unwrap(),
+            stderr: String::from_utf8(_out.stderr).unwrap(),
+        };


I feel like if this wasn't already possible via clitools more directly, then we need to improve that, rather than having this code in a test case. It makes it harder to see what the test case is up to.

This was copied from run_input, not clitools.

I see. It'd be preferable to make run_input() more generic (perhaps run_input_with_env() with run_input() changed to call through that) just so that we don't end up with code duplication for this stuff.

In your rebase I think you lost this bit of the test change.

sollyucko · 2020-02-01T19:22:53Z

Yeah, the commits are definitely a mess...

sollyucko · 2020-02-02T04:10:17Z

Does this git rebase input look good?

p 46cf331 Made some errors non-fatal.
s c11150c Ran rustfmt.
p 5a88830 Fixed test case.
r a2db8ae Renamed `non_fatal_error` to `ignorable_error`.
r 88b7de9 Added environment variable `RUSTUP_INIT_SKIP_CHECKING_EXISTENCE`.
s f55f76e Oops, got environment variable check backwards...
r 0e57beb Added tests for ignorable errors.
s d0292ef Ran rustfmt.
s fa3e3b4 Fixed `#[ignore]` attribute.
s 527be7d Fixed tests (e.g. setting up environment for test).
s 24a0863 Added `use` statements.
s 9ebc560 Parameter types go *after* the name and a colon...
s 30e0273 Fixed error.
s db6db45 rustfmt
s da0a80a Fixed test errors.
r 7445b7a Added test for `RUSTUP_INIT_SKIP_CHECKING_EXISTENCE`.
r d5064eb Created `run_input_with_env`.
s 414cee1 Removed unnecessary `use`s.

# Rebase 703b405..414cee1 onto 703b405 (18 commands)

kinnison · 2020-02-02T09:15:21Z

I'd probably make the run_input_with_env() stuff come before the new test cases, but the basic shape of the result looks good. You'll then want to run again and reword your commits to make the commit messages be nice, rather than the concatenation of messages which squash will result in.

kinnison

This change is starting to look really good. Let's get the rebase done, and then I will go over the UX aesthetics by running it on my laptop to see how it feels. In general this is very promising though.

sollyucko · 2020-02-02T13:57:54Z

Rebase done!

kinnison

You've managed to lose the use of run_input_with_env() in your test case during the rebase. That needs fixing.

Also, do you need to keep the renaming of non_fatal() as a separate commit or can that be folded into the original work?

sollyucko · 2020-02-02T15:38:23Z

You've managed to lose the use of run_input_with_env() in your test case during the rebase. That needs fixing.

Oh, oops.

Also, do you need to keep the renaming of non_fatal() as a separate commit or can that be folded into the original work?

I'll squash it in.

sollyucko · 2020-02-02T15:49:15Z

Both of those should now be fixed.

kinnison · 2020-02-03T15:34:03Z

I shall re-review this tonight, Thanks.

kinnison

The code now looks good, I need to see how I feel when I run this locally and experiment, but I'm pretty sure it's mergeable.

sollyucko · 2020-02-04T18:01:55Z

Sounds good, thanks!

rbtcollins · 2020-02-05T07:51:31Z

src/cli/self_update.rs

-        Err("cannot install while Rust is installed".into())
-    } else {
-        Ok(())
+        ignorable_error("cannot install while Rust is installed".into(), no_prompt)?;


Errata: this is not ignorable if the location of the existing rust is in the CARGO_BIN directory that rustup's proxies will be written to. We're making this easier to run into, so perhaps we should check for that at this point.

@rbtcollins Should it be possible to skip it in any way (e.g. environment variable) or not?

Currently the code will give a warning later about "tool X is already installed", and not touch those binaries. This would mean that rustup isn't actually installed. I don't think it makes sense to allow that to be bypassed, but would like @kinnison 's input. This might be something to do as a separate PR for instance.

The cases this PR covers are to do with rustc on the PATH or about detecting an old-style rustup.sh install as I understand the change. If it happens that it's in CARGO_BIN then the user is going to get warnings as you say, and if they ignore those then they get to keep all the pieces. I don't think we can stop all the footguns -- this is simply a way to allow another footgun for someone who really can't install rustup otherwise due to immutable underlying layers.

The concern I have is that this is escalating it from a read-the-docs situation to a y/n prompt, so perhaps a lot easier to perform targeted foot removal.

Possibly so. I shall check the feel of that in my testing which I'm about to undertake.

Okay so the issue here is that if the rustc/cargo is in $CARGO_BIN then the only real pain is that rustup will replace them with proxies without prompting. The only tools you get the warnings about are rustfmt and cargo-fmt because those used to be provided via cargo install rather than always being proxied via rustup. I think that the footgun is permissible in this case because it's exceedingly unlikely that the user will have set up this kind of thing without also being aware of what rustup-init will end up doing

kinnison · 2020-02-18T19:30:45Z

Hi @sollyucko

I've now done some user-testing and I'm concerned about some of the wording...

When you have /usr/bin/rustc and you try and install you get:

error: it looks like you have an existing installation of Rust at:
error: /usr/bin
error: rustup should not be installed alongside Rust. Please uninstall your existing Rust first.
error: Otherwise you may have confusion unless you are careful with your PATH
error: If you are sure that you want both rustup and your already installed Rust
error: then please restart the installation and pass `-y' to bypass this check.
error: cannot install while Rust is installed

Continue? (y/N)

It seems odd to both tell the user they have to restart and pass -y yet offer them the chance to continue. We'll need to change that wording a little, perhaps to something like:

error: If you are sure that you want both rustup and your already installed Rust
error: then you can continue, though be aware that it may cause confusion depending
error: on your PATH settings.  You can bypass this check by passing `-y` when installing.

Otherwise I think I'm okay with this. So if you can look over and provide a better message, we're close to being able to merge this.

src/cli/self_update.rs

…_in_path`.

sollyucko · 2020-02-21T03:21:12Z

Does this look good?

kinnison

Let's get this merged and see how we feel over time. Thank you.

Allow installing alongside existing Rust (with warning)

703b405

See rust-lang#953

kinnison suggested changes Jan 28, 2020

View reviewed changes

src/cli/common.rs Show resolved Hide resolved

src/cli/self_update.rs Outdated Show resolved Hide resolved

src/cli/common.rs Outdated Show resolved Hide resolved

src/cli/self_update.rs Outdated Show resolved Hide resolved

kinnison suggested changes Feb 1, 2020

View reviewed changes

kinnison reviewed Feb 2, 2020

View reviewed changes

sollyucko force-pushed the patch-1 branch from 414cee1 to 454c028 Compare February 2, 2020 13:58

kinnison suggested changes Feb 2, 2020

View reviewed changes

sollyucko added 5 commits February 2, 2020 10:44

Made some errors non-fatal.

62f32f7

Fixed test case.

680b2ed

Added environment variable RUSTUP_INIT_SKIP_CHECKING_EXISTENCE.

5bc1c8c

Added tests for ignorable errors.

76164d2

Created run_input_with_env.

8095f81

sollyucko force-pushed the patch-1 branch from 454c028 to ef45af5 Compare February 2, 2020 15:48

sollyucko force-pushed the patch-1 branch 2 times, most recently from 0e2333d to 251bb55 Compare February 2, 2020 15:56

Added test for RUSTUP_INIT_SKIP_CHECKING_EXISTENCE.

ac5105f

sollyucko force-pushed the patch-1 branch from 251bb55 to ac5105f Compare February 2, 2020 15:58

kinnison self-assigned this Feb 3, 2020

kinnison approved these changes Feb 4, 2020

View reviewed changes

rbtcollins reviewed Feb 5, 2020

View reviewed changes

kinnison suggested changes Feb 18, 2020

View reviewed changes

src/cli/self_update.rs Outdated Show resolved Hide resolved

kinnison removed their assignment Feb 18, 2020

Improved warning/error message for `check_existence_of_rustc_or_cargo…

1e0a497

…_in_path`.

sollyucko force-pushed the patch-1 branch from 14128aa to 1e0a497 Compare February 20, 2020 00:57

kinnison approved these changes Feb 27, 2020

View reviewed changes

kinnison merged commit 4656d95 into rust-lang:master Feb 27, 2020

Allow installing alongside existing Rust (with warning) #2214

Allow installing alongside existing Rust (with warning) #2214

Conversation

sollyucko commented Jan 23, 2020

kinnison commented Jan 25, 2020

sollyucko commented Jan 28, 2020

sollyucko commented Jan 28, 2020

sollyucko commented Jan 28, 2020

kinnison left a comment

Choose a reason for hiding this comment

sollyucko commented Jan 29, 2020 • edited Loading

sollyucko commented Jan 31, 2020 • edited Loading

kinnison commented Feb 1, 2020

kinnison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sollyucko commented Feb 1, 2020

sollyucko commented Feb 2, 2020 • edited Loading

kinnison commented Feb 2, 2020

kinnison left a comment

Choose a reason for hiding this comment

sollyucko commented Feb 2, 2020

kinnison left a comment

Choose a reason for hiding this comment

sollyucko commented Feb 2, 2020

sollyucko commented Feb 2, 2020

kinnison commented Feb 3, 2020

kinnison left a comment

Choose a reason for hiding this comment

sollyucko commented Feb 4, 2020

Choose a reason for hiding this comment

sollyucko Feb 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinnison commented Feb 18, 2020

sollyucko commented Feb 21, 2020 • edited Loading

kinnison left a comment

Choose a reason for hiding this comment

sollyucko commented Jan 29, 2020 •

edited

Loading

sollyucko commented Jan 31, 2020 •

edited

Loading

sollyucko commented Feb 2, 2020 •

edited

Loading

sollyucko Feb 5, 2020 •

edited

Loading

sollyucko commented Feb 21, 2020 •

edited

Loading