Check rustdocs in CI #11545
Check rustdocs in CI #11545
Conversation
Setting `RUSTDOCFLAGS='-D warnings'` is needed to fail on warnings. For `cargo test --doc` no equivalent option seems to exist. See rust-lang/cargo#14802.
| - uses: dtolnay/rust-toolchain@stable | ||
| - name: cargo doc | ||
| run: | | ||
| RUSTDOCFLAGS='-D warnings' cargo doc --workspace --all |
There was a problem hiding this comment.
These tests are already run as part of "cargo t".
If we want to deny warnings, ideally we'd add that over there.
So we have a couple places where we currently allow warnings
- rustc warnings
- rustdoc warnings
- clippy warnings
As or recently, we deny sphinx warnings; it's definitely realistic
to keep our RST code warning-free across a range of sphinx versions.
(some developers might run something that's more recent than GHA so
that works well).
As for Rust warnings, we should understand why they are disabled,
and maybe take a look at what other projects do etc.
We want to support (on a best-effort basis) building fish with
everything between Rust 1.70 (our current MSRV, for systems like
Debian) and nightly (for address-sanitizer builds and similar). Though
I guess we could relax the nightly requirement and configure only
the ASan build to allow warnings.
The clippy check above has a comment explaining why warnings are currently allowed.
I think we should consider revisiting this decision and make the CI fail on any warnings.
We want to fix all warnings anyway, so better invite the person
who introduced the warning to do it.
The downside is, as mentioned above that
we need to fix rustc warnings on every new rustc release (6 months).
Again, we want to do that regardless so I don't think this is super relevant.
The other downside is that it will basically force developers
to switch to our MSRV to fully test their changes.
Not sure how important that is.
clippy has various warning categories. Maybe we should only fail on the ones that are considered stable.
Currently we are running "cargo clippy" manually. There are a
bunch of other checks that are done in CI but not by ninja -Cbuild fish_run_tests.
If we had a script or test target that runs all of
them (including doc tests), then we'd definitely have noticed the
doctest problems by now, and perhaps have fewer "fix formatting"
commits. I guess we can call it ninja -Cbuild lint.
Note that there is build_tools/style.fish although I'm not necessarily saying we should reuse that.
There was a problem hiding this comment.
These tests are already run as part of "cargo t".
I don't think so, see danielrainer@ef3fae6.
The first doc-test there doesn't run at all because it's in a binary (unrelated to our CI, see rust-lang/rust#50784).
But the one in src/common.rs passes our CI, even though it obviously shouldn't. Running cargo test --doc --workspace results in the expected failure.
It seems that there is no straightforward way of running all rust checks in a single command. rust-lang/cargo#6669
There was a problem hiding this comment.
Regarding warnings in general, I don't know what would work best for fish. The potential CI failures every 6 weeks are definitely annoying, but as you say, we do want such warnings fixed as soon as possible.
Having a script which runs locally (alongside other tests, so people actually run it) would help with detecting warnings and resolved some of the discrepancy between local tests and CI. (The local Rust version can of course still be different from the one in CI.)
I'd like to have a single script which runs all the rust tests and the tests in tests/{checks,pexpects}, so running all checks locally takes just a single command, even for people (like me) who don't use the CMake build tooling. That would be one more step towards getting rid of CMake.
The build_tools/style.fish script could be part of these checks, although I'm not sure if we want it to change files by default.
There was a problem hiding this comment.
Ah I was running cargo t which does run doctests; cargo t --all-targets doesn't and it's probably a good idea to use that..
Setting
RUSTDOCFLAGS='-D warnings'is needed to fail on warnings. Forcargo test --docno equivalent option seems to exist. See rust-lang/cargo#14802.