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

Automate the release process, produce pre-compiled binaries #239

Merged
merged 3 commits into from
Sep 19, 2023

Conversation

Veetaha
Copy link
Contributor

@Veetaha Veetaha commented Sep 10, 2023

Closes #193

This PR adds two new CI workflows release and release-on-tag.
The release process and artifacts are described in docs/internal/release.md in this PR,
so I won't repeat it here.

You can see how it works in my fork.
Here is an example release worklfow run (link).
Here is an example release-on-tag workflow run (link) (the crates-io job is failing becase I don't have access to publishing marker crates).

Here are the two generated commits (link):
image

Here is the github release that it makes (link):
image

The other change made in this PR is the addition of package.rust_version
property to Cargo.toml of the library crates marker_api, marker_uitest,
marker_utils.

I was thinking to refactor the layout of the crates in the repo to separate
the library crates that have a strict MSRV, and the pre-compiled crates that
depend on the pinned nightly toolchain version (#193 (comment)),
but I decided not to do that for now. It's not a big deal, and I don't want to
extend this PR with even more changes for that.

The scope of this PR doesn't include the code changes in cargo-marker to
make it use the pre-compiled marker_rustc_driver from the GitHub release artifacts,
falling back to cargo install if that doesn't work. That should be implemented in
the scope of #238.


The automation also covers publishing crates to crates.io with cargo-release

As a result of this PR the following changes to marker infrastructure need to be done manually and only once.

The new bash-test CI job needs to be marked as required to pass in master branch protection rules in the repo settings.
This test validates that the edits that the workflow makes to the repo match the expected git diff snapshot.

The set-version.sh script does a bunch of regex replacements with sed in all .md files and in some .rs files to update the places where we mention the bare version numbers. For example the constants of marker API and driver versions in the cargo-marker code, and the references to marker_lints/api/uitest = "x.y.z" all over the documentation.


The new RELEASE_PAT secret needs to be created in the repository. This is required because the default github.token that the workflows get doesn't work for the tag-event-based flow. See the comment here.

To create it follow this link: https://github.com/settings/personal-access-tokens/new.

Make a token with a really long expiration date (unless you are okay with having a shorter expiration duration and rotating the token manually on schedule). For the permissions allow access only to the marker repository and give it "read and write" for the repo contents.

Screenshots


Then add this as as a GitHub Actions secret in the repo settings.

Screenshot


Generate a new crates.io API token by following this link: https://crates.io/settings/tokens.
The token needs permissions to publish-update and optionally to publish-new. Unless you are going to manually reserve the crate names, and not rely on the release CI to publish the first version of the crate you may not give it publish-new permission.

The same comment about the token expiration applies here as well. Unfortunatelly, there isn't an easy way to automate the rotation of the expired tokens, so you either have to do that manually which is secure but annoying, or you can set a really high expiration duration, which is convenient but unsecure.

You can also restrict the token with the crate name pattern like marker_*.

The token then needs to be set as a secret named CRATES_IO_TOKEN in the repository. On my screenshots above I didn't have that secret, and I actually didn't fully test the publishing of the crates to crates.io because I don't have access to marker crates on crates.io, and otherwise I wouldn't like to create junk crates just for testing that will be left on crates.io indefinitely. I expect there may be publishing failures on the first release that uses this automation because I didn't test this entirely, but hopefully, there won't be any, or they will be simple to fix.

@Veetaha Veetaha force-pushed the feat/automate-releases branch 4 times, most recently from 7f150b0 to 56f39c2 Compare September 10, 2023 13:28
marker_api/Cargo.toml Outdated Show resolved Hide resolved
@xFrednet xFrednet self-assigned this Sep 11, 2023
@xFrednet xFrednet added A-infra Area: Infrastructure and CI magic :sparkles: S-waiting-on-review Status: Awaiting review labels Sep 11, 2023
@xFrednet xFrednet added this to the v0.3.0 milestone Sep 11, 2023
Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've skimmed over the documentation and some files. I'll continue the review during this week.

Thank you so much for this PR and also all the documentation you wrote. This must have taken a lot of effort and I highly appreciate it!

CHANGELOG.md Outdated Show resolved Hide resolved
scripts/download/taplo.sh Outdated Show resolved Hide resolved
scripts/update-toolchain.sh Outdated Show resolved Hide resolved
docs/internal/release.md Show resolved Hide resolved
Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was finally able to read through all your changes, and I want to say it one more: Thank you!

It all looks clean and is also well documented, while being a fun read :D This is how reviewing can be fun! I'm excited to test this with the next release :)

scripts/release/set-version.sh Show resolved Hide resolved
scripts/release/set-version.sh Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
scripts/release/qemu-install.sh Outdated Show resolved Hide resolved
@Veetaha Veetaha force-pushed the feat/automate-releases branch 2 times, most recently from 73cdd24 to f0dad5b Compare September 16, 2023 14:45
@Veetaha
Copy link
Contributor Author

Veetaha commented Sep 16, 2023

@xFrednet I addressed all your review comments. However, I mistakenly force-pushed the branch. You may review only the changes starting with the commit "Address xFrednet and self-review".

In these changes I additionally added support for releasing versions with an arbitrary -[a-zA-Z0-9.] suffix. Meaning with this automation it will be possible to release say v1.0.0-rc or v1.0.0-rc.2 pre-release version. Release of such a version will update only the Cargo.toml and driver.rs file versions. All the documentation will still refer to the stable version.

The sections region replace-version stable/unstable specify wether we want to have the latest stable marker version mentioned in the region, or the unstable version.

See an example of a pre-release here. The flow is the same as for a regular release. The changelog file should just contain an x.y.z-name version entry.


I also came up with a better workaround than removing the rust-toolchain.toml file in set-version.sh. We just specify the +stable version explicitly to the cargo invocation.


Another thing is that I added an install.sh script. I think this script actually covers most of the use cases that #238 would otherwise cover.

It means with this script it's possible to install all the components (cargo-marker and marker_rustc_driver) with a single bash command for users, so most of them won't need to install anything from sources on the next release.

@xFrednet
Copy link
Member

You'll get a full review in the coming days. For now, I have to say that I absolutely love the region markers, to replace the Marker version. This will make it so much easier to do releases! It really feels like v0.3 will be the greatest releases to date :D

Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This version looks good to me, I have some final comments, and then we can merge this! 🎉

Would you mind squashing some of your commits? You can just force-push. Github can provide diffs, if the branch was not rebased on another one :)

install.sh Outdated
@@ -0,0 +1,72 @@
#!/usr/bin/env bash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works like a charm ❤️

What are your thoughts on moving this file into the scrips or even scripts/release directory? I see little benefit from having it at the root of the repo, as the master will basically always have the -dev version specified. If it's in the root, I think it's likely that people trying to contribute, will just run it, and then get confused that the curl fails. If people use the script link, it won't make a difference 🤔

Copy link
Contributor Author

@Veetaha Veetaha Sep 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that you mention it, I don't like that this script mentions a -dev version at all, because it simply won't ever work, because we'll never actually make a -dev release.

Maybe the replace-version regions should have three states (basically extended the current two ones):

  • stable (will be set to the latest suffixless version)
  • unstable (will be set to the latest released version, since -dev isn't released, it won't include it, but it will include things like 1.0.0-rc)
  • dev (will be set any latest version, including -dev)

Otherwise, I don't have a strong opinion on this. We can put it under scripts/release, will do. If you change your mind at some point, moving it is not a big deal

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having three states for this would make a lot of sense. Are you okay with adding the third state? It's totally fine, if you want to be done, then we can merge it as is. :)

Thank you for moving the file! :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind, you're already a step a head! That's super nice, thank you!

The following components are considered to be internal and they are excluded from the semver and API/ABI stability guarantees.

- `marker_rustc_driver`
- `marker_adapter`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- `marker_adapter`
- `marker_adapter`
- `marker_error`

README.md Outdated

We provide pre-compiled binaries for the mainstream platforms. See the list of available artifacts in our [Github Releases](https://github.com/rust-marker/marker/releases/latest).

Run the following bash script to install the required rust toolchain dependency on your machine and download the current version of `cargo-marker` CLI, and the internal driver.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can imagine that a lot of people will just read this and then copy-pasta 🍝 the command regardless of the platform. Therefore, I would include the platform note above:

Suggested change
Run the following bash script to install the required rust toolchain dependency on your machine and download the current version of `cargo-marker` CLI, and the internal driver.
On unix based systes, you can run the following bash script to install the required rust toolchain dependency on your machine and download the current version of `cargo-marker` CLI, and the internal driver.

Same goes for docs/book/src/usage/installation.md and probably cargo-marker/README.md. It's annoying to keep them in sync, not sure if there is a simpler solution. Maybe just linking to the book in all files? Even though I like having the information directly available 🤔

Copy link
Contributor Author

@Veetaha Veetaha Sep 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also like having this in the readme, I think it's fine to keep them in sync. I bet there is a tool that allows synchronizing regions of text somewhere out there similar to our custom replace-version, but I hope this problem will be a rare occasion.

Actually, I would really like to have windows support for the install script. I just didn't make that effort due to laziness, powershell is even worse than bash and I ususually never have to write it. I wish we could use a cross-platform shell like nushell, but that one isn't at 1.0 yet, and isn't installed by default on windows and UNIX, unfortunately.

I'll try to add a PowerShell install script impl, but in a separate PR, I'd like to merge this one without that to not make "the ideal" the enemy of "the good". But for now, I'll add that clarification

README.md Outdated

### Installation

#### Download pre-compiled binaries (recommended)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should move these user-facing doc updates into a separate PR, that we merge shortly before the next release. It might not be likely that people find Marker until v0.3.0, but if they do, I don't want their first experience to be a non-existing script 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of commenting out these sections, but that looks quite ugly because you can't nest comments in markdown. Anyway, I'll move them to a separate PR

Copy link
Contributor Author

@Veetaha Veetaha Sep 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, making a PR doesn't make sense right now before this PR is merged, especially because there will be a merge commit created.

You can use "git revert 2629e12" before the release to revert the commit that removes the docs for the pre-compiled binaries

Copy link
Member

@xFrednet xFrednet Sep 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a perfect solution! :D

I'll make myself a note for the next release 👍

docs/internal/release.md Outdated Show resolved Hide resolved
docs/internal/release.md Outdated Show resolved Hide resolved
This PR adds two new CI workflows `release` and `release-on-tag`.
The release process and artifacts are described in `docs/internal/release.md`,
so I won't repeat it here.

The other change made in this PR is the addition of `package.rust_version`
property to `Cargo.toml` of the library crates `marker_api`, `marker_uitest`,
`marker_utils`.

I was thinking to refactor the layout of the crates in the repo to separate
the library crates that have a strict MSRV, and the pre-compiled crates that
depend on the pinned nightly toolchain version (rust-marker#193 (comment)),
but I decided to do that for now. It's not a big deal, and I don't want to
extend this PR with even more changes for that.

The scope of this PR doesn't include the code changes in `cargo-marker` to
make it use the pre-compiled `marker_rustc_driver` from the GitHub release artifacts,
falling back to `cargo install` if that doesn't work. That should be implemented in
the scope of rust-marker#238.
@Veetaha Veetaha force-pushed the feat/automate-releases branch 2 times, most recently from 95063b0 to 0f17cd9 Compare September 19, 2023 00:38
@Veetaha
Copy link
Contributor Author

Veetaha commented Sep 19, 2023

I addressed the review comments, and did some more improvements.

I removed --retry-delay from curl. I just noticed that it disables the exponential backoff retry (StackOverflow), which is unwanted; exponential backoff is better than a fixed one because this way we don't overload github if it is feeling unwell retrying with larger and larger intervals. I'm still in the process of establishing of what I think the "canonical" curl invocation, so you may see curl options vary while I do that.

I also fixed a minor clippy lint in tests because I added --all-targets to cargo clippy on CI it started to detect new lints.

I tested the pre-releases with 0.4.0-rc.123 version and it didn't work because the regex didn't include the .123 part, now it's fixed.

Here are release and a pre-release: link,

Script for installing a release version should just work:

curl --location --silent --fail --show-error --retry 5 --retry-connrefused \
    https://raw.githubusercontent.com/Veetaha/marker/v0.3.0/scripts/release/install.sh | bash

Script for installing a pre-release version should just work as well:

curl --location --silent --fail --show-error --retry 5 --retry-connrefused \
    https://raw.githubusercontent.com/Veetaha/marker/v0.4.0-rc.123/scripts/release/install.sh | bash

Don't forget to add bash-test to the required checks for the PR, and double-check you did all the requirements mentioned after the following sentence in the PR description.

As a result of this PR the following changes to marker infrastructure need to be done manually and only once

… script.

I tested the pre-releases with `1.0.0-rc.1` version and it didn't work because
the regex didn't include the `.1` part, now it's fixed.
@xFrednet
Copy link
Member

Everything looks good to me, thank you so much for all the work you put into this!

Merged via the queue into rust-marker:master with commit d150950 Sep 19, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-infra Area: Infrastructure and CI magic :sparkles: S-waiting-on-review Status: Awaiting review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide pre-compiled binaries on each release
2 participants