Check for unexpected cargo/rustc before install

[meqif]

Hi!

This PR fixes #681, stopping the installation if rustc or cargo are found in any directory listed in $PATH if rustup wasn't invoked with -y.

Besides ignoring rustc and cargo if they exist in $HOME/.cargo/bin, as described in #681, this PR also ignores them if they're found in $HOME/.multirust, a situation that popped up during testing. What do you think?

I was also unsure about the error message, so I ended up copying a large portion from a similar situation in do_pre_install_sanity_checks (if rustc_exists, to be specific). Better alternatives would be very appreciated.

Finally, thank you for your work on rustup, it's a very nice tool. :)

[meqif]

Hm, I didn't account for $HOME/.multirust staying in $PATH even when the homedir member of a test configuration is changed. This makes the tests fail when run with cargo test but not when run manually (i.e., target/debug/…).

I worked around this issue in 3897c2a by filtering any path entry that contains ".multirust", but it feels hacky.

[meqif]

Looking at Travis' build logs more closely, it seems a couple more entries in $PATH need to be filtered. I think extracting the closure used in filter and then using conditional compilation is a reasonable solution.

I'm not sure what to make of the test failures in Windows. I guess I'll boot up a virtual machine and debug it there.

EDIT: I was trying to use #[cfg(test)] and #[cfg(not(test))] to choose the proper function passed to filter, but it seems test is not set when running cargo test. 😕

[brson]

Thanks at @meqif, and thanks for writing tests!

From the various build failure here it's clear this needs to be considered carefully. Some observations:

The windows failure might be because the you are filtering out paths ending with ".cargo/bin" and windows uses backslashes canonically; though I'd be a bit surprised if the Path type didn't just handle that. You might consider just filtering out any path containing ".cargo" for simplicity.

For the error message I think we should print out the path to the binary that we found so that users can decide whether what rustup is thinking makes sense. Maybe it can say "of Rust at:", and the next line can print the path. I suspect this will look a bit ugly but it's a rare case so maybe it doesn't have to be beautiful (and rustup already has a lot of ui issues...).

By setting env::set_var here it's going to override PATH for all subsequent tests, creating an inconsistent environment depending on the order tests are run. Instead I'd suggest running your commands using the clitools::run method, which lets you set the environment. By using that method you won't get all the benefits of the expect_err helper though. It's a tradeoff. As a compromise I'd say you don't need to verify the entire output string, just that the process returns the correct value and includes some part of the error message.

The linux failures are interesting. The builders use a Rust not installed by rustup, so the test cases are finding those toolchains that are being used to test rustup and failing! Messy situation. The best idea I have to fix this is to use an environment variable just for the test suite to tell rustup-init to skip this sanity check (see the existing "RUSTUP_INIT_SKIP_SUDO_CHECK"), and set it for the entire test suite it clitools. The only wrinkle in here is that you somehow have to disable that environment variable for the three tests you do want that check running. For simplicity I'd say you can accomplish this by setting the "skip" environment variable in clitools::setup with env::set_var (instead of in clitools::env where the others are set), and then removing it again in your individual test cases. Setting the environment globally here is different than in the PATH case I mentioned before since PATH doesn't have any pre-existing content (also all these tests hold a mutex so they are run sequentially and there is no risk of them clobbering each others environment).

OK, I know that was a lot to digest, and it's kinda messy, but that's sorta the territory of this tool. Let me know if it makes sense.

[brson]

Oh, also I think it's fine to just filter out all paths containing ".multirust".

[meqif]

Thank you so much for the thoughtful feedback, @brson. 👍

The windows failure might be because the you are filtering out paths ending with ".cargo/bin" and windows uses backslashes canonically; though I'd be a bit surprised if the Path type didn't just handle that. You might consider just filtering out any path containing ".cargo" for simplicity.

That makes sense, you're probably right. I suppose it would have been fine if I used something like Path::new(".cargo").join("bin"), as Path would have used the correct separator in that case.

For the error message I think we should print out the path to the binary that we found so that users can decide whether what rustup is thinking makes sense. Maybe it can say "of Rust at:", and the next line can print the path. I suspect this will look a bit ugly but it's a rare case so maybe it doesn't have to be beautiful (and rustup already has a lot of ui issues...).

Good idea. I was doing that during the development of the PR, but removed it because it did look a little ugly. Oh well, now it's back.

By setting env::set_var here it's going to override PATH for all subsequent tests, creating an inconsistent environment depending on the order tests are run. Instead I'd suggest running your commands using the clitools::run method, which lets you set the environment. By using that method you won't get all the benefits of the expect_err helper though. It's a tradeoff. As a compromise I'd say you don't need to verify the entire output string, just that the process returns the correct value and includes some part of the error message.

Ah, thank you. I wasn't aware there was such a significant difference.

I ended up doing something similar to what you suggested. I checked where RUSTUP_INIT_SKIP_SUDO_CHECK was being set (clitools::env) and added RUSTUP_INIT_SKIP_PATH_CHECK there, then overrode the default value in the tests, like for PATH. I tried to keep this change as small as possible. Let's see if I didn't miss anything else. :)

[meqif]

There seems to be a (spurious?) issue with fetching one of ring's dependencies. What an annoying coincidence.

[meqif]

I tracked down that build issue and opened another pull request that fixes it: #711.

[brson]

Patch looks good to me! I think now that your rustls fixes are merged you'll need to rebase onto master to get travis to test this pr correctly.

[meqif]

Great! Let's hope nothing else goes wrong this time around. 💃

[meqif]

The tests pass in every Travis target, but not in Windows:

---- install_stops_if_cargo_exists stdout ----
    stderr: error: unable to read from stdin for confirmation
thread 'install_stops_if_cargo_exists' panicked at 'assertion failed: out.stderr.contains("it looks like you have an existing installation of Rust at:")', tests\cli-misc.rs:416
note: Run with `RUST_BACKTRACE=1` for a backtrace.
---- install_stops_if_rustc_exists stdout ----
    stderr: error: unable to read from stdin for confirmation
thread 'install_stops_if_rustc_exists' panicked at 'assertion failed: out.stderr.contains("it looks like you have an existing installation of Rust at:")', tests\cli-misc.rs:396

I assume I'm missing some special setup, but I'm not sure what it is. Any advice?

[meqif]

Since only the interactive tests fail, but not the non-interactive one, I've been looking at the tests in cli-inst-interactive for clues. PATH change aside, the major difference I noticed is that those tests are run with fake user input (e.g., "\n\n"), and that seems to be enough to avoid unable to read from stdin for confirmation in Windows.

So, I added a helper that makes the PATH modification and as "\n\n" as user input, similarly to cli-inst-interactive::run_input. The tests pass on my machine (OSX), so I'm going to push a commit with those modifications and hope it's enough to make Windows happy.

[meqif]

Oh bother, they still fail on Windows — the installation doesn't abort like it should. Well, at least I learned something.

[meqif]

I finally noticed that the file checks didn't account for EXE_SUFFIX. After fixing that, the tests pass on all targets except one Windows target, TARGET=x86_64-pc-windows-gnu, MSYS_BITS=64. More digging will follow. 🔍

[bors]

☔ The latest upstream changes (presumably #850) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #842) made this pull request unmergeable. Please resolve the merge conflicts.

[brson]

Thanks for the rebase @meqif.

The x86_64-pc-windows-gnu error looks legit. I haven't been able to reproduce it though because I don't have a working mingw environment :-/

[bors]

☔ The latest upstream changes (presumably #1012) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #1005) made this pull request unmergeable. Please resolve the merge conflicts.

[Diggsey]

Hi @meqif, are you still looking into this? Would you like any help debugging the test failures?

[meqif]

Hi @Diggsey! I've been trying to reproduce the test failure on TARGET=x86_64-pc-windows-gnu, MSYS_BITS=64 whenever I get the chance, without much success. I plan on trying again this weekend again.
I'd love if someone tried their hand at finding and fixing that elusive issue, though.

[meqif]

@Diggsey @brson Looks like some other commit in master fixed the issue I was having in this branch. I'd say it's good to go, unless you have any objections. 😄

[Diggsey]

Looks good to me, but it's slightly worrying that this code will effectively be untested, since we have to disable the path check on CI, and if it goes wrong it will prevent rustup from being installed.

@brson thoughts?

[brson]

@meqif oh wow, thanks for sticking it out this long.

@Diggsey I think I'm fine with it since there are a few tests where the path check is enabled. Whatever, let's just go for it!

[meqif]

It was a long fight, but it was worth it. 💪 Thanks for merging!