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

chore(ci): release-plz: Complete overhaul of the release process #762

Merged
merged 6 commits into from
Oct 17, 2024

Conversation

jbeaurivage
Copy link
Contributor

In the midst of trying to upgrade our automated release workflows, I found release-plz, which is a ready-made solution which essentially take care of everything we're trying to accomplish with our release processes:

  • Changelog management and updates based on commit history
  • Git tagging
  • Automated github releasees
  • Automated version bumps, including bumping dependencies in Cargo.tomls where necessary
  • Publishing to crates.io
  • Out-of-the box support for multi-crate setups
  • Version bumps and changelog updates for each crates are based on the specific commits which affected their files

What has to change?

  • All crates must live in a Cargo workspace
  • There is no longer a need to have separate release processes for Tier 1 and Tier 2 BSPs (or atsamd-hal, atsamd-hal-macros or PACs for that matter)

What should contributors and maintainers expect going forward?

  • Changelog management works much better if all commits on master use conventional commits.
  • Every time a commit is pushed to master, a release PR will be created, or updated if it is already open. Merging that PR automatically triggers a release of all affected crates. The PR branch may also be edited by maintainers if release-plz made a mistake, or manual changes are needed, and those changes will be respected by the tool.

Transitioning

release-plz determines if a release is necessary by looking at the previous published version of a crate on crates.io. Before going live with release-plz, we should definitely manually inspect the first release PR to make sure the repo is at a clean state.

@jbeaurivage
Copy link
Contributor Author

During testing I ran into this issue and proposed this fix which should cover our needs.

@jbeaurivage jbeaurivage force-pushed the release-plz branch 4 times, most recently from d91e98c to 3fe011f Compare October 1, 2024 00:09
@jbeaurivage jbeaurivage marked this pull request as ready for review October 1, 2024 00:10
@jbeaurivage jbeaurivage changed the title release-plz: Complete overhaul of the release process chore(ci): release-plz: Complete overhaul of the release process Oct 1, 2024
@jbeaurivage
Copy link
Contributor Author

This is all good to go now.

I opened a test repo to see how the tool would behave. You can find out what running the tool on this repo in its current state would look like here: https://github.com/jbeaurivage/atsamd-release-test/pull/4.

As previously mentioned, the first release PR will need to be babysat a bit since we never used conventional commits and our releases weren't really standardized, but after that it should be much more smooth sailing.

@ianrrees
Copy link
Contributor

This looks really good to me in principle, but I've not had a chance to play with it yet. Will aim to spend some time on it this weekend.

One very minor comment/question: do the profiles added in the workspace Cargo.toml have the desired effect?

@jbeaurivage
Copy link
Contributor Author

About the workspace profiles: now that we're using a workspace, Cargo was complaining that the profiles specified in some BSPs were ignored in favour of the workspace profiles.

The profiles I added to the workspace Cargo.toml are what seemed to be the least common denominator between the BSPs. The next step would be tree remove the profiles from the BSP Cargo.toml's.

I wonder if some BSPs really have specific profile requirements for the examples to run? What seems more likely to me is that they're there to serve as examples for newcomers.

@sajattack
Copy link
Member

About the workspace profiles: now that we're using a workspace, Cargo was complaining that the profiles specified in some BSPs were ignored in favour of the workspace profiles.

The profiles I added to the workspace Cargo.toml are what seemed to be the least common denominator between the BSPs. The next step would be tree remove the profiles from the BSP Cargo.toml's.

I wonder if some BSPs really have specific profile requirements for the examples to run? What seems more likely to me is that they're there to serve as examples for newcomers.

some of the neopixel examples rely on flakey timing IIRC. Probably part of the original reason for the profiles. But not a very good one.

@bradleyharden
Copy link
Contributor

I think one or two of the BSP had to be built in release mode for some reason, I think because of size requirements. That's definitely my memory, but it doesn't immediately make sense to me as I type it.

@jbeaurivage
Copy link
Contributor Author

jbeaurivage commented Oct 12, 2024

That makes sense, I think in any case if we have profiles specified in the workspace with the right options the builds should still work. Some things I would carry over because I sent think they matter, like codegen-units, unless I'm misunderstanding how it's used.

I think I remember seeing a BSP that was specifying lto = thin in the debug profile . I'm kind of reluctant to carry it over to the workspace, since it makes debugging harder than it needs to be. As a compromise we could make a brand new profile that enables light optimizations to build those examples that are too big to fit in flash unoptimized. Even opt-level = 1 cuts down on size a lot IME.

@ianrrees
Copy link
Contributor

The USB examples only work reliably under release mode, but I think a large part of that is down to the SETUP issue which should be resolved by rust-embedded-community/usb-device#153 .

Neopixel examples are quite problematic, I'm thinking the right answer is to write a SAMD specific driver for them, that uses timer peripherals or maybe DMA-fed SPI to control the timing. Existing neopixel drivers try to use embedded-hal traits but they don't give strong enough guarantees for the neopixel's timing requirements.

@jbeaurivage
Copy link
Contributor Author

Cool. At least it seems like switching to workspace profiles won't bring in any new bugs or anything.

.github/workflows/release-crates.yml Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
@jbeaurivage
Copy link
Contributor Author

Some of the BSPs aren't currently building because they need optimizations enabled to fit in flash. Let me get that sorted today.

Copy link
Member

@sajattack sajattack left a comment

Choose a reason for hiding this comment

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

feel free to merge once you have the build errors fixed, thanks

@jbeaurivage
Copy link
Contributor Author

Sweet, thanks for the review!

@jbeaurivage jbeaurivage merged commit e453894 into atsamd-rs:master Oct 17, 2024
90 of 108 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants