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

Implicit install breakage falls below usual project standards #4211

Open
tmandry opened this issue Mar 3, 2025 · 24 comments
Open

Implicit install breakage falls below usual project standards #4211

tmandry opened this issue Mar 3, 2025 · 24 comments

Comments

@tmandry
Copy link
Member

tmandry commented Mar 3, 2025

Hi folks, I just learned about the breaking change in #3985 as it was released today. I can understand why this change might be desirable, but I have concerns that the change was released without the kind of messaging or feedback mechanism that Rust users have come to expect from the project.

Rustup is an incredibly useful and valuable tool for Rust, forming the entry point of the vast majority of users' experience with Rust. Of course, that kind of importance means we must be careful. The kind of experience that Rust tries to prevent with its stability guarantee is "this used to work and it doesn't anymore". Each of these is frustrating to users and they are usually experienced "in anger". Each time we create them, we are taking out a loan against Rust's reputation with the expectation that Rust will get significantly better as a result of the change.

I do see that there was care to add an error message prompting the user with the exact command to run - that is in line with what users expect from Rust, and is highly appreciated. At the same time, there were two steps not taken that I have considered standard for any breaking change on the lang team:

  • A final comment period run with @rfcbot that indicates a supermajority of the team is on board with the change. This gets published in places like This Week in Rust, giving users the opportunity to voice their opinion and team members to raise an objection on their behalf.
  • For any change with significant disruption (i.e. any nontrivial number of broken crates in crater), starting with a future compatibility warning that gives users time to update ahead of the breaking change.

Additionally, some changes are accompanied with a dedicated blog post ahead of the change explaining the rationale for the change with instructions to users. Going through this process is more work, but I would argue it is not worth making a disruptive change without taking these steps.

Finally, I suspect this situation is arising because we don't have a project-wide standard for how breaking changes are handled. I have seen a need for this in other recent situations too; this just happens to be the one I'm commenting on. In parallel with this conversation, I would ask @rust-lang/leadership-council to adopt such a standard – even a basic one we can iterate on is better than none. I think it rises to the level of the leadership council because of the reputational risk each breaking change creates for Rust, and we should not expect individual teams to have to decide for themselves how to navigate that risk without guidance.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 3, 2025

some changes are accompanied with a blog post explaining the rationale for the change with instructions to users.

This part was done:

https://blog.rust-lang.org/2025/03/02/Rustup-1.28.0.html

@tmandry
Copy link
Member Author

tmandry commented Mar 3, 2025

What I meant was more like a dedicated blog post ahead of the change (I edited the text to clarify). There was a blog post announcing the release and describing the change, but it didn't include rationale.

@tmandry tmandry changed the title Implicit install breakage falls below typical project standards Implicit install breakage falls below usual project standards Mar 3, 2025
@str4d
Copy link

str4d commented Mar 3, 2025

some changes are accompanied with a blog post explaining the rationale for the change with instructions to users.

This part was done:

https://blog.rust-lang.org/2025/03/02/Rustup-1.28.0.html

The blog post contains instructions to users (albeit without any mention of how to update CI systems specifically, which is the primary instantaneous breaking surface), but does not explain the rationale for the change.

@tmandry
Copy link
Member Author

tmandry commented Mar 3, 2025

I do agree with most of what's suggested in #4210 as a remedy, with a caveat that bugfixes that are technically breaking changes are allowed.

I'm not sure if this falls into the category of "bugfix" though; if I could sum up the rationale I saw in #3635 it amounted to something like, "the previous maintainers had one philosophy (prioritize convenience) and the current maintainers have a different philosophy (prioritize predictable behavior), so we're changing the behavior".

@chriskrycho
Copy link

Concur re: the distinction about bug-fixes-that-are-breaking being allowed (I’ll be the first to say that SemVer is hard!), and that it’s hard for me to frame this as a bug-fix.

Re: blog post—as @tmandry notes (and as I suggested on #4210), it would be really good to get feedback on this before rolling it out, and other teams have started doing that for these kinds of things. But I’ll also second @tmandry in noting that this is likely a thing we need to solve for at a project guidance level. From my POV this not at all a “you screwed up, boooo!” attitude for rustup, but rather an “ah, this exposes a gap we’ve had as a project in lacking clarity about how to approach things like this!”

@sunshowers
Copy link

I really appreciate everything the rustup team has been doing here, and on balance I support the change. Thank you for making it happen!

I don't want to add to the pile on, but I wanted to gently suggest maybe putting this behavior behind an edition boundary. I hate suggesting this because the 2024 edition is now out, but I think this might benefit from being behind an opt-in flag in rust-toolchain.toml and then becoming the default for the 2027 edition.

@ChrisDenton
Copy link
Member

ChrisDenton commented Mar 3, 2025

Just on a procedural note, call for beta testing/feedback happened here https://internals.rust-lang.org/t/seeking-beta-testers-for-rustup-v1-28-0/ and this was cross-posted to e.g. reddit, etc.

@weihanglo
Copy link
Member

  • A final comment period run with @rfcbot that indicates a supermajority of the team is on board with the change. This gets published in places like This Week in Rust, giving users the opportunity to voice their opinion and team members to raise an objection on their behalf.

FWIW, FCP doesn't always go to TWiR. There is a call-for-testing section though it needs an existing-RFC to trigger the automation. Otherwise you can only open PR to TWiR for including the change.

@tmandry
Copy link
Member Author

tmandry commented Mar 3, 2025

@ChrisDenton I appreciate that the team tried to solicit feedback. I haven't found beta testing to be very helpful for this kind of feedback unfortunately, though it looks like it may have surfaced bugs.

@weihanglo I see.. there's a section with issues tagged final-comment-period on rust-lang/rust, but I guess issues from this repo aren't included there?

@Rigidity
Copy link

Rigidity commented Mar 3, 2025

This breaks CI workflows that rely on installing Rustup and assuming the rust-toolchain.toml file will be respected automatically. That said, the dtolnay/rust-toolchain workflow skips installing Rustup if it's already installed, and the GitHub runners probably have an older version still. Therefore, unless you're using a runner without it preinstalled the fallout will be minimized until GitHub updates all of their actions to the new version.

@weihanglo
Copy link
Member

@tmandry yes, and issues in rust-lang/cargo aren't included either (and that's totally okay because the cargo team use FCP as a general polling tool).

For any change with significant disruption (i.e. any nontrivial number of broken crates in crater), starting with a future compatibility warning that gives users time to update ahead of the breaking change.

Note that the breakage is mostly in CI. Only a few people check non-fatal warnings in successful CI logs. The mechanism helps, but doesn't really help much IMO.

To me, one of the critical issues is lacking of opt-in testing process for rustup. For other Rust tools we rely on rustup to do that, but rustup itself cannot.

@Rigidity
Copy link

Rigidity commented Mar 3, 2025

Personally, widespread breaking changes like this should be a major version bump, and specified as part of the URL passed into cURL. So for instance, you would go from sh.rustup.rs to sh2.rustup.rs for this version, making updates explicit

@iliana
Copy link

iliana commented Mar 3, 2025

On the topic of a call for feedback, I would like to echo the ask to post to blog.rust-lang.org (either the main blog or the "Inside Rust" blog) in the future ahead of major behavior changes in rustup, or provide some other way that allows me to see upcoming rustup changes in my RSS reader. As someone who is mostly an end-user of the project I do not find myself lurking the Rust Internals forum (and there doesn't seem to be any categorization that would allow me to get something other than a firehose in my RSS reader), and I don't read Reddit. I know that the methods for getting word out to end-users is somewhat fragmented but it seems so surprising to not see this behavioral change announced ahead of release on blog.rust-lang.org.

(There is also no better way to get the word out than to have rustup itself loudly warn users that the implicit behavior is changing over a period of a few months!)

I think this flag day didn't impact many, many more users in CI is because rustup is cached on GitHub Actions runners, and rustup appears to self-update only after installing the requested toolchain. The next time GitHub updates their image there will be more breakage.

To me, one of the critical issues is lacking of opt-in testing process for rustup. For other Rust tools we rely on rustup to do that, but rustup itself cannot.

Another problem that compounds this issue is that it's difficult to lock to a particular rustup release (to mitigate against behavior changes in CI in the same way rust-toolchain.toml and Cargo.lock do) without rolling the installation process yourself. While investigating CI issues we started having this morning I found there wasn't a way to specify the version of rustup using rustup-init.sh. I can file a separate issue and contribute that change if that sounds useful?

@djc
Copy link
Contributor

djc commented Mar 3, 2025

We've been discussing and I am inclined to figure out a way to revert this release or to roll out a 1.28.1 with this behavior change rolled back. That would buy us some time to figure out a better migration strategy.

@mitsuhiko
Copy link

I think it would be better to roll this back. The good thing is that because a lot of CI is not yet on the latest version there is some period to avoid breaking more people than it already has.

In particular I did not even know that I rely on that behavior in a wrapper to that invokes cargo until it stopped working.

I'm wondering if this should become a configuration option (envvar or similar) that can be set to re-enable the old behavior which I think is for CI uses in particular the better one. Then the github action could automatically set that flag.

@djc
Copy link
Contributor

djc commented Mar 3, 2025

Sorry to everyone who suffered from breaking changes due to this, and thanks for all the constructive feedback. I agree that the way this went down isn't up to the standards we usually expect from Rust project products.

Apparently we don't have good tools in place to revert to 1.27.1, and it also probably makes sense to keep most of the other changes intact while we come up with a better plan for dealing with this migration. So I think the way forward will involve a 1.28.1 release with the following behavior changes:

  1. The new way should keep working, to avoid penalizing people who already migrated.
  2. The old way should also work and generate some warnings, with a link to leave feedback.
  3. There should be an environment variable that disables the old way.
  4. Maybe there should also be a setting (as in ~/.rustup/settings.roml) to disable the old way?
  5. Address the incorrect suggestions as described in Recommended toolchain install command line isn't exactly right #4212

After 1.28.1 has been released, we should do more outreach, including a blog post on the Rust blog that specifically discusses the proposed change in behavior (calling it out in the title).

As for the discussion about calling this 2.0 -- that has been considered. Personally I don't think it would have substantially altered the amount of breakage, but I'm certainly not opposed to it if people think it makes a big difference.

(Note that there's also the structural issue of this load-bearing tool -- parts of which are 10 years old, and developed by a discontinuous group of people -- now being maintained by three volunteers who all have many other responsibilities.)

@sunshowers
Copy link

Thank you so much for all your work. It is truly and deeply appreciated.

@tmandry
Copy link
Member Author

tmandry commented Mar 3, 2025

+1 to what @sunshowers said, and an additional thank you for taking this feedback constructively!

@chriskrycho
Copy link

Another +1 to that sentiment!

Note that there's also the structural issue of this load-bearing tool -- parts of which are 10 years old, and developed by a discontinuous group of people -- now being maintained by three volunteers who all have many other responsibilities.

10,000% acknowledged, and props to all of you for quickly responding and working to make a path forward here. I’ve been on the other side of that and it can be a lot!


I also think the proposed path forward makes good sense, particularly if you can get out a fix that gets the old behavior working again quickly and then iteratively knock down the other parts of the list for a 1.28.2 etc.?


Personally I don't think it would have substantially altered the amount of breakage, but I'm certainly not opposed to it if people think it makes a big difference.

I think on its own it would not have, given the distribution mechanism. However, as a signaling mechanism, it would matter, and combined with other suggested tweaks in approach, like getting a blog post out ahead of time to announce it as likely change, warning about it in rustup itself with significant lead time, and generally rolling it out more slowly. SemVer is a sociotechnical contract, and the “socio-” part of that is super important! It’s not just “how much breakage does it cause?” (though that is obviously the most significant factor) but also “what are we communicating about any breakage?”

@est31
Copy link
Member

est31 commented Mar 4, 2025

Personally, I don't think one should break the implicit install workflow at all, phased in gently or not: being able to git clone a project and run cargo build is really useful, without one needing to worry about installing that project's toolchain. rustup should get out of your way, instead of requiring people to acknowledge its existence each time the pinned rust version changes. That's at least what I like about it.

Similarly, I wouldn't like it either that you update a dependency in Cargo.toml, you'd need to run cargo update-lockfile manually, or run cargo download when there is dependencies in the lockfile that aren't present on local disk.

Requiring manual rustup toolchain install reminds me a bit of git submodules. After a git checkout, you still need to run git submodule update --init to get the right versions of the submodules (and if submodule got removed, you need to rm the dir manually). A lot of people stumble over that issue with git, people often make PRs where they forgot to run that and then commit a change to an older version of the submodule.

This would have similar effects. People might be wondering why their editor-integrated rustfmt doesn't work any more (editors don't have good UI around this), or why their makefile breaks that invokes rustc manually.

It seems that the reason for the drop of the implicit install was to support riced prompts. Personally, that feels like a niche use case to me, at least compared to the more usual case of people wanting to run cargo build or cargo install --path . after a checkout. And even here it's not automatic that a download is harmful: it's just downloading things, not compiling or anything.

I see there is some potential security risk, where rust-toolchain.toml could contain a pointer to an executable in a local path, one could have that local path contain a malicious executable and the prompt auto-execution of rustc --version would execute that. But then we should actually put the "local path" feature of rustup behind a command you need to run manually, not the "download official binaries from the internet" feature. With any security feature, if you require approvals too often, you will end up with more and more users who click "accept" to just be able to continue with their business.

Still, I think it's worth to support opting out of implicit installs, one but not enough to degrade the experience of everyone else. Maybe one could add something like a +no-install-toolchain modifier to opt out of implicit installs? To make that work with rustc that's not from rustup, one could even add support of that flag to rustc itself.

@djc
Copy link
Contributor

djc commented Mar 4, 2025

I've created a tracking issue for keeping track of the 1.28.1 progress:

@Jasper-Bekkers
Copy link

I'd like to add my 2ct's here at least in the hope that it doesn't end up either spamming this issue too much, but also doesn't disappear into the ether.

The new way should keep working, to avoid penalizing people who already migrated.

I, personally, don't think the new way should keep working. Or rather, I'd love it if the step that the computer did automatically for me (and the team) without thinking, stays entirely automatic. It's the reason we check in a rust-toolchain.yaml in the first place: it made doing compiler version upgrades across our team super easy.

The new behavior, however, introduces additional friction to the upgrade process (we now have to remember to update all our CI images, every team member needs to remember to update their setup or get reminded by this tool to update their setup). The old behavior was exactly what I wanted this tool to do.

I understand this is a community maintained tool, with volunteers working on it and I'm not in a place to ask for much; however, I'd still like this feedback to at least be heard. Thanks! :-)

@cratelyn
Copy link

cratelyn commented Mar 4, 2025

Thank you so much for all your work. It is truly and deeply appreciated.

another +1 to what @sunshowers said, above. i greatly appreciate all of your collective time and effort, and that this conversation is happening both thoughtfully and constructively. thank you! 💛💐

@djc
Copy link
Contributor

djc commented Mar 5, 2025

1.28.1 has been released, it brings back implicit toolchain installation. I've created #4231 to track further changes for 1.28.2, which we will likely release soon depending on further feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests