Automatically install override toolchain when missing.

[SimonSapin]

A typical scenario is:

• I work on a repository that uses rust-toolchain to pin to a specific Nightly version
• I run git pull, rust-toolchain has been changed to update to a new Rust version
• I run cargo build

Result before this PR (typically): rustup fails with an error like:

error: override toolchain 'nightly-2017-08-31' is not installed
info: caused by: the toolchain file at '/home/simon/projects/servo/rust-toolchain' specifies an uninstalled toolchain

A better result would be to automatically install toolchains as needed.

Closes #1218

[aturon]

cc @nrc @Diggsey @alexcrichton @withoutboats -- any of you up for reviewing?

[Diggsey]

I'm not that comfortable having it automatically install the new toolchain, I can imagine that would be unacceptable to some people. I can see a few alternatives:

1. Prompt before installing the missing toolchain, overridable with -y or similar.
2. Fail as before, but print out the command needed to install the missing toolchain.
3. Have it be a global config option, "automatically install missing toolchains"

Would one or more of those work for you?

@alexcrichton thoughts?

[SimonSapin]

Servo tries to keep up with Rust nightly, which means upgrading several times a month. I think it is important that this is as seamless as possible for regular contributors. Pulling from master and rebuilding should Just Work.

So I think option 2 is not particularly useful.

Option 1 seems tricky for two reasons: this code path is often used in non-interactive environments (e.g. Travis-CI) where prompting would just hang. As to the command line argument, I don’t know where it should go for proxy executables like cargo.

Option 3 sounds like that config should be owned by the user and Servo’s build system should not mess with it.

What do you think of an environment variable?

[SimonSapin]

Should all this apply only to rust-toolchain (whose "source of truth" is often an upstream repository) but not other forms of overrides (which are controlled by the local user)?

[nrc]

Given that the behaviour is only triggered by building a project with a toolchain file, it seems to me that the user has already opted-in. I don't think this is difference from Cargo downloading a crate for a build. At most we should get the user's consent once per project (e.g., the first time you build Servo, rustup asks if it should download a toolchains, user can then opt-in to auto-downloads or get errors, but I don't think even that is necessary.

[SimonSapin]

I agree that this is similar to automatically downloading dependency crates.

@Diggsey, what do you think of that point of view, or of the environment variable approach?

[SimonSapin]

CC @alexcrichton

[alexcrichton]

I personally agree with @nrc that if this is only used in conjunction with rustup.toml then it seems reasonable to automatically download the appropriate toolchain rather than requiring an explicit update step.

In reading this, though, I had a few questions:

• Does this only trigger with rustup.toml? In reading the tests it looks like this happens regardless of the presence of a rustup.toml.
• Could a test be added explicity for this behavior?

[SimonSapin]

What is rustup.toml? It is not mentioned in https://github.com/rust-lang-nursery/rustup.rs/blob/master/README.md

This PR is relevant when:

• A rust-toolchain file exists in the current directory or an ancestor,
• Or the rustup override command has been used to set up an override for the current directory or an ancestor,
• Or the RUSTUP_TOOLCHAIN environment variable is set.

Each of these cases end up going through the same code path.

[alexcrichton]

@SimonSapin oh sorry, I meant the rust-toolchain file you pointed out to me on IRC.

It sounds like though it's already working! Sounds good to me that we'd also include overrides and the env var.

Can you add a test (or maybe there already is one?) about rustc +beta returning an error if the beta toolchain isn't installed? (with no other override/toolchain file/etc business). Could you also add a test which explicitly includes the rust-toolchain file and auto-install behavior?

[SimonSapin]

I’ve pushed a new commit with the requested tests.

[alexcrichton]

@bors: r+

Thanks!

[bors]

📌 Commit eae3131 has been approved by alexcrichton

[bors]

⌛ Testing commit eae3131 with merge 768f9b8...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 768f9b8 to master...

[SimonSapin]

Could we get a new release with this change? Can I help?

[alexcrichton]

Sorry I've been slow to respond, I will try to get to a release when I get a chance