From 97692bd1c25620d78cd3e9779892d87b4341d614 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 --- .github/workflows/ci.yml | 11 +- Cargo.toml | 4 + Makefile | 6 +- cli/src/main.rs | 3 + lib/build.rs | 2 +- lib/src/boundimage.rs | 3 + lib/src/cli.rs | 2 +- lib/src/lib.rs | 4 - lib/src/lsm.rs | 11 +- lib/src/podman.rs | 11 +- lib/src/status.rs | 147 +++++++++++---------- lib/src/task.rs | 2 + lib/src/utils.rs | 2 + tests-integration/src/tests-integration.rs | 2 + utils/src/command.rs | 6 +- xtask/src/xtask.rs | 3 + 16 files changed, 129 insertions(+), 90 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4855d348..3ef05e0a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -33,14 +33,9 @@ jobs: uses: Swatinem/rust-cache@v2 with: key: "tests" - - name: cargo fmt (check) - run: cargo fmt -- --check -l - - name: Build - run: cargo test --no-run - - name: Build lib without default features - run: cd lib && cargo check --no-default-features - - name: Individual checks - run: (cd cli && cargo check) && (cd lib && cargo check) + - name: make validate-rust + # the ruff checks are covered via a dedicated action + run: make validate-rust - name: Run tests run: cargo test -- --nocapture --quiet - name: Manpage generation diff --git a/Cargo.toml b/Cargo.toml index 56382031..0f775d4a 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 0b8a8630..1a7fd64b 100644 --- a/Makefile +++ b/Makefile @@ -38,10 +38,14 @@ 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 + (cd lib && cargo check --no-default-features) + cargo test --no-run 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 8af63064..8407c41c 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 88690b55..54746b35 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/boundimage.rs b/lib/src/boundimage.rs index 95f328bf..f5b552c3 100644 --- a/lib/src/boundimage.rs +++ b/lib/src/boundimage.rs @@ -12,6 +12,7 @@ use camino::Utf8Path; use cap_std_ext::cap_std::fs::Dir; use cap_std_ext::dirext::CapStdExtDirExt; use fn_error_context::context; +#[cfg(feature = "install")] use ostree_ext::containers_image_proxy; use ostree_ext::ostree::Deployment; @@ -34,6 +35,7 @@ pub(crate) struct BoundImage { } #[derive(Debug, PartialEq, Eq)] +#[cfg(feature = "install")] pub(crate) struct ResolvedBoundImage { pub(crate) image: String, pub(crate) digest: String, @@ -104,6 +106,7 @@ pub(crate) fn query_bound_images(root: &Dir) -> Result> { Ok(bound_images) } +#[cfg(feature = "install")] impl ResolvedBoundImage { pub(crate) async fn from_image(src: &BoundImage) -> Result { let proxy = containers_image_proxy::ImageProxy::new().await?; diff --git a/lib/src/cli.rs b/lib/src/cli.rs index 0438cc3a..a2c4986b 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 b2263b01..d8255499 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/lsm.rs b/lib/src/lsm.rs index 773616d7..ddd308a3 100644 --- a/lib/src/lsm.rs +++ b/lib/src/lsm.rs @@ -7,10 +7,15 @@ use std::process::Command; use anyhow::{Context, Result}; use camino::{Utf8Path, Utf8PathBuf}; -use cap_std::fs::{Dir, DirBuilder, OpenOptions}; +use cap_std::fs::Dir; +#[cfg(feature = "install")] +use cap_std::fs::{DirBuilder, OpenOptions}; +#[cfg(feature = "install")] use cap_std::io_lifetimes::AsFilelike; use cap_std_ext::cap_std; +#[cfg(feature = "install")] use cap_std_ext::cap_std::fs::{Metadata, MetadataExt}; +#[cfg(feature = "install")] use cap_std_ext::dirext::CapStdExtDirExt; use fn_error_context::context; #[cfg(feature = "install")] @@ -212,6 +217,7 @@ pub(crate) fn set_security_selinux(fd: std::os::fd::BorrowedFd, label: &[u8]) -> /// The labeling state; "unsupported" is distinct as we need to handle /// cases like the ESP which don't support labeling. +#[cfg(feature = "install")] pub(crate) enum SELinuxLabelState { Unlabeled, Unsupported, @@ -219,6 +225,7 @@ pub(crate) enum SELinuxLabelState { } /// Query the SELinux labeling for a particular path +#[cfg(feature = "install")] pub(crate) fn has_security_selinux(root: &Dir, path: &Utf8Path) -> Result { // TODO: avoid hardcoding a max size here let mut buf = [0u8; 2048]; @@ -231,6 +238,7 @@ pub(crate) fn has_security_selinux(root: &Dir, path: &Utf8Path) -> Result Result<()> { // TODO: avoid hardcoding a max size here let fdpath = format!("/proc/self/fd/{}/", root.as_raw_fd()); @@ -244,6 +252,7 @@ pub(crate) fn set_security_selinux_path(root: &Dir, path: &Utf8Path, label: &[u8 Ok(()) } +#[cfg(feature = "install")] pub(crate) fn ensure_labeled( root: &Dir, path: &Utf8Path, diff --git a/lib/src/podman.rs b/lib/src/podman.rs index 2f8d0196..058984ee 100644 --- a/lib/src/podman.rs +++ b/lib/src/podman.rs @@ -1,14 +1,19 @@ -use anyhow::{anyhow, Result}; +#[cfg(feature = "install")] +use anyhow::Result; +#[cfg(feature = "install")] use camino::Utf8Path; +#[cfg(feature = "install")] use cap_std_ext::cap_std::fs::Dir; use serde::Deserialize; /// Where we look inside our container to find our own image /// for use with `bootc install`. +#[cfg(feature = "install")] pub(crate) const CONTAINER_STORAGE: &str = "/var/lib/containers"; #[derive(Deserialize)] #[serde(rename_all = "PascalCase")] +#[cfg(feature = "install")] pub(crate) struct Inspect { pub(crate) digest: String, } @@ -31,11 +36,12 @@ pub(crate) fn imageid_to_digest(imgid: &str) -> Result { let i = o .into_iter() .next() - .ok_or_else(|| anyhow!("No images returned for inspect"))?; + .ok_or_else(|| anyhow::anyhow!("No images returned for inspect"))?; Ok(i.digest) } /// Return true if there is apparently an active container store at the target path. +#[cfg(feature = "install")] pub(crate) fn storage_exists(root: &Dir, path: impl AsRef) -> Result { fn impl_storage_exists(root: &Dir, path: &Utf8Path) -> Result { let lock = "storage.lock"; @@ -49,6 +55,7 @@ pub(crate) fn storage_exists(root: &Dir, path: impl AsRef) -> Result Result { storage_exists(root, CONTAINER_STORAGE.trim_start_matches('/')) } diff --git a/lib/src/status.rs b/lib/src/status.rs index 67f55062..1f00d020 100644 --- a/lib/src/status.rs +++ b/lib/src/status.rs @@ -100,6 +100,7 @@ pub(crate) struct Deployments { pub(crate) other: VecDeque, } +#[cfg(feature = "install")] pub(crate) fn try_deserialize_timestamp(t: &str) -> Option> { match chrono::DateTime::parse_from_rfc3339(t).context("Parsing timestamp") { Ok(t) => Some(t.into()), @@ -362,20 +363,24 @@ 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 { + use super::*; -#[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" + 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" Current staged image: quay.io/example/someimage:latest Image version: nightly (2023-10-14 19:22:15 UTC) Image transport: registry @@ -386,29 +391,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 +422,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 +438,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 +457,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/lib/src/task.rs b/lib/src/task.rs index 19ebc447..5db8e95b 100644 --- a/lib/src/task.rs +++ b/lib/src/task.rs @@ -21,6 +21,7 @@ enum CmdVerbosity { Verbose, } +/// Too many things in the install path are conditional pub(crate) struct Task { description: String, verbosity: CmdVerbosity, @@ -28,6 +29,7 @@ pub(crate) struct Task { pub(crate) cmd: Command, } +#[allow(dead_code)] impl Task { pub(crate) fn new(description: impl AsRef, exe: impl AsRef) -> Self { Self::new_cmd(description, Command::new(exe.as_ref())) diff --git a/lib/src/utils.rs b/lib/src/utils.rs index 2d865fdc..81b253fe 100644 --- a/lib/src/utils.rs +++ b/lib/src/utils.rs @@ -43,6 +43,7 @@ pub(crate) fn deployment_fd( /// Given an mount option string list like foo,bar=baz,something=else,ro parse it and find /// the first entry like $optname= /// This will not match a bare `optname` without an equals. +#[cfg(feature = "install")] pub(crate) fn find_mount_option<'a>( option_string_list: &'a str, optname: &'_ str, @@ -99,6 +100,7 @@ pub(crate) fn sigpolicy_from_opts( /// Output a warning message that we want to be quite visible. /// The process (thread) execution will be delayed for a short time. +#[cfg(feature = "install")] pub(crate) fn medium_visibility_warning(s: &str) { anstream::eprintln!( "{}{s}{}", diff --git a/tests-integration/src/tests-integration.rs b/tests-integration/src/tests-integration.rs index 4e44b96e..de84c8e3 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 3a360caa..0ce4b464 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 df55e2ff..4e283ffb 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};