Failure to generate rustdoc when RUSTFLAGS env var sets -Dwarnings

[obi1kenobi]

If the RUSTFLAGS="-Dwarnings" env var value is set, cargo-semver-checks will fail to generate rustdoc on crates that have warnings.

Encountered in:
libp2p/rust-libp2p#4932 (comment)
https://github.com/libp2p/rust-libp2p/actions/runs/7003957574/job/19050749994

cargo-semver-checks produces the following error log:

Error: running cargo-doc failed on /home/runner/work/rust-libp2p/rust-libp2p/semver-checks/target/semver-checks/local-libp2p-0_53_1/Cargo.toml:
    Checking cfg-if v1.0.0
   Compiling libc v0.2.150
< ... snip ... >
    Checking libp2p-dns v0.41.1 (/home/runner/work/rust-libp2p/rust-libp2p/transports/dns)
    Checking libp2p-yamux v0.45.0 (/home/runner/work/rust-libp2p/rust-libp2p/muxers/yamux)
error: use of deprecated unit variant `yamux::WindowUpdateMode::OnReceive`: Use `WindowUpdateMode::OnRead` instead.
   --> /home/runner/work/rust-libp2p/rust-libp2p/muxers/yamux/src/lib.rs:[235](https://github.com/libp2p/rust-libp2p/actions/runs/7003957574/job/19050749994#step:4:236):51
    |
235 |         WindowUpdateMode(yamux::WindowUpdateMode::OnReceive)
    |                                                   ^^^^^^^^^
    |
    = note: `-D deprecated` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(deprecated)]`

    Checking libp2p-autonat v0.12.0 (/home/runner/work/rust-libp2p/rust-libp2p/protocols/autonat)
    Checking libp2p-tcp v0.41.0 (/home/runner/work/rust-libp2p/rust-libp2p/transports/tcp)
error: could not compile `libp2p-yamux` (lib) due to previous error

We can't simply unset RUSTFLAGS, because it may contain settings other than -Dwarnings, for example --cfg <something>, that are relevant and required. At the same time, many users deny warnings in CI, so the current behavior offers poor UX by default for those users. We want to offer "pit of success" UX, and here we fall short.

Options:

1. Print out a warning message if RUSTFLAGS is set to a value that contains -Dwarnings or -D warnings.
   a. This warning can urge users not to set that value, but can be missed when it gets buried behind a long error message from a rustdoc generation failure like in the case above.

2. Attempt to parse RUSTFLAGS and remove -Dwarnings in its many forms.
   a. Can we really be sure that we're doing this safely and in a way that won't ever go wrong?
   b. If we fail to remove -Dwarnings because it was specified in some way we didn't catch, this can be even more confusing. I generally don't love "magic" like this because it has a tendency to go wrong and cause more issues.

3. Change the default behavior to unset RUSTFLAGS / remove -Dwarnings from RUSTFLAGS, but offer a flag to switch that behavior off and leave RUSTFLAGS unchanged.
   a. This is more complex to implement, but also seems like it would suit more users' needs both by default and in general.
   b. Should we do this inside cargo-semver-checks itself or just inside our prebuilt GitHub Action?

I'm currently inclined toward 3. and toward implementing it just inside our action, not inside cargo-semver-checks itself. Would love to hear your thoughts!

[Nemo157]

You can also just add --cap-lints=warn (or allow if they're not being displayed anywhere).

[obi1kenobi]

We actually already run with --cap-lints allow in RUSTDOCFLAGS when invoking cargo doc to generate rustdoc JSON:

cargo-semver-checks/src/rustdoc_cmd.rs

Line 99 in e21509e

"-Z unstable-options --document-private-items --document-hidden-items --output-format=json --cap-lints allow",

IIRC we added it to resolve a previous issue where I believe lints in dependencies could cause a problem.

In this case, when RUSTFLAGS=-Dwarnings, it seems like we end up setting conflicting settings for cargo doc and it chooses that denying warnings should dominate capping lints to "allow." I'm not sure if that's a bug in cargo doc, intended behavior, or something else entirely.

If you're interested in sorting out this issue, I'd love the help!

[Nemo157]

Ah, in this case you need to append --cap-lints=allow to RUSTFLAGS as well, because it's failing when compiling the dependencies with rustc.

[obi1kenobi]

Here's what I'm uncertain about. Should we:

• just set RUSTFLAGS="--cap-lints=allow" and override what the user set RUSTFLAGS to themselves
• append --cap-lints=allow to the user's set RUSTFLAGS value, ignoring the case where they already set --cap-lints
• somehow parse RUSTFLAGS to determine if they set --cap-lints, and then append if not, or do something appropriate (warn, error, etc.) otherwise.

Presumably there are RUSTFLAGS settings we should preserve, or else the tool might not work correctly?

I genuinely don't know enough about all the things RUSTFLAGS could be used for to know which (if any) of those choices above is the right one.

[obi1kenobi]

I'm going with appending --cap-lints=allow to RUSTFLAGS and otherwise leaving it be.