rustup icon indicating copy to clipboard operation
rustup copied to clipboard

Project: simplification

Open nrc opened this issue 7 years ago • 22 comments

We could simplify Rustup a lot. It was built with some very ambitious goals and it is clear that some of things will never happen, but the code is designed to accommodate them causing unnecessary complexity. It is also backwards compatible with very old versions of Rust and Rustup. There is lots of opportunity to reduce complexity. Some ideas:

  • remove telemetry (this has never been used and is not that useful in any case)
  • remove support for v1 manifests (we should still detect and error out with a message to delete things manually or whatever)
  • support fewer ways to install Rust
  • use standard logging rather than notifications
  • merge some of the crates (probably only rustup and rustup-dist)

cc @rust-lang/cargo for any suggestions of things we should/should not do

nrc avatar Jan 11 '19 08:01 nrc

I'd be happy to assist here. But I've never hacked on Rustup so I'd need a little babysitting.

dwijnand avatar Jan 11 '19 09:01 dwijnand

I think

  • we may delete code for compatible with multirust
  • may we need reqwest backend download?
  • if we need only curl support, we maybe merge download crate into rustup-utils

My suggestion may be besides the point, sorry.

h-michael avatar Jan 11 '19 10:01 h-michael

I'd personally recommend removing all but the curl backend that @h-michael mentioned, and then I'd also look to simplify filesystem transaction handling code. IIRC it's overly complicated and can be implemented far more efficiently than it is today

alexcrichton avatar Jan 11 '19 17:01 alexcrichton

This kind of simplification sounds sensible. If you're interested in non-core contributions toward this then I'm very interested in helping.

kinnison avatar Jan 15 '19 21:01 kinnison

I'd actually recommend removing the curl backend and using only reqwest. Is there some reason we can't have a Rust tool use more Rust and less C?

seanmonstar avatar Jan 16 '19 00:01 seanmonstar

@nrc @alexcrichton could you give more information regarding "support fewer ways to install Rust" and "simplify filesystem transaction handling code" please? If it's likely to generate a back-and-forth, then perhaps in its own issue.

Meanwhile I've opened an issue dedicated to the crate merging: https://github.com/rust-lang/rustup.rs/issues/1644

dwijnand avatar Feb 08 '19 07:02 dwijnand

Also, it's probably good to spin out the curl vs reqwest discussion into a thread and come to a conclusion.

dwijnand avatar Feb 08 '19 07:02 dwijnand

support fewer ways to install Rust

iirc, there are a lot of ways to install Rust via Rustup but the only ones anyone ever uses are dist and the windows installer (which is somewhat separate any way). devs do sometimes install from local disk, but I think that might be able to be simplified by separating it and handling it differently (we can certainly make breaking changes in this case).

nrc avatar Feb 10 '19 05:02 nrc

rustup toolchain install <toolchain>... is the common case.

For rustc devs they also use rustup toolchain link <toolchain> <path> (as documented also in https://rust-lang.github.io/rustc-guide/how-to-build-and-run.html#creating-a-rustup-toolchain), and I think we should keep.

Other than that rustup help toolchain does say:

but rustup can also install toolchains from the official archives, for alternate host platforms, and from local builds.

But I don't exactly see how or where that happens in the code (I'm looking at rustup_mode::update).

dwijnand avatar Feb 10 '19 11:02 dwijnand

@h-michael @alexcrichton @seanmonstar Created a PR for the removal of the cURL backend. Figure discussion could happen there with respect to @dwijnand 's request for a place to hold one.

das-sein avatar Feb 12 '19 12:02 das-sein

remove support for v1 manifests

Were v2 manifests generated for older releases? I think we shouldn’t break the ability for future rustup.rs to install, say, Rust 1.2.0. (I’ve used this to find old regressions.)

SimonSapin avatar Feb 12 '19 17:02 SimonSapin

cc @brson

petrochenkov avatar Feb 12 '19 17:02 petrochenkov

Were v2 manifests generated for older releases? I think we shouldn’t break the ability for future rustup.rs to install, say, Rust 1.2.0. (I’ve used this to find old regressions.)

@SimonSapin afaik, no

I'm in agreement with everything in the op, except for possibly removing v1 support.

The multi-backend support was a pet project just because I wanted to see if I could support all the HTTP/HTTPS implementations, particularly to support production usage of rustls. It's fine to strip down to just reqwest, assuming nobody is using the curl backend - I know at some point somebody needed to explicitly use the curl backend because reqwest didn't work. It's probably reasonable to remove it and see if anybody complains (though if they do complain there's no recovery - they'll need to downgrade until the subsequent release that re-adds curl).

multirust and very-old rustup compat code can probably be deleted at this kind, though if possible, emitting an error would prevent rustup from doing something destructive in the case those scenarios do come up.

Re multiple ways for rustup to install rust, the only ways I recall are from the xz-tarballs and gz-tarballs (even on windows). I don't recall rustup having windows installer support, though there is a rustup-win-installer subdirectory that I assume still doesn't do anything useful. That can be deleted.

The gz support can't be removed as long as rustup supports pre-xz toolchains.

I don't have any insight into whether people use rustup toolchain link or not, though I do know I was pretty close to using it the other day for testing custom rust toolchains, but got distracted before I did.

The notification system should be dropped. I agree it's unnecessarily complex.

brson avatar Feb 12 '19 23:02 brson

Telemetry is fine to remove, though I still think having telemetry somewhere is a good idea.

brson avatar Feb 12 '19 23:02 brson

I left a comment on the curl-removal PR about the prudence of both making reqwest the default and removing curl in the same release.

brson avatar Feb 12 '19 23:02 brson

very-old rustup compat code

If you could give some more specifics or pointers that would be great.

though there is a rustup-win-installer subdirectory that I assume still doesn't do anything useful. That can be deleted

Oh!

Thanks, @brson!

dwijnand avatar Feb 13 '19 00:02 dwijnand

I don't have any insight into whether people use rustup toolchain link or not, though I do know I was pretty close to using it the other day for testing custom rust toolchains, but got distracted before I did.

It's convenient way to use Rust packaged by Linux distributions. Not like I'm using it often but it comes handy.

mati865 avatar Feb 20 '19 13:02 mati865

I'm using rustup toolchain link system /usr for that exact purpose.

joshtriplett avatar Feb 20 '19 15:02 joshtriplett

rustup toolchain link is also very useful for rustc development.

yodaldevoid avatar Feb 28 '19 02:02 yodaldevoid

So I think rustup toolchain link is important to keep; certainly I wouldn't advocate removing it.

I do wonder if there might be something even more nice we could do for supporting system-managed toolchains (e.g. converting components, targets, etc, into package names and reporting useful commands such as apt install rust-rls or somesuch should a proxy be run which isn't installed for the system toolchain.

Of course, this doesn't quite fall under the topic of simplification, so I'll shut up now :D

kinnison avatar Feb 28 '19 20:02 kinnison

Agreed on rustup toolchain link; I'd even go as far and possibly better integrate the local workflow.

I know I wanted to rustup component add XYZ in the past. This could possibly run ./x.py dist XYZ under the hood or we could change the way these components are built inside Rust repo so that rustup can understand the resulting artifacts and be able to use them for rustup component add purposes.

Xanewok avatar Feb 28 '19 21:02 Xanewok

~~Can there be +nightly in rustup to keep it similar to cargo rather than using --toolchain=nightly?~~

EDIT: https://github.com/rust-lang/rustup.rs/issues/1570

pickfire avatar Oct 02 '19 13:10 pickfire