- Notifications
You must be signed in to change notification settings - Fork14
Implement release process for rustup#84
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
jdno commentedMay 23, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The following features still need to be implemented:
|
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.
I'm not very happy with this implementation. It feels like we're trying to force a square through a round hole. Most of the configuration forpromote-release is not needed forrustup, doesn't map 1:1 to its release process (e.g. channels), and passing in the version dynamically as an argument is difficult. 🙁
| /// [rust-lang/rustup]: https://github.com/rust-lang/rustup | ||
| pub fn promote_rustup(&mut self) -> anyhow::Result<()> { | ||
| println!("Checking channel..."); | ||
| if self.config.channel != Channel::Stable && self.config.channel != Channel::Beta { |
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.
This feels really weird, since the current environments forrustup aredev-static andprod.
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.
Why are we making the channel be the condition here? Promote release already has dev static and static, as two separate environments - should be able to ignore channels I'd expect. (In a manner similar to promote branches ignoring them iirc)
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.
Without channels, how would you distinguish between beta and stable releases? Only be setting the respective environment variables (e.g.UPLOAD_BUCKET)?
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.
I also think we should always have the channel bestable and then do the prod/dev distinction like Rust does.
Without channels, how would you distinguish between beta and stable releases?
dev and prod releases would happen in different CodeBuild pipelines, which have different configured secrets/env vars.
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.
PS: The status quo is that we are callingdev-static ourbeta environment, and probably that's what has caused the confusion (because it's not a proper "channel" like with the releases of Rust itself)? In the end I wouldn't refuse getting something closer to Rust if that helps simplify your workflow.
src/rustup.rs Outdated
| let version = self | ||
| .current_version | ||
| .as_ref() | ||
| .ok_or_else(|| anyhow!("failed to get current version for rustup release"))?; |
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.
This doesn't work, sinceself.current_version isNone when this runs. Apparently, the version is set much later in the normal release process.
The easiest workaround might be setting a newPROMOTE_RELEASE_RUSTUP_VERSION environment variable, but that would need to be updated manually before running the CodeBuild project. That feels risky, since it's very easy to forget this step and right now we would just override existing artifacts. 😬
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.
I think we should figure out how to persist the intended version with the artifacts. That should be possible similarly to how rust has a version containing file checked in, and we can upload that directly to the S3 bucket in rustup's existing "CI".
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.
I agree with Mark that the best approach here is for rustup to store the version number in a file in the repository, so that they can update it on their own.
In general, regarding arguments, I don't see any problem with sticking with environment variables. We never invoke the CodeBuild job directly, we always go throughhttps://github.com/rust-lang/simpleinfra/blob/master/release-scripts/promote-release.py, which has a CLI parser and then sets the environment variables.
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.
An early step in our release process is a version bump inCargo.toml(e.g.rust-lang/rustup@cfca13c). Is that useful for this particular purpose?
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.
The version is now read from theCargo.toml in the latest commit on thestable branch.
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.
@djc Now that we're at it, maybe it makes sense to use workspace-wide version numbers? I don't see howrustup-download can be at a different version fromrustup itself.
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.
@jdno FYI:rust-lang/rustup#4041rust-lang/rustup#4061 now uses a workspace-wide version number.
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.
Really sorry for the back and forth, but sincerust-lang/rustup#4277 rustup is now a single crate, so the workspace-wide version number no longer applies.
One approach might be to refactor the interface for this tool and turn it into a CLI with clap. That way, each command can have its own configuration and parameters. We can still get the arguments from the environment, but it would be much clearer what configuration is needed by each command. |
I'm not sure CLI arguments really make any strong difference there? Whether it's environment variables or flags shouldn't really matter - we can add a common prefix if we want, but the two feel very similar to me. (Modulo --help but that is useful more to humans and it's not intended that humans are running this stuff). |
src/rustup.rs Outdated
| /// `rustup` uses different branches to manage releases. Whenever a commit is pushed to the | ||
| /// `stable` branch in [rust-lang/rustup], GitHub Actions workflows build release artifacts and | ||
| /// copy them into `s3://dev-static-rust-lang-org/rustup/dist/`. |
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.
This is not great, I'd prefer if we tweak rustup's CI to upload them to a location likes3://rustup-artifacts/${commit}, check what is the latest commit hash on the stable branch, and download from that location. Every build overriding the previous build feels iffy.
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.
@Mark-Simulacrum@pietroalbini Thanks for the feedback! I've drafted a new release process with the suggested changes inrust-lang/rustup#3844. If that makes sense I'm gonna implement the changes here. |
I've maderust-lang/rustup#3932 so that we can share our notes on how to improve Rustup's release process. Currently |
rami3l commentedDec 9, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hey there! FYI on our side we haverust-lang/rustup#4105 open, which hopefully will be ready by this weekend (there are some upstream changes waiting to be synchronized over), so we might be able to ship a beta very soon. I wonder if it would be an interesting occasion for us to test out the new solution. OTOH we are not in a hurry or anything; if it's not ready yet, just take your time! ❤️ |
Rustup no longer installs missing toolchains automatically.
The previous, outdated version of MinIO did not work correctly with thelatest AWS CLI, resulting in an `invalid argument (invalid/missingchecksum)` error. This issue has been fixed in MinIO a while ago(minio/minio#19680).
New binaries of `rustup` are now uploaded to the `rustup-builds` bucketon S3 and no longer to `dev-static`, which means that they cannot betested without performing a release to the `beta` channel first. Thismust include promoting the release and copying the binaries into the`dev-static` bucket.
The override feature for the rustup version has been removed, since theversion seems to be hardcoded in the binary as well. So it was possibleto change the path where the artifacts were stored in S3, but using theversion to verify that rustup got updated wasn't possible.
Comparing the Rustup versions before and after the update does not work,since the version in the GitHub repository is not bumped after arelease. We're going back to the override version so that we can forcerustup to self update, which we can check for without comparing versionnumbers.This reverts commitc9b96fe.
After publishing a new rustup release, we are now invalidating bothCloudFront and Fastly. This fixes [rust-lang/simpleinfra#415].[rust-lang/simpleinfra#415]:rust-lang/simpleinfra#415
@Mark-Simulacrum@pietroalbini@rami3l I think this is in a good state now for an actual review. The last item on my mental todo list is checking that the documentation here and inrust-lang/rustup#3844 actually match the implementation. |
Changes look good to me! Just left a comment on#84 (comment). |
| RUN curl "https://awscli.amazonaws.com/awscli-exe-linux-$(arch).zip" -o "awscliv2.zip"; \ | ||
| unzip awscliv2.zip; \ | ||
| rm awscliv2.zip; \ | ||
| ./aws/install | ||
| # Install rustup while removing the pre-installed stable toolchain. |
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.
Outdated comment now that we are not removing the default toolchain?
@jdno Sorry for the interruption, but I'm wondering what is blocking this PR from being merged since a few months ago. If there is anything I can help, please let me know! 🙏 |
No worries and sorry for the delay on this... There is one more item of development work that I'm tracking, and that is changing the parsing of the My suggestion at this point is that I update the implementation in this PR, and then hand the testing of the implementation over to either you guys or the release team. I think the two of you can iterate more quickly on the final details than I can as a middleman between the two teams... |
rami3l commentedJul 14, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@jdno Sounds good to me! I'm just wondering if there has been any prior art in terms of testing this properly or we kind of have to figure out on the go. I'm asking because I'm actually planning for another release soon™️... If you would like us to do some shadow testing or anything I guess we can arrange that. |
New versions of
rustupare currently released manually by running thesync-dist.pyscript in therust-lang/rustup repository multiple times. This process should be automated so that it reproducible and less error-prone. We also want to add more sanity checks in the future, which are more convenient to implement in Rust rather than in Python.Inrust-lang/rustup#3819, we discussed reimplementing the script as part of
promote-release. This pull request makes a first attempt at migrating the existing functionality into this repository.