Conversation
danobi
force-pushed
the
static_ci
branch
3 times, most recently
from
December 14, 2023 17:40
fd8fd71 to
749a4bd
Compare
danobi
marked this pull request as ready for review
December 14, 2023 17:44
Owner
Author
|
cc @matklad |
matklad
approved these changes
Dec 14, 2023
Contributor
There was a problem hiding this comment.
LGTM, but couple of points about overall CI:
- I usually
cargo test --no-runrather thancargo build, to compile the tests as well - I think actions/checkout is at v3 these days? Not that it matters
- If we want to check static linking, we might build against
-muslrather than-gnu - That's highly controversial opinion, but I personally prefer
cargo fmtto be triggered bycargo test, rather than by my CI, so that I can runcargo testlocal and not worry about some checks which are there on the CI but are missing locally. The same applies to static linking --- if I am worried about being able to statically link, I'd probably write an integration tests for this which shells out to cargo, rather than leaving this logic to a .yml in CI. - I like adding
-Dwarningsto RUSTFLAGS on CI --- I don't mind warnings locally when I develop, but I'd rather CI not allowed me to merge them to master - default caching on CI is pretty bad, https://github.com/Swatinem/rust-cache/actions is a big improvement if you care about CI times.
danobi
force-pushed
the
static_ci
branch
2 times, most recently
from
December 17, 2023 19:04
87d1be1 to
52b296d
Compare
Owner
Author
|
@matklad thanks for the tips! I added most of those suggestions to this pr. I filed a ticket to track the other two cuz I have various thoughts on that as well. Nesting cargo calls for rustfmt is a neat trick -- I'll have to remember that. But for static build, it kinda requires a bit more setup like having |
Higher signal from PR status overview
Before, it was only building non-test code and allowing warnings. Now, we build test code as well as deny warnings.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Various small tweaks. See individual commits for more details.