Add rustfmt to the tools list

[nrc]

r? @alexcrichton

We should probably add something for cargo-fmt too once rust-lang/rust#46031 lands, but I'm not sure if we can just add it to the list or not, seeing as it is not its own component.

[alexcrichton]

Thanks! Mind detailing a bit what happens if we land this? Does rustup auto-create shims for all these tools? Do we know what happens when rustup encounters a file that already exists?

I'm a little worried that the upgrade experience here won't be great (you update rustup and all of a sudden rustfmt stops working) but maybe because it's nightly-only it's ok?

[nrc]

The list is used for creating hard links - rustup creates a hard link for each name in the list to the rustup executable. The same list is also used for cleaning up the old multirust stuff and for uninstall. If a file exists where rustup wants to hardlink, then it just removes it.

I guess that is pretty sucky for people who have cargo installed rustfmt, but the fix is pretty easy. However, this would also affect people using the Syntex version of rustfmt, so isn't limited to nightly users.

I'll have a think about if we can improve this...

[alexcrichton]

Oh so I don't think it's critical we think too hard about this, but rather we can just be clear about the messaging. For a "true lossless experience" we'd have to wait for rustfmt-preview to hit the stable channel I think (right now I think it's only beta/nightly?). That is, for all beta/nightly users there's a "better method" of installing the rls which we'll manage from now on if they follow a few simple steps (albeit not automatic steps, but hey rustfmt is pre-1.0).

I'd personally be ok doing that but if you think we should be more cautious I'm ok with that as well!

[nrc]

I think we can't land as is - everyone who has rustfmt installed will have it broken when the rustup update with no messaging about the issue or how to fix it. I'm not too worried about stable users, there are probably a few left, but that version of rustfmt is really old.

I don't think we can do this only when the user adds the rustfmt component without some major re-jigging of rustup. Two ideas:

• add a temporary, opt-in flag, e.g., rustup update --rustfmt which adds the shims, by default don't add rustfmt
• detect existing rustfmt and display a warning and continue y/n prompt (do people run rustup update on CI or other automated environments? Could this cause problems there? I suppose we could just issue a warning and do it anyway without a prompt).

[alexcrichton]

Actually yeah a warning sounds great to me! Something like "that binary already exists, please remove it and then add the rustup component". Presumably rustup would just automatically create the binary if it doesn't exist on each invocation?

[Diggsey]

Bear in mind that the component may well not exist for many of the toolchains the user has installed, assuming it has only just been added. Am I right in thinking rustfmt doesn't support stable rust yet? So at least you won't have to wait for the component to land on stable, but you may still want to leave some time between adding the component, and enabling the warning and proxy in rustup.

[nrc]

but you may still want to leave some time between adding the component, and enabling the warning and proxy in rustup.

It's already been a few weeks since we added the component and it is available on beta.

[nrc]

@alexcrichton ok, this is a version with a warning. It was a bit more complicated than I thought because how do we distinguish rustfmt that was installed by the user from rustfmt installed as a shim by rustup? So I added some token files to hack around the issue. It is not clear to me how tool shims actually get deleted, but it looks like rustup nukes the Cargo directory, which makes the rustfmt thing a non-issue. I've not done anything with the Windows installer because I figured the number of stable Rustfmt users on Windows will be very small.

[alexcrichton]

@nrc oh these are all hard links I think, so I believe same-file should do the trick? (the crate)

[alexcrichton]

Would it be possible to add a test for this as well to ensure the UI looks right and the file isn't clobbered?

[nrc]

I'm not clear on the details here, but it seems like rustup may make hard links or soft links, depending on the platform, would the same-file trick still work? And what could we compare the file to using same-file?

[alexcrichton]

Oh if it's symlinks then it wouldn't work yeah, and I was thinking we'd compare to rustup itself. Perhaps in the meantime we could just compare file sizes? (seems unlikely they'd be byte-for-byte the same size)

[nrc]

This version compares the size of the possible links to try and work out if they are rustup shims or real files.

[alexcrichton]

@bors: r+

Looks great!

[bors]

📌 Commit 93a5569 has been approved by alexcrichton

[bors]

⌛ Testing commit 93a5569 with merge 4c23be5...

[bors]

💥 Test timed out

[alexcrichton]

@bors: retry

[bors]

⌛ Testing commit 93a5569 with merge 9f013e8...

[nrc]

is bors stuck? It shouldn't take 10 hours, right?

[bors]

💥 Test timed out

[alexcrichton]

urgh well it passed anyway

[mcheshkov]

So, with this released, how should one install rustfmt on a clean environment? Pass --force to cargo install?
I hope this is the right place to ask this kind of questions.

$ docker run -it --rm ubuntu:xenial-20171201
# apt-get update
# apt-get install build-essential curl
# curl https://sh.rustup.rs -sSf | bash -s -- --default-toolchain nightly -y
# rustc -V
rustc 1.24.0-nightly (169929308 2017-12-23)
# rustfmt 
error: toolchain 'nightly-x86_64-unknown-linux-gnu' does not have the binary `rustfmt`
# cargo install rustfmt-nightly
    Updating registry `https://github.com/rust-lang/crates.io-index`
 Downloading rustfmt-nightly v0.3.4                                             
  Installing rustfmt-nightly v0.3.4                                             
error: binary `cargo-fmt` already exists in destination
binary `rustfmt` already exists in destination
Add --force to overwrite

[nrc]

@mcheshkov eventually, the right way will be to rustup component add rustfmt-preview, but I think there is still a bug or two to deal with. In the meantime, cargo install rustfmt-nightly --force should work.