Implement release process for rustup #84
Conversation
|
The following features still need to be implemented:
|
There was a problem hiding this comment.
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 for promote-release is not needed for rustup, doesn't map 1:1 to its release process (e.g. channels), and passing in the version dynamically as an argument is difficult. 🙁
src/rustup.rs
Outdated
| /// [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.
This feels really weird, since the current environments for rustup are dev-static and prod.
There was a problem hiding this comment.
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.
Without channels, how would you distinguish between beta and stable releases? Only be setting the respective environment variables (e.g. UPLOAD_BUCKET)?
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.
This doesn't work, since self.current_version is None when this runs. Apparently, the version is set much later in the normal release process.
The easiest workaround might be setting a new PROMOTE_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.
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.
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 through https://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.
An early step in our release process is a version bump in Cargo.toml(e.g. rust-lang/rustup@cfca13c). Is that useful for this particular purpose?
There was a problem hiding this comment.
The version is now read from the Cargo.toml in the latest commit on the stable branch.
There was a problem hiding this comment.
@djc Now that we're at it, maybe it makes sense to use workspace-wide version numbers? I don't see how rustup-download can be at a different version from rustup itself.
There was a problem hiding this comment.
@jdno FYI: rust-lang/rustup#4041 rust-lang/rustup#4061 now uses a workspace-wide version number.
|
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.
This is not great, I'd prefer if we tweak rustup's CI to upload them to a location like s3://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.
|
@Mark-Simulacrum @pietroalbini Thanks for the feedback! I've drafted a new release process with the suggested changes in rust-lang/rustup#3844. If that makes sense I'm gonna implement the changes here. |
|
I've made rust-lang/rustup#3932 so that we can share our notes on how to improve Rustup's release process. Currently |
New versions of
rustupare currently released manually by running thesync-dist.pyscript in the rust-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.In rust-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.