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:
-
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.
-
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.
-
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!
If the
RUSTFLAGS="-Dwarnings"env var value is set,cargo-semver-checkswill 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-checksproduces the following error log: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:
Print out a warning message if
RUSTFLAGSis set to a value that contains-Dwarningsor-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
rustdocgeneration failure like in the case above.Attempt to parse
RUSTFLAGSand remove-Dwarningsin 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
-Dwarningsbecause 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.Change the default behavior to unset
RUSTFLAGS/ remove-DwarningsfromRUSTFLAGS, but offer a flag to switch that behavior off and leaveRUSTFLAGSunchanged.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-checksitself 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-checksitself. Would love to hear your thoughts!