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

Adding release stage #159

Merged
merged 38 commits into from
Nov 14, 2024
Merged

Adding release stage #159

merged 38 commits into from
Nov 14, 2024

Conversation

ScottGibb
Copy link
Contributor

@ScottGibb ScottGibb commented Nov 8, 2024

Proposal for CI Publishing to crates.io

The following is a very simple solution to ticket #154 . It does not involve any changes to the existing CI infrastructure. It was based off a similar release cycle suggested for the BT-HCI repo/crate (embassy-rs/bt-hci#22)

How it Works

Whenever a new tag is made in the format VX.X.X, the release workflow will activate. It consists of two jobs.

  • build: Which is identical to the tests,yml. Ensuring that every release runs our ci tests
  • release: This job will only run if the build job passes, It will first verify that the Github tag matches the config.toml tag. (Assumptions are made here, that both crates will increment their version at the same time) Only then will it continue on with the build process and then publish both a GitHub Release and crates.io release. All tags should match each other, allowing a single source of truth to be found.

What Other options did I try

  • I tried to get the workflow to depend on the tests.yml workflow but couldn't find a good way of doing it.
  • I did look into automated PR publishing, however launching on tag means we can choose when we deliver a next release in a low effort fashion. It does rely that main is always tested and in a good stage, otherwise the release workflow will fail (on purpose) preventing a release to crates.io

Testing

@ScottGibb
Copy link
Contributor Author

ScottGibb commented Nov 11, 2024

At the moment this Release cycle looks almost identical to the one proposed for embassy-rs/bt-hci#22. However there is a couple of questions that need to be clarified:

  • There are two crates in this repo, are these being published to crates.io as seperate crates?
    - If this is true, how do we handle tagging of this repository? The way this current release flow works, the tagging is a crucial part of ensuring the single source of truth. We could tag the repo twice for each release, but this seems a bit cumborsome
  • Do we also want to run the tests workflow as well, if this is hardware dependent whats the best way of handling that?

On the Crates.io side, according to the rust book we should handle the cargo.toml like so

Multiple locations
It is possible to specify both a registry version and a git or path location. The git or path dependency will be used locally (in which case the version is checked against the local copy), and when published to a registry like crates.io, it will use the registry version. Other combinations are not allowed. Examples:

[dependencies]
# Uses `my-bitflags` when used locally, and uses
# version 1.0 from crates.io when published.
bitflags = { path = "my-bitflags", version = "1.0" }

# Uses the given git repo when used locally, and uses
# version 1.0 from crates.io when published.
smallvec = { git = "https://github.com/servo/rust-smallvec.git", version = "1.0" }

# N.B. that if a version doesn't match, Cargo will fail to compile!

@lulf
Copy link
Member

lulf commented Nov 11, 2024

At the moment this Release cycle looks almost identical to the one proposed for embassy-rs/bt-hci#22. However there is a couple of questions that need to be clarified:

* There are two crates in this repo, are these being published to crates.io as seperate crates?
  - If this is true, how do we handle tagging of this repository? The way this current release flow works, the tagging is a crucial part of ensuring the single source of truth. We could tag the repo twice for each release, but this seems a bit cumborsome

I assume you're referring to trouble-host-macros and trouble-host. Yes, those would both need to be released, but it should be based on the same tag IMHO since we cannot support changes across versions here. Also, it should only release creates that have actually changed, so if nothing changes in trouble-host-macros from 0.1.0 to 0.2.0, it should not be released.

* Do we also want to run the `tests` workflow as well, if this is hardware dependent whats the best way of handling that?

Yes, it should be run. Just running the test workflow should be enough.

On the Crates.io side, according to the rust book we should handle the cargo.toml like so

Multiple locations
It is possible to specify both a registry version and a git or path location. The git or path dependency will be used locally (in which case the version is checked against the local copy), and when published to a registry like crates.io, it will use the registry version. Other combinations are not allowed. Examples:

[dependencies]
# Uses `my-bitflags` when used locally, and uses
# version 1.0 from crates.io when published.
bitflags = { path = "my-bitflags", version = "1.0" }

# Uses the given git repo when used locally, and uses
# version 1.0 from crates.io when published.
smallvec = { git = "https://github.com/servo/rust-smallvec.git", version = "1.0" }

# N.B. that if a version doesn't match, Cargo will fail to compile!

This is for the crate consumer though, not for us?

@ScottGibb
Copy link
Contributor Author

ScottGibb commented Nov 11, 2024

I assume you're referring to trouble-host-macros and trouble-host. Yes, those would both need to be released, but it should be based on the same tag IMHO since we cannot support changes across versions here. Also, it should only release creates that have actually changed, so if nothing changes in trouble-host-macros from 0.1.0 to 0.2.0, it should not be released.

Ahh okay I think that makes sense, so each increment of this project would mean both crates increment at the same rate. So both projects have the same version number regardless of what code was updated?

As for not releasing to crates.io, does that mean that crates.io would only have for example v0.1.0 for trouble-host-macros but trouble-host might be on for example 0.5.0?

Yes, it should be run. Just running the test workflow should be enough.

Awesome! :)

This is for the crate consumer though, not for us?

I assumed it was for the publisher too? I know at the moment I cant run the --dry-run without specifying the trouble-host-macros crate, After specification it complains it doesn't exist which makes sense.

If the version isn't in the toml it spits out an error like so:

trouble/host on  Add-ci-publish-workflow [!] is 📦 v0.1.0 via 🐍 v3.13.0 via 🦀 v1.82.0 on ☁️  (eu-north-1) 
❯ cargo publish --dry-run              
    Updating crates.io index
error: all dependencies must have a version specified when publishing.
dependency `trouble-host-macros` does not specify a version
Note: The published dependency will use the version from crates.io,
the `path` specification will be removed from the dependency declaration.

@ScottGibb
Copy link
Contributor Author

As for running the tests @lulf,

Yes, it should be run. Just running the test workflow should be enough.

Since that runs on every PR, is this what you mean by should be enough?

@petekubiak
Copy link
Collaborator

petekubiak commented Nov 11, 2024

There are two crates in this repo, are these being published to crates.io as seperate crates?
Is there not a way to include both as packages under a single crate? My feeling is that trouble-host-macros doesn't really have meaning without trouble-host, so ideally should be packaged together with it.

I'm thinking a la serde and serde-derive

@lulf
Copy link
Member

lulf commented Nov 12, 2024

I assume you're referring to trouble-host-macros and trouble-host. Yes, those would both need to be released, but it should be based on the same tag IMHO since we cannot support changes across versions here. Also, it should only release creates that have actually changed, so if nothing changes in trouble-host-macros from 0.1.0 to 0.2.0, it should not be released.

Ahh okay I think that makes sense, so each increment of this project would mean both crates increment at the same rate. So both projects have the same version number regardless of what code was updated?

As for not releasing to crates.io, does that mean that crates.io would only have for example v0.1.0 for trouble-host-macros but trouble-host might be on for example 0.5.0?

I thought some more about this, and I think we should tag them independently, it is a lot simpler:

  • trouble-host and trouble-host-macros are versioned independently
  • a git tag 'trouble-host-v0.1.0' is created when releasing trouble-host etc.

Yes, it should be run. Just running the test workflow should be enough.

Awesome! :)

This is for the crate consumer though, not for us?

I assumed it was for the publisher too? I know at the moment I cant run the --dry-run without specifying the trouble-host-macros crate, After specification it complains it doesn't exist which makes sense.

If the version isn't in the toml it spits out an error like so:

trouble/host on  Add-ci-publish-workflow [!] is 📦 v0.1.0 via 🐍 v3.13.0 via 🦀 v1.82.0 on ☁️  (eu-north-1) 
❯ cargo publish --dry-run              
    Updating crates.io index
error: all dependencies must have a version specified when publishing.
dependency `trouble-host-macros` does not specify a version
Note: The published dependency will use the version from crates.io,
the `path` specification will be removed from the dependency declaration.

Ah, I see. Yes, we should add a version there.

@lulf
Copy link
Member

lulf commented Nov 12, 2024

As for running the tests @lulf,

Yes, it should be run. Just running the test workflow should be enough.

Since that runs on every PR, is this what you mean by should be enough?

For PRs, it is invoked by /test at present, and only for people added to the repo. But for GHA on release tags, it's ok to run the test workflow since only repo members can push tags.

@ScottGibb
Copy link
Contributor Author

ScottGibb commented Nov 14, 2024

I believe this is now at a stage for merging the workflow is as follows: @lulf @jamessizeland

  • push a tag on the main branch (all other branch tags will be ignored)
  • the release cycle will trigger
  • the CI Tests will run
  • Depending on the tag pushed (trouble-host-vX.X.X/trouble-host-macros-vX.X.X) different release workflows will be launched and if the git tag matches the version on the corresponding cargo.toml, a github release alongside a cargo publish will be launched, otherwise the workflow will fail. If successful, the workflow will complete and the status will be green.

Current Issues that need to be resolved:

  • On crates.io we need a placeholder for trouble-host-macros, otherwise the workflows fail
  • naming convention for trouble-host-macros to be agreed (ive changed the name of the folder to trouble-host-macros instead of trouble-host-macro).
  • Confirmed workflow for hardware in the loop(HIL) tests. I struggled to get any linkage between the release flow and the HIL tests. At present we can assume that any code on main is HIL tested as all merges into main trigger the PR flow which trigger the HIL tests.

When merging, could we please squash the commits, due to the type of PR this is, it required a lot more commits than necessary

@ScottGibb ScottGibb marked this pull request as ready for review November 14, 2024 11:13
@lulf
Copy link
Member

lulf commented Nov 14, 2024

/test

Copy link
Member

@lulf lulf left a comment

Choose a reason for hiding this comment

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

LGTM

@lulf
Copy link
Member

lulf commented Nov 14, 2024

/test

@lulf lulf merged commit d96ffb4 into embassy-rs:main Nov 14, 2024
2 checks passed
@ScottGibb ScottGibb deleted the Add-ci-publish-workflow branch November 14, 2024 14:41
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.

3 participants