Make lints stricter, apply crate wide

Add `dead_code = "deny"` to our default lints; we had a compiler warning for this in main. Fix the warning by moving the human readable test code into `#[cfg(test)]`. While we're here, move the other lib.rs lints into the crate; enforcing docs for *everything* at first I thought might be heavy handed but actually is fine as it only applies to things that are `pub`, of which we don't actually have that much so it mainly forced me to add some stub docs for the modules, which is probably a good idea. Signed-off-by: Colin Walters <[email protected]>
cgwalters · Sep 18, 2024 · 718373a · 718373a
1 parent ecabb89
commit 718373a
Show file tree

Hide file tree

Showing 10 changed files with 95 additions and 79 deletions.
diff --git a/Cargo.toml b/Cargo.toml
@@ -57,6 +57,10 @@ exclude-crate-paths = [ { name = "libz-sys", exclude = "src/zlib" },
 unsafe_code = "deny"
 # Absolutely must handle errors
 unused_must_use = "forbid"
+missing_docs = "deny"
+missing_debug_implementations = "deny"
+# Feel free to comment this one out locally during development of a patch.
+dead_code = "deny"
 
 [workspace.lints.clippy]
 # These should only be in local code

diff --git a/Makefile b/Makefile
@@ -38,10 +38,12 @@ test-bin-archive: all
 test-tmt:
 	cargo xtask test-tmt
 
-# Checks extra rust things (formatting and select clippy lints)
+# Checks extra rust things (formatting, a few extra rust warnings, and select clippy lints)
 validate-rust:
 	cargo fmt -- --check -l
+	cargo check
 	cargo clippy -- -D clippy::correctness -D clippy::suspicious
+	env RUSTDOCFLAGS='-D warnings' cargo doc --lib
 .PHONY: validate-rust
 
 validate: validate-rust

diff --git a/cli/src/main.rs b/cli/src/main.rs
@@ -1,3 +1,6 @@
+//! The main entrypoint for bootc, which just performs global initialization, and then
+//! calls out into the library.
+
 use anyhow::Result;
 
 /// The code called after we've done process global init and created

diff --git a/lib/build.rs b/lib/build.rs
@@ -1,4 +1,4 @@
-// build.rs
+//! Our build script, which basically today just generates a version.
 
 use std::env;
 use std::fs;

diff --git a/lib/src/cli.rs b/lib/src/cli.rs
@@ -799,7 +799,7 @@ pub fn global_init() -> Result<()> {
 }
 
 /// Parse the provided arguments and execute.
-/// Calls [`structopt::clap::Error::exit`] on failure, printing the error message and aborting the program.
+/// Calls [`clap::Error::exit`] on failure, printing the error message and aborting the program.
 pub async fn run_from_iter<I>(args: I) -> Result<()>
 where
     I: IntoIterator,

diff --git a/lib/src/lib.rs b/lib/src/lib.rs
@@ -4,10 +4,6 @@
 //! to provide a fully "container native" tool for using
 //! bootable container images.
 
-// See https://doc.rust-lang.org/rustc/lints/listing/allowed-by-default.html
-#![deny(missing_docs)]
-#![deny(missing_debug_implementations)]
-
 mod boundimage;
 pub mod cli;
 pub(crate) mod deploy;

diff --git a/lib/src/status.rs b/lib/src/status.rs
@@ -362,20 +362,22 @@ fn human_readable_output(mut out: impl Write, host: &Host) -> Result<()> {
     Ok(())
 }
 
-fn human_status_from_spec_fixture(spec_fixture: &str) -> Result<String> {
-    let host: Host = serde_yaml::from_str(spec_fixture).unwrap();
-    let mut w = Vec::new();
-    human_readable_output(&mut w, &host).unwrap();
-    let w = String::from_utf8(w).unwrap();
-    Ok(w)
-}
+#[cfg(test)]
+mod tests {
+    fn human_status_from_spec_fixture(spec_fixture: &str) -> Result<String> {
+        let host: Host = serde_yaml::from_str(spec_fixture).unwrap();
+        let mut w = Vec::new();
+        human_readable_output(&mut w, &host).unwrap();
+        let w = String::from_utf8(w).unwrap();
+        Ok(w)
+    }
 
-#[test]
-fn test_human_readable_base_spec() {
-    // Tests Staged and Booted, null Rollback
-    let w = human_status_from_spec_fixture(include_str!("fixtures/spec-staged-booted.yaml"))
-        .expect("No spec found");
-    let expected = indoc::indoc! { r"
+    #[test]
+    fn test_human_readable_base_spec() {
+        // Tests Staged and Booted, null Rollback
+        let w = human_status_from_spec_fixture(include_str!("fixtures/spec-staged-booted.yaml"))
+            .expect("No spec found");
+        let expected = indoc::indoc! { r"
     Current staged image: quay.io/example/someimage:latest
         Image version: nightly (2023-10-14 19:22:15 UTC)
         Image transport: registry
@@ -386,61 +388,62 @@ fn test_human_readable_base_spec() {
         Image digest: sha256:736b359467c9437c1ac915acaae952aad854e07eb4a16a94999a48af08c83c34
     No rollback image present
     "};
-    similar_asserts::assert_eq!(w, expected);
-}
+        similar_asserts::assert_eq!(w, expected);
+    }
 
-#[test]
-fn test_human_readable_rfe_spec() {
-    // Basic rhel for edge bootc install with nothing
-    let w =
-        human_status_from_spec_fixture(include_str!("fixtures/spec-rfe-ostree-deployment.yaml"))
-            .expect("No spec found");
-    let expected = indoc::indoc! { r"
+    #[test]
+    fn test_human_readable_rfe_spec() {
+        // Basic rhel for edge bootc install with nothing
+        let w = human_status_from_spec_fixture(include_str!(
+            "fixtures/spec-rfe-ostree-deployment.yaml"
+        ))
+        .expect("No spec found");
+        let expected = indoc::indoc! { r"
     Current staged state is native ostree
     Current booted state is native ostree
     No rollback image present
     "};
-    similar_asserts::assert_eq!(w, expected);
-}
+        similar_asserts::assert_eq!(w, expected);
+    }
 
-#[test]
-fn test_human_readable_staged_spec() {
-    // staged image, no boot/rollback
-    let w = human_status_from_spec_fixture(include_str!("fixtures/spec-ostree-to-bootc.yaml"))
-        .expect("No spec found");
-    let expected = indoc::indoc! { r"
+    #[test]
+    fn test_human_readable_staged_spec() {
+        // staged image, no boot/rollback
+        let w = human_status_from_spec_fixture(include_str!("fixtures/spec-ostree-to-bootc.yaml"))
+            .expect("No spec found");
+        let expected = indoc::indoc! { r"
     Current staged image: quay.io/centos-bootc/centos-bootc:stream9
         Image version: stream9.20240807.0 (No timestamp present)
         Image transport: registry
         Image digest: sha256:47e5ed613a970b6574bfa954ab25bb6e85656552899aa518b5961d9645102b38
     Current booted state is native ostree
     No rollback image present
     "};
-    similar_asserts::assert_eq!(w, expected);
-}
+        similar_asserts::assert_eq!(w, expected);
+    }
 
-#[test]
-fn test_human_readable_booted_spec() {
-    // booted image, no staged/rollback
-    let w = human_status_from_spec_fixture(include_str!("fixtures/spec-only-booted.yaml"))
-        .expect("No spec found");
-    let expected = indoc::indoc! { r"
+    #[test]
+    fn test_human_readable_booted_spec() {
+        // booted image, no staged/rollback
+        let w = human_status_from_spec_fixture(include_str!("fixtures/spec-only-booted.yaml"))
+            .expect("No spec found");
+        let expected = indoc::indoc! { r"
     No staged image present
     Current booted image: quay.io/centos-bootc/centos-bootc:stream9
         Image version: stream9.20240807.0 (No timestamp present)
         Image transport: registry
         Image digest: sha256:47e5ed613a970b6574bfa954ab25bb6e85656552899aa518b5961d9645102b38
     No rollback image present
     "};
-    similar_asserts::assert_eq!(w, expected);
-}
+        similar_asserts::assert_eq!(w, expected);
+    }
 
-#[test]
-fn test_human_readable_staged_rollback_spec() {
-    // staged/rollback image, no booted
-    let w = human_status_from_spec_fixture(include_str!("fixtures/spec-staged-rollback.yaml"))
-        .expect("No spec found");
-    let expected = indoc::indoc! { r"
+    #[test]
+    fn test_human_readable_staged_rollback_spec() {
+        // staged/rollback image, no booted
+        let w = human_status_from_spec_fixture(include_str!("fixtures/spec-staged-rollback.yaml"))
+            .expect("No spec found");
+        let expected = indoc::indoc! { r"
     Current staged image: quay.io/example/someimage:latest
         Image version: nightly (2023-10-14 19:22:15 UTC)
         Image transport: registry
@@ -451,29 +454,30 @@ fn test_human_readable_staged_rollback_spec() {
         Image transport: registry
         Image digest: sha256:736b359467c9437c1ac915acaae952aad854e07eb4a16a94999a48af08c83c34
     "};
-    similar_asserts::assert_eq!(w, expected);
-}
+        similar_asserts::assert_eq!(w, expected);
+    }
 
-#[test]
-fn test_convert_signatures() {
-    use std::str::FromStr;
-    let ir_unverified = &OstreeImageReference::from_str(
-        "ostree-unverified-registry:quay.io/someexample/foo:latest",
-    )
-    .unwrap();
-    let ir_ostree = &OstreeImageReference::from_str(
-        "ostree-remote-registry:fedora:quay.io/fedora/fedora-coreos:stable",
-    )
-    .unwrap();
-
-    let ir = ImageReference::from(ir_unverified.clone());
-    assert_eq!(ir.image, "quay.io/someexample/foo:latest");
-    assert_eq!(ir.signature, None);
-
-    let ir = ImageReference::from(ir_ostree.clone());
-    assert_eq!(ir.image, "quay.io/fedora/fedora-coreos:stable");
-    assert_eq!(
-        ir.signature,
-        Some(ImageSignature::OstreeRemote("fedora".into()))
-    );
+    #[test]
+    fn test_convert_signatures() {
+        use std::str::FromStr;
+        let ir_unverified = &OstreeImageReference::from_str(
+            "ostree-unverified-registry:quay.io/someexample/foo:latest",
+        )
+        .unwrap();
+        let ir_ostree = &OstreeImageReference::from_str(
+            "ostree-remote-registry:fedora:quay.io/fedora/fedora-coreos:stable",
+        )
+        .unwrap();
+
+        let ir = ImageReference::from(ir_unverified.clone());
+        assert_eq!(ir.image, "quay.io/someexample/foo:latest");
+        assert_eq!(ir.signature, None);
+
+        let ir = ImageReference::from(ir_ostree.clone());
+        assert_eq!(ir.image, "quay.io/fedora/fedora-coreos:stable");
+        assert_eq!(
+            ir.signature,
+            Some(ImageSignature::OstreeRemote("fedora".into()))
+        );
+    }
 }
diff --git a/tests-integration/src/tests-integration.rs b/tests-integration/src/tests-integration.rs
@@ -1,3 +1,5 @@
+//! Integration tests.
+
 use std::path::PathBuf;
 
 use camino::Utf8PathBuf;

diff --git a/utils/src/command.rs b/utils/src/command.rs
@@ -1,3 +1,5 @@
+//! Helpers intended for [`std::process::Command`] and related structures.
+
 use std::{
     io::{Read, Seek},
     process::Command,
@@ -7,6 +9,7 @@ use anyhow::{Context, Result};
 
 /// Helpers intended for [`std::process::Command`].
 pub trait CommandRunExt {
+    /// Execute the child process.
     fn run(&mut self) -> Result<()>;
     /// Execute the child process, parsing its stdout as JSON.
     fn run_and_parse_json<T: serde::de::DeserializeOwned>(&mut self) -> Result<T>;
@@ -84,12 +87,11 @@ impl CommandRunExt for Command {
 /// Helpers intended for [`tokio::process::Command`].
 #[allow(async_fn_in_trait)]
 pub trait AsyncCommandRunExt {
+    /// Asynchronously execute the child, and return an error if the child exited unsuccessfully.
     async fn run(&mut self) -> Result<()>;
 }
 
 impl AsyncCommandRunExt for tokio::process::Command {
-    /// Asynchronously execute the child, and return an error if the child exited unsuccessfully.
-    ///
     async fn run(&mut self) -> Result<()> {
         let stderr = tempfile::tempfile()?;
         self.stderr(stderr.try_clone()?);

diff --git a/xtask/src/xtask.rs b/xtask/src/xtask.rs
@@ -1,3 +1,6 @@
+//! See https://github.com/matklad/cargo-xtask
+//! This is kind of like "Justfile but in Rust".
+
 use std::fs::File;
 use std::io::{BufRead, BufReader, BufWriter, Write};
 use std::process::{Command, Stdio};