.github/workflows: Refactor CI jobs #3090
Conversation
thomaseizinger
force-pushed
the
2951-ci-matrix
branch
from
c191510
to
dbfad1e
Compare
This includes warnings from the latest stable clippy but also for the next beta.
To reuse this, we rename it to what it actually does: Gathering all crates that are published to crates.io.
Overall, we want to: - Run as few jobs as possible - Have the jobs finish as fast as possible - Have the jobs redo as little work as possible There are only so many jobs that GitHub Actions will run in parallel. Thus, it makes sense to not create massive matrices but instead group things together meaningfully. The new `test` job will: - Run once for each crate - Ensure that the crate compiles on its specified MSRV - Ensure that the tests pass - Ensure that there are no semver violations This is an improvement to before because we are running all of these in parallel which speeds up execution and highlights more errors at once. Previously, tests run later in the pipeline would not get run at all until you make sure the "first" one passes. The new `cross` job supersedes the existing `wasm` job. This is an improvement because we now also compile the crate for windows and MacOS. Something that wasn't checked before. `libp2p` exposes a fair few feature-flags. Some of the combinations are worth checking independently. For the moment, this concerns only the executor related transports together with the executor flags but this list can easily be extended.
thomaseizinger
force-pushed
the
2951-ci-matrix
branch
from
253ddad
to
569cbf6
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
thomaseizinger
force-pushed
the
2951-ci-matrix
branch
from
c1e5e4c
to
5ef7549
Compare
@thomaseizinger You can propose the changes to branch protection rules for this repo here: https://github.com/libp2p/github-mgmt/blob/6ca111c9ce1cdb0ffd6a5c09054e4a5d1ed67067/github/libp2p.yml#L3965 |
| test "$ALL_FEATURES" = "$FULL_FEATURE" | ||
|
|
||
| gather_published_crates: |
There was a problem hiding this comment.
Since test depends on it, my suggestion would be to place this job above it. Not a requirement but it'd make it easier to read.
There was a problem hiding this comment.
I usually follow a different philosophy:
The most important functions jobs should be at the top. Assuming sufficiently descriptive names, you shouldn't need the implementation of a function job to continue reading and understanding what is happening.
If you do the reverse - putting all "leaf"1 jobs at the top - you end up reading a lot of unrelated snippets before getting to the parts that actually matter.
Footnotes
-
As in, the leafs in a dependency-tree. ↩
Awesome, I had no idea this was all represented as code! Loving it! I'll send a patch! |
Companion PR for libp2p/rust-libp2p#3090.
Companion PR for libp2p/rust-libp2p#3090.
Done here: libp2p/github-mgmt#73 |
This comment was marked as resolved.
This comment was marked as resolved.
|
With this setup, our pretty much all jobs within the CI workflow finish around the 5min mark: https://github.com/libp2p/rust-libp2p/actions/runs/3467657544/usage Once we merge #3023, I expect this number to go down even more. Many of our crates spend ~1-2minutes compiling the entire workspace because they depend on |
There was a problem hiding this comment.
Looks good to me. Thanks for putting so much time into making our life easier!
If I understand the changes to the rust-version field correctly, they are just updating the field to the reality, i.e. we depended on these versions before but didn't make it explicit. Thus this is not a breaking change.
@thomaseizinger what do you think of mentioning this in the root CHANGELOG.md anyways? Just so that there is no surprise for people.
Correct. From here on, we can track that more accurately now!
I think it makes sense to mention this in the changelog entry of each crate. I'll push a patch for that tomorrow (unless someone beats me to it). |
|
This pull request has merge conflicts. Could you please resolve them @thomaseizinger? 🙏 |
|
I am tagging this as ready to merge. It won't automatically merge until we fix #3131 and merge libp2p/github-mgmt#73.
|
|
This pull request has merge conflicts. Could you please resolve them @thomaseizinger? 🙏 |
|
The WebRTC job is failing because #3141. I'd appreciate if we can merge libp2p/github-mgmt#73 so this can finally go in! |
|
Thanks for merging this! Very exciting! |
|
So, we are blowing the total cache limit with this setup which means we effectively don't have any caching ... |
|
With many pull requests tested using this setup: The per-crate test run is very helpful! |
Good to hear! Just need to get the caching working now. I am considering to make a "cache factory" workflow that only runs on master and make the PR workflows only restore but never save the cache. |
We refactor our continuous integration workflow with the following goals in mind: - Run as few jobs as possible - Have the jobs finish as fast as possible - Have the jobs redo as little work as possible There are only so many jobs that GitHub Actions will run in parallel. Thus, it makes sense to not create massive matrices but instead group things together meaningfully. The new `test` job will: - Run once for each crate - Ensure that the crate compiles on its specified MSRV - Ensure that the tests pass - Ensure that there are no semver violations This is an improvement to before because we are running all of these in parallel which speeds up execution and highlights more errors at once. Previously, tests run later in the pipeline would not get run at all until you make sure the "first" one passes. We also previously did not verify the MSRV of each crate, making the setting in the `Cargo.toml` rather pointless. The new `cross` job supersedes the existing `wasm` job. This is an improvement because we now also compile the crate for windows and MacOS. Something that wasn't checked before. We assume that checking MSRV and the tests under Linux is good enough. Hence, this job only checks for compile-errors. The new `feature_matrix` ensures we compile correctly with certain feature combinations. `libp2p` exposes a fair few feature-flags. Some of the combinations are worth checking independently. For the moment, this concerns only the executor related transports together with the executor flags but this list can easily be extended. The new `clippy` job runs for `stable` and `beta` rust. Clippy gets continuously extended with new lints. Up until now, we would only learn about those as soon as a new version of Rust is released and CI would run the new lints. This leads to unrelated failures in CI. Running clippy on with `beta` Rust gives us a heads-up of 6 weeks before these lints land on stable. Fixes libp2p#2951.
Description
We refactor our continuous integration workflow with the following goals in mind:
There are only so many jobs that GitHub Actions will run in parallel.
Thus, it makes sense to not create massive matrices but instead group
things together meaningfully.
The new
testjob will:This is an improvement to before because we are running all of these
in parallel which speeds up execution and highlights more errors at
once. Previously, tests run later in the pipeline would not get run
at all until you make sure the "first" one passes.
We also previously did not verify the MSRV of each crate, making the
setting in the
Cargo.tomlrather pointless.The new
crossjob supersedes the existingwasmjob.This is an improvement because we now also compile the crate for
windows and MacOS. Something that wasn't checked before.
We assume that checking MSRV and the tests under Linux is good enough.
Hence, this job only checks for compile-errors.
The new
feature_matrixensures we compile correctly with certain feature combinations.libp2pexposes a fair few feature-flags. Some of the combinationsare worth checking independently. For the moment, this concerns only
the executor related transports together with the executor flags but
this list can easily be extended.
The new
clippyjob runs forstableandbetarust.Clippy gets continuously extended with new lints. Up until now, we would only
learn about those as soon as a new version of Rust is released and CI would
run the new lints. This leads to unrelated failures in CI. Running clippy on with
betaRust gives us a heads-up of 6 weeks before these lints land on stable.
Fixes #2951.
Links to any relevant issues
Depends-On: #3131
Open Questions
We could merge
crossandfeature_matrixtogether by adding another matrix variable that is either set to--all-featuresor--features="mdns ...". Should we?Change checklist