-
Notifications
You must be signed in to change notification settings - Fork 914
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
Comments
This part was done: |
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. |
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. |
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". |
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!” |
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. |
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. |
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. |
@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? |
This breaks CI workflows that rely on installing Rustup and assuming the |
@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).
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. |
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 |
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.
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 |
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. |
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. |
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:
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.) |
Thank you so much for all your work. It is truly and deeply appreciated. |
+1 to what @sunshowers said, and an additional thank you for taking this feedback constructively! |
Another +1 to that sentiment!
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.?
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?” |
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 Similarly, I wouldn't like it either that you update a dependency in Requiring manual 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 I see there is some potential security risk, where 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 |
I've created a tracking issue for keeping track of the 1.28.1 progress: |
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.
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 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! :-) |
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! 💛💐 |
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:
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.
The text was updated successfully, but these errors were encountered: