fix: Correct dependencies and build checks #484
Conversation
Fixes some CI commands to check all code by enabling all features and checking more parts of the code. As a result fixes the dependencies of the uds-gateway feature and switches it from the deprecated tempdir to tempfile. Makes the version spec of tempfile consistent between all the workspace crates. Fixes a clippy warning about cloning a bool, which is Copy. Finally a few extra changes in ci.yml which the linter was complaining about.
.github/workflows/ci.yml
Outdated
| with: | ||
| command: test | ||
| args: --all -j 4 | ||
| args: -j 4 --workspace --all-features --examples --benches --bins |
There was a problem hiding this comment.
is -j 4 really needed though? I think cargo automatically figures out the number of cpu's and uses that. Or is the intention to use more parallelism then there are CPUs?
There was a problem hiding this comment.
Heh, welcome to the world of CI runners :) If you want to help things along it might make sense to drop down to -j 2 for a moment.
The reasoning is that most of these (esp. GHA runners) OOM very easily if you hit the tests/builds hard. Hence manually limiting to 4 and now potentially even lower for windows builds.
This should all go away once we move to our infra.
|
Very sad CI 😭 |
Much more sad than I was expecting! |
|
need to double-check the tempdir change. i believe the previous one might have been a named temporary directory and the current one might not be |
|
More sad CI since rebase :/ |
another victim of not testing with |
had another look and i think they are the same so this is good. |
|
@Arqu PTAL, I believe this is should now all work and is complete. |
| @@ -135,7 +137,7 @@ jobs: | |||
| uses: actions-rs/cargo@v1 | |||
| with: | |||
| command: check | |||
| args: --all --bins --tests --examples | |||
| args: --workspace --all-features --bins --tests --examples --benches | |||
There was a problem hiding this comment.
why having --all-features is good to test against, we also need to have tests (or at least build checks) for each individual feature, to make sure they work by themselves and with no features enabled at all
There was a problem hiding this comment.
Yes, that's true. I was trying to take small steps.
Given how difficult it was to get this to be happy in CI I'd prefer to do that as a followup since it already fixes some things on main and would stop regressions to that instead of having to keep playing catchup with main while working on this - as already happened.
There was a problem hiding this comment.
this was not meant to add more work to this pr, just a reminder for all of us that it needs more work 😅
There was a problem hiding this comment.
Made an issue to not forget: https://github.com/n0-computer/iroh/issues/496
Fixes some CI commands to check all code by enabling all features and
checking more parts of the code.
As a result fixes the dependencies of the uds-gateway feature and
switches it from the deprecated tempdir to tempfile.
Makes the version spec of tempfile consistent between all the
workspace crates.
Fixes a clippy warning about cloning a bool, which is Copy.
Finally a few extra changes in ci.yml which the linter was complaining
about.