CI updates #83
CI updates #83
Conversation
str4d
force-pushed
the
ci-updates
branch
2 times, most recently
from
ce57891 to
0acc77c
Compare
str4d
force-pushed
the
ci-updates
branch
2 times, most recently
from
6a79fa4 to
6361e1b
Compare
| name: Clippy (beta) | ||
| runs-on: ubuntu-latest | ||
| continue-on-error: true |
There was a problem hiding this comment.
I'm adding this like I do in other repos I maintain, because it's useful for checking whether there are Clippy lint changes coming down the pipeline that a future MSRV bump may encounter, without the noise of checking lints against nightly.
However, until #58 is closed, this step will silently fail, and the Clippy (MSRV) step will be checking lints against nightly. So this step won't be useful until then, but it also won't get in the way, so we can add it now in preparation.
There was a problem hiding this comment.
It looks like the auguwu/clippy-action action will add source-code annotations when run. Does this mean we'll get duplicate annotations?
There was a problem hiding this comment.
For actions-rs/clippy-check this wasn't the case with continue-on-error: true AFAIR, but IDK about this action. At least from what I've seen in this PR itself, we weren't getting duplicates.
str4d
force-pushed
the
ci-updates
branch
2 times, most recently
from
558bf73 to
09ef00f
Compare
|
The two remaining CI failures are:
|
.github/workflows/_lints.yml
Outdated
| with: | ||
| token: ${{ secrets.GITHUB_TOKEN }} | ||
| working-directory: ${{ inputs.target }} | ||
| args: --all-features --all-targets -- -D warnings |
There was a problem hiding this comment.
I'm a little wary of "make all warnings errors" in CI tests. This means that any new warnings introduced in newer versions of Rust/Clippy will cause the CI pipeline to fail. While Rust/Clippy are pretty good about not having false positives in warnings, it still can be particularly annoying if your PR fails CI checks because of some existing code triggered the new lints.
There was a problem hiding this comment.
This check is specifically against MSRV as pinned in rust-toolchain.toml. So the only time new warnings would appear is if your PR is changing that pinned toolchain, and then it's not unrelated to your PR. Once we get onto stable Rust, this will only occur in PRs that bump MSRV.
There was a problem hiding this comment.
And per above, this isn't actually working yet due to upstream bugs. We can revisit this once they are actually enabled, and see if they are overly noisy or not.
| name: Clippy (beta) | ||
| runs-on: ubuntu-latest | ||
| continue-on-error: true |
There was a problem hiding this comment.
It looks like the auguwu/clippy-action action will add source-code annotations when run. Does this mean we'll get duplicate annotations?
We now ensure that the `clippy` and `rustfmt` components are installed for the `crates` MSRV via the `rust-toolchain.toml` file.
Includes an update to the `rustix` crate which works around this ICE: rust-lang/rust#111462
Required to work around rust-lang/cargo#10368.
We instead can rely on `crates/rust-toolchain.toml` to pin the nightly version we are using. This has two benefits: - We are not exposed to random nightly breakage in CI. - Once we move to stable Rust (#58), this will ensure we build and test against MSRV.
Also pins `actions/checkout` to a specific revision, so that all of our CI action dependencies are pinned.
|
Okay, I've addressed all comments, and the only failing lint is missing documentation that will be added by #81. |
|
Could this PR be merged at its current state so that I can actualize #81 with the required fixes? |
|
Yes, I think so. |
No description provided.