Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Project: simplification #1611

Open
1 task
nrc opened this issue Jan 11, 2019 · 22 comments
Open
1 task

Project: simplification #1611

nrc opened this issue Jan 11, 2019 · 22 comments
Labels
meta This issue is related to project management.
Milestone

Comments

@nrc
Copy link
Member

nrc commented Jan 11, 2019

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

Tasks

  1. 3 of 7
    enhancement meta tracking
@dwijnand
Copy link
Member

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

@h-michael
Copy link
Contributor

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.

@alexcrichton
Copy link
Member

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

@kinnison
Copy link
Contributor

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

@seanmonstar
Copy link
Contributor

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?

@dwijnand
Copy link
Member

dwijnand commented Feb 8, 2019

@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: #1644

@dwijnand
Copy link
Member

dwijnand commented Feb 8, 2019

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

@nrc
Copy link
Member Author

nrc commented Feb 10, 2019

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).

@dwijnand
Copy link
Member

dwijnand commented Feb 10, 2019

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).

@das-sein das-sein mentioned this issue Feb 12, 2019
@das-sein
Copy link
Contributor

@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.

@SimonSapin
Copy link
Contributor

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.)

@petrochenkov
Copy link
Contributor

cc @brson

@brson
Copy link
Contributor

brson commented Feb 12, 2019

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
Copy link
Contributor

brson commented Feb 12, 2019

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

@brson
Copy link
Contributor

brson commented Feb 12, 2019

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.

@dwijnand
Copy link
Member

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!

@ghost ghost mentioned this issue Feb 20, 2019
@mati865
Copy link
Contributor

mati865 commented Feb 20, 2019

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.

@joshtriplett
Copy link
Member

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

@yodaldevoid
Copy link
Contributor

rustup toolchain link is also very useful for rustc development.

@kinnison
Copy link
Contributor

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

@Xanewok
Copy link
Member

Xanewok commented Feb 28, 2019

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.

@pickfire
Copy link
Contributor

pickfire commented Oct 2, 2019

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

EDIT: #1570

@rami3l rami3l added this to the On Deck milestone Feb 15, 2024
@rami3l rami3l added the meta This issue is related to project management. label Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta This issue is related to project management.
Projects
None yet
Development

No branches or pull requests