-
Notifications
You must be signed in to change notification settings - Fork 183
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
Refactor Makefile.toml and use it as the source of truth for (almost) all CI #783
Conversation
39a23ea
to
27ce963
Compare
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
27ce963
to
66fc6ba
Compare
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
Codecov Report
@@ Coverage Diff @@
## main #783 +/- ##
==========================================
- Coverage 76.17% 76.05% -0.12%
==========================================
Files 189 189
Lines 12151 12170 +19
==========================================
Hits 9256 9256
- Misses 2895 2914 +19
Continue to review full report at Codecov.
|
Pull Request Test Coverage Report for Build fe550cc9d36fb76896f423396775865f3de03dd1-PR-783
💛 - Coveralls |
I made it cache cargo-make, but I plan to write a github action that makes that easy to do. The existing install GHA supports caching, but badly (it uses an external cache), and using the builtin cache is blocked on actions-rs/install#5 |
e74cc35
to
cfe6a37
Compare
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
cfe6a37
to
0a5e08e
Compare
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
This is ready to land, though before we land this we need to update the required CI runs in the repo settings |
I tried cleaning up the cargo make installs in #787, but it didn't work. Eventually actions-rs/install will get support for the native cache. |
tools/scripts/tidy.toml
Outdated
@@ -4,7 +4,7 @@ | |||
|
|||
# This is a cargo-make file included in the toplevel Makefile.toml | |||
|
|||
[tasks.tidy] | |||
[tasks.tidy-minus-fmt] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion (optional): Delete the "tidy-minus-fmt" task, move these up into "ci-job-tidy", and make "tidy" invoke fmt + ci-job-tidy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda want the ci-job-foo to be toplevel so you can change CI jobs without changing other stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eh, fixed it anyway
When we're ready to merge this, we should change the branch permissions to reflect the new CI. |
Seems like I have to change the rules before the push, so we have to wait for these checks to pass and I need another r+ |
This refactors Makefile.toml into multiple subfiles. It also adds various
ci-job-foo
make tasks which are directly run by CI, so adding further CI can be done by tweaking Makefile.toml only.This reduces the number of parallel CI jobs. We still have separate jobs for things that are fast-fail: there's a separate check, tidy, fmt, and clippy job, but otherwise there's a "test" (for most tests), "features" (for the "all feature combos" test), and "wasm" jobs. Jobs like "bincode" have been rolled into "test": unless the job has a reason to be long running or require weird dependencies, it should be a part of "test".
Fixes #612