Preserve RUSTDOCFLAGS so that custom --cfg settings can be applie…

…d. (#1016)

Allow users to set `--cfg` options included when scanning a crate by
invoking the tool as:

```
RUSTDOCFLAGS="--cfg your-cfg-value" cargo semver-checks
```

README.md

@@ -46,6 +46,7 @@ to the implementation of that query in the current version of the tool.
 - [What if my project needs stronger guarantees around supported Rust versions?](#what-if-my-project-needs-stronger-guarantees-around-supported-rust-versions)
 - [Does the crate I'm checking have to be published on crates.io?](#does-the-crate-im-checking-have-to-be-published-on-cratesio)
 - [What features does `cargo-semver-checks` enable in the tested crates?](#what-features-does-cargo-semver-checks-enable-in-the-tested-crates)
+- [My crate uses `--cfg` conditional compilation. Can `cargo-semver-checks` scan it?](#my-crate-uses---cfg-conditional-compilation-can-cargo-semver-checks-scan-it)
 - [Does `cargo-semver-checks` have false positives?](#does-cargo-semver-checks-have-false-positives)
 - [Will `cargo-semver-checks` catch every semver violation?](#will-cargo-semver-checks-catch-every-semver-violation)
 - [Can I configure individual lints?](#can-i-configure-individual-lints)
@@ -154,6 +155,17 @@ For example, consider crate [serde](https://github.com/serde-rs/serde), with the
 | `--only-explicit-features`                     | none                                       | No explicit features are passed.                                   |
 | `--only-explicit-features --features unstable` | `unstable`                                 | All features can be added explicitly, regardless of their name.    |
 
+### My crate uses `--cfg` conditional compilation. Can `cargo-semver-checks` scan it?
+
+Yes! You can configure the `--cfg` options that `cargo-semver-checks` will use
+when scanning your crate by setting them in the `RUSTDOCFLAGS` environment variable.
+
+For example, you can ask `cargo-semver-checks` to enable the `some-option` config
+by invoking it as:
+```
+RUSTDOCFLAGS="--cfg some-option" cargo semver-checks
+```
+
 ### Does `cargo-semver-checks` have false positives?
 
 "False positive" means that `cargo-semver-checks` reported a semver violation incorrectly.

src/data_generation/generate.rs

@@ -244,8 +244,8 @@ fn run_cargo_doc(
     let pkg_spec = format!("{crate_name}@{version}");
 
     // Generating rustdoc JSON for a crate also involves checking that crate's dependencies.
-    // The check is done by rustc, not rustdoc, so it's subject to RUSTFLAGS not RUSTDOCFLAGS.
-    // We don't want rustc to fail that check if the user has set RUSTFLAGS="-Dwarnings" here.
+    // The check is done by rustc, not rustdoc, so it's subject to `RUSTFLAGS` not `RUSTDOCFLAGS`.
+    // We don't want rustc to fail that check if the user has set `RUSTFLAGS="-Dwarnings"` here.
     // This fixes: https://github.com/obi1kenobi/cargo-semver-checks/issues/589
     let rustflags = match std::env::var("RUSTFLAGS") {
         Ok(mut prior_rustflags) => {
@@ -255,6 +255,20 @@ fn run_cargo_doc(
         Err(_) => std::borrow::Cow::Borrowed("--cap-lints=allow"),
     };
 
+    // Ensure we preserve `RUSTDOCFLAGS` if they are set.
+    // This allows users to supply `--cfg <custom-value>` settings in `RUSTDOCFLAGS`
+    // in order to toggle what functionality is compiled into the scanned crate.
+    // Suggested in: https://github.com/obi1kenobi/cargo-semver-checks/discussions/1012
+    let extra_rustdocflags = "-Z unstable-options --document-private-items --document-hidden-items --output-format=json --cap-lints=allow";
+    let rustdocflags = match std::env::var("RUSTDOCFLAGS") {
+        Ok(mut prior_rustdocflags) => {
+            prior_rustdocflags.push(' ');
+            prior_rustdocflags.push_str(extra_rustdocflags);
+            std::borrow::Cow::Owned(prior_rustdocflags)
+        }
+        Err(_) => std::borrow::Cow::Borrowed(extra_rustdocflags),
+    };
+
     // Run the rustdoc generation command on the placeholder crate,
     // specifically requesting the rustdoc of *only* the crate specified in `pkg_spec`.
     //
@@ -266,10 +280,7 @@ fn run_cargo_doc(
     callbacks.generate_rustdoc_start();
     let mut cmd = std::process::Command::new("cargo");
     cmd.env("RUSTC_BOOTSTRAP", "1")
-        .env(
-            "RUSTDOCFLAGS",
-            "-Z unstable-options --document-private-items --document-hidden-items --output-format=json --cap-lints=allow",
-        )
+        .env("RUSTDOCFLAGS", rustdocflags.as_ref())
         .env("RUSTFLAGS", rustflags.as_ref())
         .stdout(std::process::Stdio::null()) // Don't pollute output
         .stderr(settings.stderr())

test_crates/cfg_conditional_compilation/new/Cargo.toml

@@ -0,0 +1,7 @@
+[package]
+publish = false
+name = "cfg_conditional_compilation"
+version = "0.1.0"
+edition = "2021"
+
+[dependencies]

test_crates/cfg_conditional_compilation/new/build.rs

@@ -0,0 +1,3 @@
+fn main() {
+    println!("cargo::rustc-check-cfg=cfg(custom)");
+}

test_crates/cfg_conditional_compilation/new/src/lib.rs

@@ -0,0 +1,6 @@
+pub enum Data {
+    Int(i64),
+    String(String),
+    #[cfg(custom)]
+    Bool(bool),
+}

test_crates/cfg_conditional_compilation/old/Cargo.toml

@@ -0,0 +1,7 @@
+[package]
+publish = false
+name = "cfg_conditional_compilation"
+version = "0.1.0"
+edition = "2021"
+
+[dependencies]

test_crates/cfg_conditional_compilation/old/build.rs

@@ -0,0 +1,3 @@
+fn main() {
+    println!("cargo::rustc-check-cfg=cfg(custom)");
+}

test_crates/cfg_conditional_compilation/old/src/lib.rs

@@ -0,0 +1,7 @@
+#[cfg(custom)]
+pub struct Example;
+
+pub enum Data {
+    Int(i64),
+    String(String),
+}

test_outputs/integration_snapshots__cfg_conditional_compilation.snap

@@ -0,0 +1,54 @@
+---
+source: tests/integration_snapshots.rs
+info:
+  program: cargo-semver-checks
+  args:
+    - semver-checks
+    - "--manifest-path"
+    - test_crates/cfg_conditional_compilation/new
+    - "--baseline-root"
+    - test_crates/cfg_conditional_compilation/old
+  env:
+    CARGO_TERM_COLOR: never
+    RUSTDOCFLAGS: "--cfg custom"
+    RUSTFLAGS: "--cfg custom"
+    RUST_BACKTRACE: "0"
+---
+success: false
+exit_code: 1
+----- stdout -----
+
+--- failure enum_variant_added: enum variant added on exhaustive enum ---
+
+Description:
+A publicly-visible enum without #[non_exhaustive] has a new variant.
+        ref: https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new
+       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/[VERSION]/src/lints/enum_variant_added.ron
+
+Failed in:
+  variant Data:Bool in [ROOT]/test_crates/cfg_conditional_compilation/new/src/lib.rs:5
+
+--- failure struct_missing: pub struct removed or renamed ---
+
+Description:
+A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely.
+        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
+       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/[VERSION]/src/lints/struct_missing.ron
+
+Failed in:
+  struct cfg_conditional_compilation::Example, previously in file [ROOT]/test_crates/cfg_conditional_compilation/old/src/lib.rs:2
+
+----- stderr -----
+    Building cfg_conditional_compilation v0.1.0 (current)
+       Built [TIME] (current)
+     Parsing cfg_conditional_compilation v0.1.0 (current)
+      Parsed [TIME] (current)
+    Building cfg_conditional_compilation v0.1.0 (baseline)
+       Built [TIME] (baseline)
+     Parsing cfg_conditional_compilation v0.1.0 (baseline)
+      Parsed [TIME] (baseline)
+    Checking cfg_conditional_compilation v0.1.0 -> v0.1.0 (no change)
+     Checked [TIME] [TOTAL] checks: [PASS] pass, 2 fail, 0 warn, 0 skip
+
+     Summary semver requires new major version: 2 major and 0 minor checks failed
+    Finished [TIME] cfg_conditional_compilation

test_outputs/integration_snapshots__cfg_conditional_compilation_without_cfg_set.snap

@@ -0,0 +1,31 @@
+---
+source: tests/integration_snapshots.rs
+info:
+  program: cargo-semver-checks
+  args:
+    - semver-checks
+    - "--manifest-path"
+    - test_crates/cfg_conditional_compilation/new
+    - "--baseline-root"
+    - test_crates/cfg_conditional_compilation/old
+  env:
+    CARGO_TERM_COLOR: never
+    RUST_BACKTRACE: "0"
+---
+success: true
+exit_code: 0
+----- stdout -----
+
+----- stderr -----
+    Building cfg_conditional_compilation v0.1.0 (current)
+       Built [TIME] (current)
+     Parsing cfg_conditional_compilation v0.1.0 (current)
+      Parsed [TIME] (current)
+    Building cfg_conditional_compilation v0.1.0 (baseline)
+       Built [TIME] (baseline)
+     Parsing cfg_conditional_compilation v0.1.0 (baseline)
+      Parsed [TIME] (baseline)
+    Checking cfg_conditional_compilation v0.1.0 -> v0.1.0 (no change)
+     Checked [TIME] [TOTAL] checks: [PASS] pass, 0 skip
+     Summary no semver update required
+    Finished [TIME] cfg_conditional_compilation

tests/integration_snapshots.rs

@@ -119,6 +119,86 @@ fn z_help() {
     })
 }
 
+/// Pin down the behavior when running `cargo-semver-checks` on a package that
+/// relies on `--cfg` based conditional compilation to enable or disable functionality.
+///
+/// An example of such a crate in the wild:
+/// https://docs.rs/aes/latest/aes/#configuration-flags
+///
+/// Since `RUSTDOCFLAGS` is set to `--cfg custom`, the crate should be checked with that setting.
+/// This should reveal two breaking changes -- one caused by `cfg` in the baseline
+/// and one caused by `cfg` in the current.
+#[test]
+fn cfg_conditional_compilation() {
+    assert_integration_test("cfg_conditional_compilation", |cmd, settings| {
+        cmd.args([
+            "--manifest-path",
+            "test_crates/cfg_conditional_compilation/new",
+            "--baseline-root",
+            "test_crates/cfg_conditional_compilation/old",
+        ])
+        .env("RUSTDOCFLAGS", "--cfg custom");
+
+        set_snapshot_filters(settings);
+    });
+}
+
+/// Analogous test to the one above, but when the `--cfg` is not set.
+/// In this case, no breakage should be reported.
+#[test]
+fn cfg_conditional_compilation_without_cfg_set() {
+    assert_integration_test(
+        "cfg_conditional_compilation_without_cfg_set",
+        |cmd, settings| {
+            cmd.args([
+                "--manifest-path",
+                "test_crates/cfg_conditional_compilation/new",
+                "--baseline-root",
+                "test_crates/cfg_conditional_compilation/old",
+            ]);
+
+            set_snapshot_filters(settings);
+        },
+    );
+}
+
+fn set_snapshot_filters(settings: &mut insta::Settings) {
+    // Turn dynamic time strings like [  0.123s] into [TIME] for reproducibility.
+    settings.add_filter(r"\[\s*[\d\.]+s\]", "[TIME]");
+    // Turn total number of checks into [TOTAL] to not fail when new lints are added.
+    settings.add_filter(r"\d+ checks", "[TOTAL] checks");
+    // Similarly, turn the number of passed checks to also not fail when new lints are added.
+    settings.add_filter(r"\d+ pass", "[PASS] pass");
+    // Escape the root path (e.g., in lint spans) for deterministic results in different
+    // build environments.
+    let repo_root = get_root_path();
+    settings.add_filter(&regex::escape(&repo_root.to_string_lossy()), "[ROOT]");
+    // Remove cargo blocking lines (e.g. from `cargo doc` output) as the amount of blocks
+    // is not reproducible.
+    settings.add_filter("    Blocking waiting for file lock on package cache\n", "");
+    // Filter out the current `cargo-semver-checks` version in links to lint references,
+    // as this will break across version changes.
+    settings.add_filter(
+        r"v\d+\.\d+\.\d+(-[\w\.-]+)?/src/lints",
+        "[VERSION]/src/lints",
+    );
+}
+
+/// Helper function to get the root of the source code repository, for
+/// filtering the path in snapshots.
+fn get_root_path() -> PathBuf {
+    let canonicalized = Path::new(file!())
+        .canonicalize()
+        .expect("canonicalization failed");
+    // this file is in `$ROOT/tests/integration_snapshots.rs`, so the repo root is two `parent`s up.
+    let repo_root = canonicalized
+        .parent()
+        .and_then(Path::parent)
+        .expect("getting repo root failed");
+
+    repo_root.to_owned()
+}
+
 /// Helper function to get a canonicalized version of the cargo executable bin.
 fn executable_path() -> PathBuf {
     Path::new(