Merge pull request #279 from nix-community/ci-refactor

CI: refactor

Author: NotAShelf

.github/workflows/build.yaml

@@ -8,9 +8,9 @@ on:
   workflow_dispatch:
 
 jobs:
-  build:
+  build-linux:
+    name: "Build NH on Linux"
     runs-on: ubuntu-latest
-
     steps:
       - uses: cachix/install-nix-action@master
         with:
@@ -21,13 +21,8 @@ jobs:
       - run: nix build -L --no-link
         name: Build
 
-      - run: |
-          eval "$(nix print-dev-env)"
-          ./fix.sh
-          git diff-index --quiet HEAD
-        name: Check formatting
-
-  Test_Darwin:
+  build-darwin:
+    name: "Build NH on Darwin"
     runs-on: macos-latest
 
     steps:
@@ -40,6 +35,9 @@ jobs:
       - run: nix build -L --no-link
         name: Build
 
+      # FIXME: this should be moved out of the build workflow. It is **not** a build job
+      # and opens the door to CI failures due to upstream (nix-darwin) errors. This should
+      # instead me made into a VM test.
       - run: |
           mkdir flake
           cd flake

.github/workflows/check.yaml

@@ -0,0 +1,37 @@
+name: "Check formating & lints"
+
+on:
+  workflow_dispatch:
+  pull_request:
+    branches: [ "main" ]
+  push:
+    branches-ignore:
+      - 'update-*'
+
+jobs:
+  treewide-checks:
+    runs-on: ubuntu-latest
+
+    steps:
+      - uses: cachix/install-nix-action@master
+        with:
+          github_access_token: ${{ secrets.GITHUB_TOKEN }}
+
+      - name: Checkout repository
+        uses: actions/checkout@v4
+
+      # Unlike the clippy lints below, this should always pass.
+      - name: Check Formatting
+        run: |
+          # Run cargo fmt in check mode
+          cargo fmt --check
+
+
+      # We run clippy with lints that help avoid overall low-quality code or what is called "code smell."
+      # Stylistic lints (e.g., clippy::style and clippy::complexity) are avoided but it is a good idea to
+      # follow those while working on the codebase.
+      - name: Clippy Lints
+        run: |
+          # Lint Changes
+          cargo clippy -- -W clippy::pedantic -W clippy::correctness -W clippy::suspicious
+

Cargo.toml

@@ -3,7 +3,8 @@ name = "nh"
 version = "4.0.3"
 edition = "2021"
 license = "EUPL-1.2"
-# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
+repository = "https://github.com/nix-community/nh"
+description = "Yet Another Nix Helper"
 
 [dependencies]
 anstyle = "1.0.0"

fix.sh

@@ -1,7 +1,15 @@
 #! /usr/bin/env bash
 set -eux
 
+echo "Running 'cargo fix' on the codebase"
 cargo fix --allow-dirty
-cargo clippy --fix --allow-dirty
+
+echo "Running clippy linter and applying available fixes"
+cargo clippy --fix --allow-dirty \
+	-W clippy::pedantic \
+	-W clippy::pedantic \
+	-W clippy::correctness \
+	-W clippy::suspicious
+
+echo "Running formatter"
 cargo fmt
-# nix fmt # FIXME

src/clean.rs

@@ -15,7 +15,7 @@ use regex::Regex;
 use tracing::{debug, info, instrument, span, warn, Level};
 use uzers::os::unix::UserExt;
 
-use crate::{commands::Command, *};
+use crate::{commands::Command, interface, Result};
 
 // Nix impl:
 // https://github.com/NixOS/nix/blob/master/src/nix-collect-garbage/nix-collect-garbage.cc
@@ -42,12 +42,12 @@ impl interface::CleanMode {
         // What profiles to clean depending on the call mode
         let uid = nix::unistd::Uid::effective();
         let args = match self {
-            interface::CleanMode::Profile(args) => {
+            Self::Profile(args) => {
                 profiles.push(args.profile.clone());
                 is_profile_clean = true;
                 &args.common
             }
-            interface::CleanMode::All(args) => {
+            Self::All(args) => {
                 if !uid.is_root() {
                     crate::self_elevate();
                 }
@@ -72,7 +72,7 @@ impl interface::CleanMode {
                 }
                 args
             }
-            interface::CleanMode::User(args) => {
+            Self::User(args) => {
                 if uid.is_root() {
                     bail!("nh clean user: don't run me as root!");
                 }
@@ -129,7 +129,7 @@ impl interface::CleanMode {
                     AccessFlags::F_OK | AccessFlags::W_OK,
                     AtFlags::AT_SYMLINK_NOFOLLOW,
                 ) {
-                    Ok(_) => true,
+                    Ok(()) => true,
                     Err(errno) => match errno {
                         Errno::EACCES | Errno::ENOENT => false,
                         _ => {
@@ -191,7 +191,7 @@ impl interface::CleanMode {
             }
             println!();
         }
-        for (profile, generations_tagged) in profiles_tagged.iter() {
+        for (profile, generations_tagged) in &profiles_tagged {
             println!("{}", profile.to_string_lossy().blue().bold());
             for (gen, tbr) in generations_tagged.iter().rev() {
                 if *tbr {
@@ -218,7 +218,7 @@ impl interface::CleanMode {
                 }
             }
 
-            for (_, generations_tagged) in profiles_tagged.iter() {
+            for generations_tagged in profiles_tagged.values() {
                 for (gen, tbr) in generations_tagged.iter().rev() {
                     if *tbr {
                         remove_path_nofail(&gen.path);
@@ -324,7 +324,7 @@ fn cleanable_generations(
     }
 
     let now = SystemTime::now();
-    for (gen, tbr) in result.iter_mut() {
+    for (gen, tbr) in &mut result {
         match now.duration_since(gen.last_modified) {
             Err(err) => {
                 warn!(?err, ?now, ?gen, "Failed to compare time!");

src/commands.rs

@@ -43,12 +43,12 @@ impl Command {
         }
     }
 
-    pub fn elevate(mut self, elevate: bool) -> Self {
+    pub const fn elevate(mut self, elevate: bool) -> Self {
         self.elevate = elevate;
         self
     }
 
-    pub fn dry(mut self, dry: bool) -> Self {
+    pub const fn dry(mut self, dry: bool) -> Self {
         self.dry = dry;
         self
     }
@@ -163,7 +163,7 @@ pub struct Build {
 }
 
 impl Build {
-    pub fn new(installable: Installable) -> Self {
+    pub const fn new(installable: Installable) -> Self {
         Self {
             message: None,
             installable,
@@ -183,7 +183,7 @@ impl Build {
         self
     }
 
-    pub fn nom(mut self, yes: bool) -> Self {
+    pub const fn nom(mut self, yes: bool) -> Self {
         self.nom = yes;
         self
     }

src/completion.rs

@@ -2,8 +2,8 @@ use clap_complete::generate;
 use color_eyre::Result;
 use tracing::instrument;
 
+use crate::interface;
 use crate::interface::Main;
-use crate::*;
 
 impl interface::CompletionArgs {
     #[instrument(ret, level = "trace")]

src/darwin.rs

@@ -17,7 +17,7 @@ const CURRENT_PROFILE: &str = "/run/current-system";
 
 impl DarwinArgs {
     pub fn run(self) -> Result<()> {
-        use DarwinRebuildVariant::*;
+        use DarwinRebuildVariant::{Build, Switch};
         match self.subcommand {
             DarwinSubcommand::Switch(args) => args.rebuild(Switch),
             DarwinSubcommand::Build(args) => {
@@ -38,7 +38,7 @@ enum DarwinRebuildVariant {
 
 impl DarwinRebuildArgs {
     fn rebuild(self, variant: DarwinRebuildVariant) -> Result<()> {
-        use DarwinRebuildVariant::*;
+        use DarwinRebuildVariant::{Build, Switch};
 
         if nix::unistd::Uid::effective().is_root() {
             bail!("Don't run nh os as root. I will call sudo internally as needed");
@@ -121,7 +121,7 @@ impl DarwinRebuildArgs {
             }
         }
 
-        if let Switch = variant {
+        if matches!(variant, Switch) {
             Command::new("nix")
                 .args(["build", "--no-link", "--profile", SYSTEM_PROFILE])
                 .arg(out_path.get_path())

src/generations.rs

@@ -14,7 +14,7 @@ pub struct GenerationInfo {
     /// Date on switch a generation was built
     pub date: String,
 
-    /// NixOS version derived from `nixos-version`
+    /// `NixOS` version derived from `nixos-version`
     pub nixos_version: String,
 
     /// Version of the bootable kernel for a given generation
@@ -66,7 +66,7 @@ pub fn describe(generation_dir: &Path, current_profile: &Path) -> Option<Generat
         .join("kernel")
         .canonicalize()
         .ok()
-        .and_then(|path| path.parent().map(|p| p.to_path_buf()))
+        .and_then(|path| path.parent().map(std::path::Path::to_path_buf))
         .unwrap_or_else(|| PathBuf::from("Unknown"));
 
     let kernel_modules_dir = kernel_dir.join("lib/modules");
@@ -195,9 +195,10 @@ pub fn print_info(mut generations: Vec<GenerationInfo>) {
     // Parse all dates at once and cache them
     let mut parsed_dates = HashMap::with_capacity(generations.len());
     for gen in &generations {
-        let date = DateTime::parse_from_rfc3339(&gen.date)
-            .map(|dt| dt.with_timezone(&Local))
-            .unwrap_or_else(|_| Local.timestamp_opt(0, 0).unwrap());
+        let date = DateTime::parse_from_rfc3339(&gen.date).map_or_else(
+            |_| Local.timestamp_opt(0, 0).unwrap(),
+            |dt| dt.with_timezone(&Local),
+        );
         parsed_dates.insert(
             gen.date.clone(),
             date.format("%Y-%m-%d %H:%M:%S").to_string(),
@@ -216,7 +217,7 @@ pub fn print_info(mut generations: Vec<GenerationInfo>) {
         println!("Error getting current generation!");
     }
 
-    println!("Closure Size: {}", closure);
+    println!("Closure Size: {closure}");
     println!();
 
     // Determine column widths for pretty printing
@@ -256,7 +257,7 @@ pub fn print_info(mut generations: Vec<GenerationInfo>) {
             generation
                 .specialisations
                 .iter()
-                .map(|s| format!("*{}", s))
+                .map(|s| format!("*{s}"))
                 .collect::<Vec<String>>()
                 .join(" ")
         };