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

Refactor Makefile.toml and use it as the source of truth for (almost) all CI #783

Merged
merged 24 commits into from
Jun 10, 2021

Conversation

Manishearth
Copy link
Member

@Manishearth Manishearth commented Jun 9, 2021

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

@Manishearth Manishearth requested a review from a team as a code owner June 9, 2021 23:33
@Manishearth Manishearth requested review from sffc and echeran June 9, 2021 23:33
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • .github/workflows/build-test.yml is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • .cargo/config.toml is different
  • .github/workflows/build-test.yml is different
  • tools/scripts/wasm.toml is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

.cargo/config.toml Outdated Show resolved Hide resolved
.github/workflows/build-test.yml Show resolved Hide resolved
@Manishearth Manishearth closed this Jun 9, 2021
@Manishearth Manishearth reopened this Jun 9, 2021
@Manishearth
Copy link
Member Author

image

looks better now!

@codecov-commenter
Copy link

codecov-commenter commented Jun 10, 2021

Codecov Report

Merging #783 (612f643) into main (2a47704) will decrease coverage by 0.11%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
experimental/provider_static/src/lib.rs 4.00% <0.00%> (-8.50%) ⬇️
ffi/capi/src/custom_writeable.rs 0.00% <0.00%> (ø)
ffi/capi/src/fixed_decimal.rs 0.00% <0.00%> (ø)
ffi/capi/src/provider.rs 0.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da5bfbf...612f643. Read the comment docs.

@coveralls
Copy link

coveralls commented Jun 10, 2021

Pull Request Test Coverage Report for Build fe550cc9d36fb76896f423396775865f3de03dd1-PR-783

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 76.119%

Totals Coverage Status
Change from base Build da5bfbfdfa8dc7946530ec3ec97b365e1ab627c7: 0.0%
Covered Lines: 9384
Relevant Lines: 12328

💛 - Coveralls

sffc
sffc previously approved these changes Jun 10, 2021
CONTRIBUTING.md Outdated Show resolved Hide resolved
@Manishearth
Copy link
Member Author

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

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • .cargo/config.toml is different
  • Makefile.toml is different
  • tools/scripts/tidy.toml is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • components/icu/src/lib.rs is no longer changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@Manishearth Manishearth requested a review from sffc June 10, 2021 02:40
@Manishearth
Copy link
Member Author

This is ready to land, though before we land this we need to update the required CI runs in the repo settings

@Manishearth
Copy link
Member Author

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.

sffc
sffc previously approved these changes Jun 10, 2021
@@ -4,7 +4,7 @@

# This is a cargo-make file included in the toplevel Makefile.toml

[tasks.tidy]
[tasks.tidy-minus-fmt]
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

eh, fixed it anyway

@sffc
Copy link
Member

sffc commented Jun 10, 2021

When we're ready to merge this, we should change the branch permissions to reflect the new CI.

@Manishearth
Copy link
Member Author

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+

@Manishearth Manishearth merged commit 17385b2 into unicode-org:main Jun 10, 2021
@Manishearth Manishearth deleted the makefile-refactor branch June 10, 2021 14:23
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.

Refactor Makefile.toml into smaller files
5 participants