diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index d5890f40..57b9ba88 100644 --- a/.github/workflows/build.yaml +++ b/.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 diff --git a/.github/workflows/check.yaml b/.github/workflows/check.yaml new file mode 100644 index 00000000..1b0ef410 --- /dev/null +++ b/.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 + diff --git a/Cargo.toml b/Cargo.toml index 9c32c3d1..d5cefb02 100644 --- a/Cargo.toml +++ b/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" diff --git a/fix.sh b/fix.sh index 1e51fd33..207a921a 100755 --- a/fix.sh +++ b/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 diff --git a/src/clean.rs b/src/clean.rs index 5b2d2c2e..17c037da 100644 --- a/src/clean.rs +++ b/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!"); diff --git a/src/commands.rs b/src/commands.rs index 122730b5..94976b8f 100644 --- a/src/commands.rs +++ b/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 } diff --git a/src/completion.rs b/src/completion.rs index 486c143d..08de785a 100644 --- a/src/completion.rs +++ b/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")] diff --git a/src/darwin.rs b/src/darwin.rs index 79b9bdbd..fb1b4427 100644 --- a/src/darwin.rs +++ b/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()) diff --git a/src/generations.rs b/src/generations.rs index efcf513b..1e00f200 100644 --- a/src/generations.rs +++ b/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) { // 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) { 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) { generation .specialisations .iter() - .map(|s| format!("*{}", s)) + .map(|s| format!("*{s}")) .collect::>() .join(" ") }; diff --git a/src/home.rs b/src/home.rs index cb38ebb0..1817ea9b 100644 --- a/src/home.rs +++ b/src/home.rs @@ -15,7 +15,7 @@ use crate::util::get_hostname; impl interface::HomeArgs { pub fn run(self) -> Result<()> { - use HomeRebuildVariant::*; + use HomeRebuildVariant::{Build, Switch}; match self.subcommand { HomeSubcommand::Switch(args) => args.rebuild(Switch), HomeSubcommand::Build(args) => { @@ -37,7 +37,7 @@ enum HomeRebuildVariant { impl HomeRebuildArgs { fn rebuild(self, variant: HomeRebuildVariant) -> Result<()> { - use HomeRebuildVariant::*; + use HomeRebuildVariant::Build; if self.update_args.update { update(&self.common.installable, self.update_args.update_input)?; @@ -170,10 +170,10 @@ where I: IntoIterator, S: AsRef, { - let mut res = installable.clone(); + let mut res = installable; let extra_args: Vec = { let mut vec = Vec::new(); - for elem in extra_args.into_iter() { + for elem in extra_args { vec.push(elem.as_ref().to_owned()); } vec @@ -206,7 +206,7 @@ where // Check if an explicit configuration name was provided via the flag if let Some(config_name) = configuration_name { // Verify the provided configuration exists - let func = format!(r#" x: x ? "{}" "#, config_name); + let func = format!(r#" x: x ? "{config_name}" "#); let check_res = commands::Command::new("nix") .arg("eval") .args(&extra_args) @@ -228,29 +228,26 @@ where ) })?; - match check_res.map(|s| s.trim().to_owned()).as_deref() { - Some("true") => { - debug!("Using explicit configuration from flag: {}", config_name); - attribute.push(config_name.clone()); - if push_drv { - attribute.extend(toplevel.clone()); - } - found_config = true; - } - _ => { - // Explicit config provided but not found - let tried_attr_path = { - let mut attr_path = attribute.clone(); - attr_path.push(config_name.clone()); - Installable::Flake { - reference: flake_reference.clone(), - attribute: attr_path, - } - .to_args() - .join(" ") - }; - bail!("Explicitly specified home-manager configuration not found: {tried_attr_path}"); + if check_res.map(|s| s.trim().to_owned()).as_deref() == Some("true") { + debug!("Using explicit configuration from flag: {}", config_name); + attribute.push(config_name); + if push_drv { + attribute.extend(toplevel.clone()); } + found_config = true; + } else { + // Explicit config provided but not found + let tried_attr_path = { + let mut attr_path = attribute.clone(); + attr_path.push(config_name); + Installable::Flake { + reference: flake_reference, + attribute: attr_path, + } + .to_args() + .join(" ") + }; + bail!("Explicitly specified home-manager configuration not found: {tried_attr_path}"); } } @@ -260,8 +257,8 @@ where let hostname = get_hostname()?; let mut tried = vec![]; - for attr_name in [format!("{username}@{hostname}"), username.to_string()] { - let func = format!(r#" x: x ? "{}" "#, attr_name); + for attr_name in [format!("{username}@{hostname}"), username] { + let func = format!(r#" x: x ? "{attr_name}" "#); let check_res = commands::Command::new("nix") .arg("eval") .args(&extra_args) @@ -293,7 +290,7 @@ where match check_res.map(|s| s.trim().to_owned()).as_deref() { Some("true") => { debug!("Using automatically detected configuration: {}", attr_name); - attribute.push(attr_name.clone()); + attribute.push(attr_name); if push_drv { attribute.extend(toplevel.clone()); } diff --git a/src/installable.rs b/src/installable.rs index 351b0ea5..e8d59e2a 100644 --- a/src/installable.rs +++ b/src/installable.rs @@ -66,7 +66,12 @@ impl FromArgMatches for Installable { let reference = elems.next().unwrap().to_owned(); return Ok(Self::Flake { reference, - attribute: parse_attribute(elems.next().map(|s| s.to_string()).unwrap_or_default()), + attribute: parse_attribute( + elems + .next() + .map(std::string::ToString::to_string) + .unwrap_or_default(), + ), }); } @@ -80,7 +85,10 @@ impl FromArgMatches for Installable { return Ok(Self::Flake { reference: elems.next().unwrap().to_owned(), attribute: parse_attribute( - elems.next().map(|s| s.to_string()).unwrap_or_default(), + elems + .next() + .map(std::string::ToString::to_string) + .unwrap_or_default(), ), }); } @@ -90,7 +98,10 @@ impl FromArgMatches for Installable { return Ok(Self::Flake { reference: elems.next().unwrap().to_owned(), attribute: parse_attribute( - elems.next().map(|s| s.to_string()).unwrap_or_default(), + elems + .next() + .map(std::string::ToString::to_string) + .unwrap_or_default(), ), }); } @@ -100,7 +111,10 @@ impl FromArgMatches for Installable { return Ok(Self::Flake { reference: elems.next().unwrap().to_owned(), attribute: parse_attribute( - elems.next().map(|s| s.to_string()).unwrap_or_default(), + elems + .next() + .map(std::string::ToString::to_string) + .unwrap_or_default(), ), }); } @@ -111,7 +125,12 @@ impl FromArgMatches for Installable { let mut elems = f.splitn(2, '#'); return Ok(Self::Flake { reference: elems.next().unwrap().to_owned(), - attribute: parse_attribute(elems.next().map(|s| s.to_string()).unwrap_or_default()), + attribute: parse_attribute( + elems + .next() + .map(std::string::ToString::to_string) + .unwrap_or_default(), + ), }); } @@ -119,7 +138,12 @@ impl FromArgMatches for Installable { let mut elems = f.splitn(2, '#'); return Ok(Self::Flake { reference: elems.next().unwrap().to_owned(), - attribute: parse_attribute(elems.next().map(|s| s.to_string()).unwrap_or_default()), + attribute: parse_attribute( + elems + .next() + .map(std::string::ToString::to_string) + .unwrap_or_default(), + ), }); } @@ -127,7 +151,12 @@ impl FromArgMatches for Installable { let mut elems = f.splitn(2, '#'); return Ok(Self::Flake { reference: elems.next().unwrap().to_owned(), - attribute: parse_attribute(elems.next().map(|s| s.to_string()).unwrap_or_default()), + attribute: parse_attribute( + elems + .next() + .map(std::string::ToString::to_string) + .unwrap_or_default(), + ), }); } @@ -135,7 +164,12 @@ impl FromArgMatches for Installable { let mut elems = f.splitn(2, '#'); return Ok(Self::Flake { reference: elems.next().unwrap().to_owned(), - attribute: parse_attribute(elems.next().map(|s| s.to_string()).unwrap_or_default()), + attribute: parse_attribute( + elems + .next() + .map(std::string::ToString::to_string) + .unwrap_or_default(), + ), }); } @@ -252,37 +286,35 @@ where res.push(elem); - if in_quote { - panic!("Failed to parse attribute: {}", s); - } + assert!(!in_quote, "Failed to parse attribute: {s}"); res } #[test] fn test_parse_attribute() { - assert_eq!(parse_attribute(r#"foo.bar"#), vec!["foo", "bar"]); + assert_eq!(parse_attribute(r"foo.bar"), vec!["foo", "bar"]); assert_eq!(parse_attribute(r#"foo."bar.baz""#), vec!["foo", "bar.baz"]); let v: Vec = vec![]; - assert_eq!(parse_attribute(""), v) + assert_eq!(parse_attribute(""), v); } impl Installable { pub fn to_args(&self) -> Vec { let mut res = Vec::new(); match self { - Installable::Flake { + Self::Flake { reference, attribute, } => { res.push(format!("{reference}#{}", join_attribute(attribute))); } - Installable::File { path, attribute } => { + Self::File { path, attribute } => { res.push(String::from("--file")); res.push(path.to_str().unwrap().to_string()); res.push(join_attribute(attribute)); } - Installable::Expression { + Self::Expression { expression, attribute, } => { @@ -290,7 +322,7 @@ impl Installable { res.push(expression.to_string()); res.push(join_attribute(attribute)); } - Installable::Store { path } => res.push(path.to_str().unwrap().to_string()), + Self::Store { path } => res.push(path.to_str().unwrap().to_string()), } res @@ -335,7 +367,7 @@ where let s = elem.as_ref(); if s.contains('.') { - res.push_str(&format!(r#""{}""#, s)); + res.push_str(&format!(r#""{s}""#)); } else { res.push_str(s); } @@ -351,12 +383,12 @@ fn test_join_attribute() { } impl Installable { - pub fn str_kind(&self) -> &str { + pub const fn str_kind(&self) -> &str { match self { - Installable::Flake { .. } => "flake", - Installable::File { .. } => "file", - Installable::Store { .. } => "store path", - Installable::Expression { .. } => "expression", + Self::Flake { .. } => "flake", + Self::File { .. } => "file", + Self::Store { .. } => "store path", + Self::Expression { .. } => "expression", } } } diff --git a/src/interface.rs b/src/interface.rs index 92c664dc..ea79d8f0 100644 --- a/src/interface.rs +++ b/src/interface.rs @@ -107,7 +107,7 @@ pub enum OsSubcommand { /// Rollback to a previous generation Rollback(OsRollbackArgs), - /// Build a NixOS VM image + /// Build a `NixOS` VM image BuildVm(OsBuildVmArgs), } diff --git a/src/json.rs b/src/json.rs index 8a52e9ea..60b109a3 100644 --- a/src/json.rs +++ b/src/json.rs @@ -19,7 +19,7 @@ impl Display for Error { if i != 0 { write!(f, ".")?; } - write!(f, "{}", elem)?; + write!(f, "{elem}")?; } Ok(()) @@ -29,7 +29,7 @@ impl Display for Error { impl std::error::Error for Error {} impl<'v> Value<'v> { - pub fn new(value: &'v serde_json::Value) -> Self { + pub const fn new(value: &'v serde_json::Value) -> Self { Self { inner: value, get_stack: vec![], diff --git a/src/logging.rs b/src/logging.rs index 05cf732f..bad8e4aa 100644 --- a/src/logging.rs +++ b/src/logging.rs @@ -11,7 +11,7 @@ use tracing_subscriber::prelude::*; use tracing_subscriber::registry::LookupSpan; use tracing_subscriber::EnvFilter; -use crate::*; +use crate::Result; struct InfoFormatter; @@ -43,7 +43,7 @@ where if *level != Level::INFO { if let (Some(file), Some(line)) = (metadata.file(), metadata.line()) { - write!(writer, " (nh/{}:{})", file, line)?; + write!(writer, " (nh/{file}:{line})")?; } } @@ -52,7 +52,7 @@ where } } -pub(crate) fn setup_logging(verbose: bool) -> Result<()> { +pub fn setup_logging(verbose: bool) -> Result<()> { color_eyre::config::HookBuilder::default() .display_location_section(true) .panic_section("Please report the bug at https://github.com/nix-community/nh/issues") diff --git a/src/nixos.rs b/src/nixos.rs index 7db44659..775828d7 100644 --- a/src/nixos.rs +++ b/src/nixos.rs @@ -25,7 +25,7 @@ const SPEC_LOCATION: &str = "/etc/specialisation"; impl interface::OsArgs { pub fn run(self) -> Result<()> { - use OsRebuildVariant::*; + use OsRebuildVariant::{Boot, Build, Switch, Test}; match self.subcommand { OsSubcommand::Boot(args) => args.rebuild(Boot, None), OsSubcommand::Test(args) => args.rebuild(Test, None), @@ -64,7 +64,7 @@ impl OsBuildVmArgs { impl OsRebuildArgs { // final_attr is the attribute of config.system.build.X to evaluate. fn rebuild(self, variant: OsRebuildVariant, final_attr: Option) -> Result<()> { - use OsRebuildVariant::*; + use OsRebuildVariant::{Boot, Build, BuildVm, Switch, Test}; if self.build_host.is_some() || self.target_host.is_some() { // if it fails its okay @@ -158,7 +158,7 @@ impl OsRebuildArgs { let target_specialisation = if self.no_specialisation { None } else { - current_specialisation.or_else(|| self.specialisation.to_owned()) + current_specialisation.or_else(|| self.specialisation.clone()) }; debug!("target_specialisation: {target_specialisation:?}"); @@ -183,7 +183,7 @@ impl OsRebuildArgs { .message("Comparing changes") .run()?; } else { - debug!("Not running nvd as the target hostname is different from the system hostname.") + debug!("Not running nvd as the target hostname is different from the system hostname."); } if self.common.dry || matches!(variant, Build | BuildVm) { @@ -207,7 +207,7 @@ impl OsRebuildArgs { .args([ "copy", "--to", - format!("ssh://{}", target_host).as_str(), + format!("ssh://{target_host}").as_str(), target_profile.to_str().unwrap(), ]) .message("Copying configuration to target") @@ -247,7 +247,7 @@ impl OsRebuildArgs { Command::new(switch_to_configuration) .arg("boot") - .ssh(self.target_host.clone()) + .ssh(self.target_host) .elevate(elevate) .message("Adding configuration to bootloader") .run()?; @@ -376,7 +376,7 @@ impl OsRollbackArgs { .elevate(elevate) .run() { - Ok(_) => { + Ok(()) => { info!( "Successfully rolled back to generation {}", target_generation.number @@ -388,7 +388,7 @@ impl OsRollbackArgs { // If activation fails, rollback the profile if rollback_profile && current_gen_number > 0 { let current_gen_link = - profile_dir.join(format!("system-{}-link", current_gen_number)); + profile_dir.join(format!("system-{current_gen_number}-link")); Command::new("ln") .arg("-sfn") // Force, symbolic link @@ -526,7 +526,7 @@ pub fn toplevel_for>( installable: Installable, final_attr: &str, ) -> Installable { - let mut res = installable.clone(); + let mut res = installable; let hostname = hostname.as_ref().to_owned(); let toplevel = ["config", "system", "build", final_attr] diff --git a/src/update.rs b/src/update.rs index cf471b11..7b8e5bb4 100644 --- a/src/update.rs +++ b/src/update.rs @@ -10,7 +10,7 @@ pub fn update(installable: &Installable, input: Option) -> Result<()> { let mut cmd = Command::new("nix").args(["flake", "update"]); if let Some(i) = input { - cmd = cmd.arg(&i).message(format!("Updating flake input {}", i)); + cmd = cmd.arg(&i).message(format!("Updating flake input {i}")); } else { cmd = cmd.message("Updating all flake inputs"); }