Move cargo hack to it's own workflow and cleanup #794
Conversation
| @@ -7,9 +7,6 @@ on: | |||
| branches: ["master"] | |||
| merge_group: | |||
|
|
|||
| env: | |||
| CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse | |||
There was a problem hiding this comment.
Just curious, do you have documentation you can link about rust-toolchain managing the crates.io lookup? I was under the impression that rust-toolchain would handle the initial toolchain download and install, but this is referring to grabbing the crates later with cargo update. I could easily be wrong, but I couldn't find any relevant rust-toolchain docs in a little bit of googling.
There was a problem hiding this comment.
True the documentation is vague in saying "good defaults" but there are some pr's which show this. dtolnay/rust-toolchain#27 dtolnay/rust-toolchain#66 dtolnay/rust-toolchain#54
There was a problem hiding this comment.
That's good enough for me. BTW, it looks like all of those environmental variables are also set at the top of tools/ci.sh. Do we want to remove them there as well?
There was a problem hiding this comment.
I was thinking about that, I'm not sure if they are helpful when running the script on it's own?
I think it makes sense to remove them, but I don't have too strong of a stance here.
tools/ci.sh
Outdated
| @@ -6,8 +6,6 @@ export CARGO_INCREMENTAL=0 | |||
| export CARGO_REGISTRIES_CRATES_IO_PROTOCOL=sparse | |||
|
|
|||
| cargo build --bin lalrpop --features pico-args | |||
| # build with minimal amount of features to make sure dependencies can build too. | |||
| cargo hack build --workspace --feature-powerset --exclude-features pico-args --optional-deps | |||
There was a problem hiding this comment.
Did you do any measurement on how much time dropping the feature-powerset build saves in CI? My impression is that test is the really expensive bit. I'm inclined to agree with dropping the powerset test. The really important thing is just getting the single binary the tests need, but we do get some value here, since "cargo build" tests a slightly different configuration that "cargo test" does. It's probably not enough value to be worth the extra CI time, but I'm curious if we have any data to compare what it's costing us to leave this in.
(If we were to leave it in, it should maybe be reordered to before the regular build to ensure we run tests with a specified lalrpop binary, rather than whatever hack happened to decide to do last.)
There was a problem hiding this comment.
This seems to hold up in that it was only a 20s saving but probably close enough to some margin of error.
since "cargo build" tests a slightly different configuration that "cargo test" does In what way? Maybe this is what I'm unclear about.
There was a problem hiding this comment.
hmm running with incremental but without cargo hack build has given the best time so far... but I'm not sure what my error margin is when doing this.
There was a problem hiding this comment.
(Inspired by dtolnay/rust-toolchain#61)
There was a problem hiding this comment.
since "cargo build" tests a slightly different configuration that "cargo test" does In what way? Maybe this is what I'm unclear about.
IIUC there are two major differences:
- The final binary isn't linked together. I doubt we care too much about this difference. I'm not sure if there's anything that potentially could go wrong here that wouldn't be a rust bug.
#[cfg(test)]blocks wouldn't be included in build. This could potentially cause issues if a certain block of code was conditionally only needed under certain features, but was also marked#[cfg(test)], I believe.
IMHO, running just the single build without a hack, and then running test on the full hack seems like a sensible compromise between CI usage and test coverage. I don't think we're getting a ton of added value in doing the hack build, and I worry that we could end up causing unneeded CI failures by accidentally using the wrong built version during cargo test.
There was a problem hiding this comment.
Yeah, I think those differences are unlikely to come up in practice. I also am in favor of removing cargo hack build purely on it being simpler for what is basically the same benefit(and then not needing to rely on cargo properly reusing build artifacts).
| @@ -42,3 +39,16 @@ jobs: | |||
| run: cargo fmt --all -- --check | |||
| - name: Run clippy | |||
| run: cargo clippy --all --all-targets -- -D warnings | |||
| feature_powerset: | |||
There was a problem hiding this comment.
Just a thought. Would we want to add "needs: lint" here? It would lengthen the total time spent on PR testing, since this wouldn't start until after linting is done, but it would avoid doing this heavyweight check on a PR that's just failing on format or clippy anyways.
There was a problem hiding this comment.
Hmmm, what do you think compared to https://stackoverflow.com/questions/74690423/how-can-i-use-github-actions-concurrency-to-cancel-other-jobs-if-any-of-them-f ?
There was a problem hiding this comment.
I tend to try to minimize my use of third party actions, since they're running potentially untrusted code in my CI pipeline with access to secrets, but that's above average on the paranoia level. If I were the maintainer, I'd prefer the straight dependency to avoid adding the extra external action, but I won't pitch a fit if you/the maintainers prefer to go the other way.
There was a problem hiding this comment.
Fair, it's a trade off between "average" runtime usage, "worst-case" runtime usage, and additional outside dependencies. I don't have a strong stance here.
There was a problem hiding this comment.
Would we want to add "needs: lint" here?
That sounds good idea.
CI running time for this job is not a blocker of lalrpop development (yet). So blocking dependency seems fine.
|
+1 to the general idea here. Just left a few comments/questions in the PR about some of the specific details. |
Pat-Lafon
force-pushed
the
master
branch
2 times, most recently
from
3bdd063
to
af98db2
Compare
There was a problem hiding this comment.
Thank you!
@dburgener would you add a linter dependency to the new job later?
| @@ -42,3 +39,16 @@ jobs: | |||
| run: cargo fmt --all -- --check | |||
| - name: Run clippy | |||
| run: cargo clippy --all --all-targets -- -D warnings | |||
| feature_powerset: | |||
There was a problem hiding this comment.
Would we want to add "needs: lint" here?
That sounds good idea.
CI running time for this job is not a blocker of lalrpop development (yet). So blocking dependency seems fine.
Sure. I'll send a PR later this week. |
With the addition of
cargo hackinto theci.shscript, the github workflows run has significantly slowed down. This also makes it more inconvenient to add further things to the workflow like #790. I think the workflow should prioritize failing fast and speed.To this end, I've moved the
cargo hack testcall out of theci.shscript into it's own workflow. This invocation is a bit heavy weight to be the first thing a contributor runs and can be better parallelized on it's own. Maybe something like taiki-e/cargo-hack#180.Note I've ignore two features from the
cargo hackinvocation:pico-argsanddefault.pico-argsis only needed for the cli so it's unnecessary to include with the tests.defaultis excluded because it added an extra unnecessary dimension to the features being run(as all default does is enable other features, it is not used in the code itself). Add the--print-command-listflag to your cargo hack call to see all of the commands that will be run and with what features.