Merge pull request #795 from cgwalters/human-readable-followup

Make lints stricter, apply crate wide

.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

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

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

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

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;

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<Vec<BoundImage>> {
     Ok(bound_images)
 }
 
+#[cfg(feature = "install")]
 impl ResolvedBoundImage {
     pub(crate) async fn from_image(src: &BoundImage) -> Result<Self> {
         let proxy = containers_image_proxy::ImageProxy::new().await?;

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,

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;

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,13 +217,15 @@ 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,
     Labeled,
 }
 
 /// Query the SELinux labeling for a particular path
+#[cfg(feature = "install")]
 pub(crate) fn has_security_selinux(root: &Dir, path: &Utf8Path) -> Result<SELinuxLabelState> {
     // 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<SELinu
     }
 }
 
+#[cfg(feature = "install")]
 pub(crate) fn set_security_selinux_path(root: &Dir, path: &Utf8Path, label: &[u8]) -> 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,

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<String> {
     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<Utf8Path>) -> Result<bool> {
     fn impl_storage_exists(root: &Dir, path: &Utf8Path) -> Result<bool> {
         let lock = "storage.lock";
@@ -49,6 +55,7 @@ pub(crate) fn storage_exists(root: &Dir, path: impl AsRef<Utf8Path>) -> Result<b
 ///
 /// Note this does not attempt to parse the root filesystem's container storage configuration,
 /// this uses a hardcoded default path.
+#[cfg(feature = "install")]
 pub(crate) fn storage_exists_default(root: &Dir) -> Result<bool> {
     storage_exists(root, CONTAINER_STORAGE.trim_start_matches('/'))
 }

lib/src/status.rs

@@ -100,6 +100,7 @@ pub(crate) struct Deployments {
     pub(crate) other: VecDeque<ostree::Deployment>,
 }
 
+#[cfg(feature = "install")]
 pub(crate) fn try_deserialize_timestamp(t: &str) -> Option<chrono::DateTime<chrono::Utc>> {
     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<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 {
+    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<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"
     Current staged image: quay.io/example/someimage:latest
         Image version: nightly (2023-10-14 19:22:15 UTC)
         Image transport: registry
@@ -386,61 +391,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 +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()))
+        );
+    }
 }