From 718373a914069f3d8496c1e35de710d9290e4e3c Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 18 Sep 2024 11:33:00 -0400 Subject: [PATCH] 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 --- Cargo.toml | 4 + Makefile | 4 +- cli/src/main.rs | 3 + lib/build.rs | 2 +- lib/src/cli.rs | 2 +- lib/src/lib.rs | 4 - lib/src/status.rs | 144 +++++++++++---------- tests-integration/src/tests-integration.rs | 2 + utils/src/command.rs | 6 +- xtask/src/xtask.rs | 3 + 10 files changed, 95 insertions(+), 79 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 56382031a..0f775d4a7 100644 --- 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 index 0b8a86305..3247fe449 100644 --- 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 index 8af630640..8407c41c4 100644 --- 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 index 88690b557..54746b354 100644 --- 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 index 0438cc3aa..a2c4986b3 100644 --- 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(args: I) -> Result<()> where I: IntoIterator, diff --git a/lib/src/lib.rs b/lib/src/lib.rs index b2263b017..d8255499a 100644 --- 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 index 67f550624..28ec23c47 100644 --- 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 { - 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 { + 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,29 +388,30 @@ 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 @@ -416,15 +419,15 @@ fn test_human_readable_staged_spec() { 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) @@ -432,15 +435,15 @@ fn test_human_readable_booted_spec() { 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 index 4e44b96ea..de84c8e34 100644 --- 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 index 3a360caae..0ce4b464f 100644 --- 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(&mut self) -> Result; @@ -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 index df55e2ffb..4e283ffb3 100644 --- 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};