-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[all OSs] Pin Rust to 1.89.0 due to a 1.90.0 breaking change #13044
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
[all OSs] Pin Rust to 1.89.0 due to a 1.90.0 breaking change #13044
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR pins Rust to version 1.89.0 across all operating systems due to a breaking change introduced in Rust 1.90.0. The change is a temporary workaround while waiting for resolution of reported issues in the Rust and runner-images repositories.
- Pin Rust toolchain from "stable" to "1.89.0" across Windows, Ubuntu, and macOS
- Add explanatory comments linking to relevant GitHub issues
- Ensure consistent behavior across all platforms until the breaking change is resolved
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| images/windows/scripts/build/Install-Rust.ps1 | Pin Rust version and add explanatory comment for Windows installation |
| images/ubuntu/scripts/build/install-rust.sh | Pin Rust version and add explanatory comment for Ubuntu installation |
| images/macos/scripts/build/install-rust.sh | Pin Rust version and add explanatory comment for macOS installation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@erik-bershel This isn't actually a breaking change! The original issue in #13041 had compiler errors because the developer likely enabled error-on-warning. As you'll note in the discussion at rust-lang/rust#145936, the change in behavior is the reporting of additional warnings, and isn't a breaking change in the compiler. In fact, the discussion in that issue seems to indicate that this new behavior will not be changed, instead it is a valid lint that encourages users to adjust their code. You'll see that other developers in the Rust ecosystem updated their code to resolve these lints: dtolnay/thiserror#423 |
|
Hey @shadaj! |
There's no indication from the Rust project that this will ever be fixed. Rust takes breaking changes very seriously, so it seems strange to make this decision. I truly do appreciate the work y'all do to maintain a stable build environment, so just want to help towards that goal :) The issue is, it's arguably a breaking change to pin users the other way. If a Rust user works on code on their laptop with an updated toolchain, they can use APIs stabilized in 1.90 that were not available in prior versions. But on GHA they will get build errors. I'm sure there are projects that will already face this if they started adopting 1.90 features (and haven't pinned the toolchain themselves). |
|
Hello! I'm a co-lead of the Rust infrastructure team. I'm posting as an interested party and semi-officially: I've gotten light agreement but haven't sought a project-wide consensus on this post. As always, GitHub Actions should do what it thinks is best for its broad range of users, including not updating to the newest version of Rust. However, the Rust project would like to take the opportunity to share our views about this case. Rust's general policyThe Rust project does not consider new warnings to be breaking changes as warnings do not prevent code from compiling or running. For example, Rust 1.89 introduced a new warning that affected a comparatively large chunk of the ecosystem. End users may opt-in to converting warnings to errors1, which can cause their code to fail to compile any time a new warning is introduced. To mitigate this, we recommend that projects do one of:
This specific caseThe change introduced in Rust 1.90 causes a new warning to be reported in certain cases. We do not believe that this specific warning will affect many projects, and have no plans to issue a patch release. This warning may receive refinement in future releases of Rust as part of normal development activity. Going forward
I am curious what tools and techniques you are using to gauge impact in order to make the joint decisions of holding back a specific version and then later reinstating it. Rust uses tools like crater to get an estimate2 of the fallout from given changes. We are always on the hunt for further high-signal information we could incorporate into our decisions to make changes; perhaps we could collaborate on sharing what information GitHub Actions has! Footnotes |
|
Hey @shepmaster! Thank you for such an informative comment. ❤️ I feel compelled to provide a bit more information.
UPD: I should note that the full set of circumstances that influenced the decision to temporarily lock the version is slightly larger than presented, but the remaining circumstances have little direct interplay with Rust users or maintainers, and therefore I omit them. |
|
I think it should be noted that #13041 is probably not an instance of rust-lang/rust#145936 The rust issue there is a case where it's technically correct to lint, but it's not useful to emit the lint, as there's nothing meaningful to do but ignore the lint in that case. However, intentionally creating code that meets this scenario is very rare and needs careful consideration anyway. The runner-images issue is almost certainly a case of "the lint correctly emitted a warning on a struct that could be constructed, but was not" (however it's not possible to know without a few more lines of source code and context) |
|
Hi! Pietro from the Rust infra and release teams here. Unfortunately this will cause a noticeable increase in traffic on our infrastructure from workflows downloading the latest stable. Last time we did manual analysis on the sources of traffic in our CDNs, GitHub Actions alone contributed to half of our bandwidth costs, and caching the stable image in your images reduced the burden on our infrastructure. We can see in our graphs traffic spikes whenever a new release is published that disappear once the GitHub Actions image is updated. The more the latest stable is held off, the more wasted bandwidth this will generate.
This is not strictly true. Folks who want to use the latest stable usually do |
|
Hey @pietroalbini! 👋
Now that's a good reason to reconsider. 🤔 How significantly has the workload increased already? Is there a risk of problems with the distribution system if the number of requests continues to grow? |
|
@erik-bershel we are currently seeing an increase of 10% to 20% more traffic compared to equivalent days before the release happened. Unfortunately we don't have a good accounting right now of whether all of that increase is coming from GitHub Actions, as we never had any reason to integrate traffic sources into our monitoring. Still, the past few times we manually checked how much traffic came from GHA IPs we found that it was 50% of the total traffic, so a significant chunk of this 10% to 20% increase is GHA workflows downloading the stable release. While there isn't a risk of our infrastructure going down (our traffic flows through either CloudFront or Fastly), having a 10/20% increase for more than a few weekdays is going to eat into our sponsored budget faster than our forecasts, and is going to require more work on our end to keep the funding flowing. We'd really appreciate not wasting the sponsored budget we managed to obtain, especially given how hard it is for open source projects to secure committed and reliable funding. |
|
Is the goal of GitHub Actions to make workflows run "green", or to make them run according to the will of the programmer? These are not the same thing. |
|
Hey @pietroalbini! Considering everything discussed so far, the importance of regular updates on our side for your project maintainability, and @workingjubilee very comprehensive comment: rust-lang/rust#145936 (comment), I find the decision to unblock updates on our side acceptable. I'll make the necessary changes by the end of this week, and the images will likely be rolled out next week as usual (except for macOS 15 Intel - it has its own challenges at the moment). |
|
Thank you so much! |
|
Hey @workingjubilee! 👋 Thank you for your absolutely comprehensive comment under the original issue.
This is such a philosophical question that I find it difficult to answer. Fortunately, I can't—I'm not an official representative of GitHub Actions. 🤷 I can only say that the goal is certainly much more complex, even for my small project of building base images. They need to be secure and stable, meet the needs of the majority, be as modern as possible, and at the same time be as backwards-compatible as possible. All of this also needs to be updated regularly and quickly, while still remaining within reasonable limits in terms of disk space and other resource usage, so as not to cost as much as a Boeing wing. Somewhere in between, we navigate. The results vary, but we're not giving up. 😄 |
|
I see! For what it's worth, I completely understand that you in effect will have some people expecting one and others expecting the contrary, which will create some strange events sometimes. Thank you for the efforts with the compass and sextant. |
|
Note that this change is a regression for CI scripts that currently invoke https://github.com/Traverse-Research/amd-ext-d3d-rs/actions/runs/18037606743/job/51328138001 Footnotes
|
…ctions#13044)" (actions#13076) This reverts commit 98a1416.
FWIW, this change broke all my builds and I cannot get a release out. Turns out my jobs were relying on |
|
Please would someone with the right privileges edit this MR description to remove the link to rust-lang/rust#145936. As noted in rust-lang/rust#145936 (comment), that Rust ticket is about something very specific. It seems highly unlikely to me that the (I wasn't able to find the actual source code mentioned in the ticket, even with a github code search.) |
Description
Initially reported issue highlights upcoming breaking change. Let's pin current version until some information on that behaviour provided in the developers repo: rust-lang/rust#145936
Related issue:
#13041
Check list