diff --git a/Cargo.lock b/Cargo.lock index 63818b5..a3725f0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -146,6 +146,49 @@ version = "3.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7704b5fdd17b18ae31c4c1da5a2e0305a2bf17b5249300a9ee9ed7b72114c636" +[[package]] +name = "derive-enum-from-into" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "42e5ddace13a8459cb452b19e01f59f16d3e2049c8b808f338a13eeadc326e33" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + +[[package]] +name = "derive-new" +version = "0.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2cdc8d50f426189eef89dac62fabfa0abb27d5cc008f25bf4156a0203325becc" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + +[[package]] +name = "derive_more" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4a9b99b9cbbe49445b21764dc0625032a89b145a2642e67603e1c936f5458d05" +dependencies = [ + "derive_more-impl", +] + +[[package]] +name = "derive_more-impl" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cb7330aeadfbe296029522e6c40f315320aba36fc43a5b3632f3795348f3bd22" +dependencies = [ + "proc-macro2", + "quote", + "syn", + "unicode-xid", +] + [[package]] name = "diff" version = "0.1.13" @@ -254,6 +297,9 @@ dependencies = [ "anyhow", "clap", "colored", + "derive-enum-from-into", + "derive-new", + "derive_more", "indoc", "itertools", "lazy_static", @@ -540,6 +586,12 @@ version = "0.1.14" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7dd6e30e90baa6f72411720665d41d89b9a3d039dc45b8faea1ddd07f617f6af" +[[package]] +name = "unicode-xid" +version = "0.2.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "229730647fbc343e3a80e463c1db7f78f3855d3f3739bee0dda773c9a037c90a" + [[package]] name = "utf8parse" version = "0.2.2" diff --git a/Cargo.toml b/Cargo.toml index 4e698f7..d186d40 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -18,6 +18,9 @@ rowan = "0.15.16" indoc = "2.0.5" relative-path = "1.9.3" textwrap = "0.16.1" +derive-enum-from-into = "0.2.0" +derive-new = "0.7.0" +derive_more = { version = "1.0.0", features = ["display"] } [dev-dependencies] pretty_assertions = "1.4.1" diff --git a/src/eval.rs b/src/eval.rs index db9152e..28d4cb6 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -1,23 +1,22 @@ -use crate::nix_file::CallPackageArgumentInfo; -use crate::nixpkgs_problem::{ - ByNameError, ByNameErrorKind, ByNameOverrideError, ByNameOverrideErrorKind, NixEvalError, - NixpkgsProblem, -}; -use crate::ratchet::RatchetState::{Loose, Tight}; -use crate::ratchet::{self, ManualDefinition, RatchetState}; -use crate::structure; -use crate::utils; -use crate::validation::ResultIteratorExt as _; -use crate::validation::{self, Validation::Success}; -use crate::NixFileStore; -use relative_path::RelativePathBuf; use std::path::{Path, PathBuf}; use std::{env, fs, process}; use anyhow::Context; +use relative_path::RelativePathBuf; use serde::Deserialize; use tempfile::Builder; +use crate::nix_file::CallPackageArgumentInfo; +use crate::problem::{ + npv_100, npv_101, npv_102, npv_103, npv_104, npv_105, npv_106, npv_107, npv_108, npv_120, +}; +use crate::ratchet::RatchetState::{Loose, Tight}; +use crate::structure::{self, BASE_SUBPATH}; +use crate::validation::ResultIteratorExt as _; +use crate::validation::{self, Validation::Success}; +use crate::NixFileStore; +use crate::{location, ratchet}; + const EVAL_NIX: &[u8] = include_bytes!("eval.nix"); /// Attribute set of this structure is returned by `./eval.nix` @@ -59,16 +58,18 @@ struct Location { } impl Location { - /// Returns the [file] field, but relative to Nixpkgs. - fn relative_file(&self, nixpkgs_path: &Path) -> anyhow::Result { - let path = self.file.strip_prefix(nixpkgs_path).with_context(|| { + /// Returns the location, but relative to the given Nixpkgs root. + fn relative(self, nixpkgs_path: &Path) -> anyhow::Result { + let Self { file, line, column } = self; + let path = file.strip_prefix(nixpkgs_path).with_context(|| { format!( "The file ({}) is outside Nixpkgs ({})", - self.file.display(), + file.display(), nixpkgs_path.display() ) })?; - Ok(RelativePathBuf::from_path(path).expect("relative path")) + let relative_file = RelativePathBuf::from_path(path).expect("relative path"); + Ok(location::Location::new(relative_file, line, column)) } } @@ -219,8 +220,7 @@ pub fn check_values( if !result.status.success() { // Early return in case evaluation fails - let stderr = String::from_utf8_lossy(&result.stderr).to_string(); - return Ok(NixpkgsProblem::NixEval(NixEvalError { stderr }).into()); + return Ok(npv_120::NixEvalError::new(String::from_utf8_lossy(&result.stderr)).into()); } // Parse the resulting JSON value @@ -268,29 +268,18 @@ fn by_name( attribute_name: &str, by_name_attribute: ByNameAttribute, ) -> validation::Result { - use ratchet::RatchetState::*; - use ByNameAttribute::*; - - let to_validation = |kind| -> validation::Validation> { - NixpkgsProblem::ByName(ByNameError { - attribute_name: attribute_name.to_owned(), - kind, - }) - .into() - }; - // At this point we know that `pkgs/by-name/fo/foo/package.nix` has to exists. This match // decides whether the attribute `foo` is defined accordingly and whether a legacy manual // definition could be removed. let manual_definition_result = match by_name_attribute { // The attribute is missing. - Missing => { + ByNameAttribute::Missing => { // This indicates a bug in the `pkgs/by-name` overlay, because it's supposed to // automatically defined attributes in `pkgs/by-name` - to_validation(ByNameErrorKind::UndefinedAttr) + npv_100::ByNameUndefinedAttribute::new(attribute_name).into() } // The attribute exists - Existing(AttributeInfo { + ByNameAttribute::Existing(AttributeInfo { // But it's not an attribute set, which limits the amount of information we can get // about this attribute (see ./eval.nix) attribute_variant: AttributeVariant::NonAttributeSet, @@ -301,10 +290,10 @@ fn by_name( // // We can't know whether the attribute is automatically or manually defined for sure, // and while we could check the location, the error seems clear enough as is. - to_validation(ByNameErrorKind::NonDerivation) + npv_101::ByNameNonDerivation::new(attribute_name).into() } // The attribute exists - Existing(AttributeInfo { + ByNameAttribute::Existing(AttributeInfo { // And it's an attribute set, which allows us to get more information about it attribute_variant: AttributeVariant::AttributeSet { @@ -317,7 +306,7 @@ fn by_name( let is_derivation_result = if is_derivation { Success(()) } else { - to_validation(ByNameErrorKind::NonDerivation).map(|_| ()) + npv_101::ByNameNonDerivation::new(attribute_name).into() }; // If the definition looks correct @@ -330,7 +319,7 @@ fn by_name( // Such an automatic definition should definitely not have a location. // Having one indicates that somebody is using // `_internalCallByNamePackageFile`, - to_validation(ByNameErrorKind::InternalCallPackageUsed) + npv_102::ByNameInternalCallPackageUsed::new(attribute_name).into() } else { Success(Tight) } @@ -348,13 +337,11 @@ fn by_name( let nix_file = nix_file_store.get(&location.file)?; // The relative path of the Nix file, for error messages - let relative_location_file = - location.relative_file(nixpkgs_path).with_context(|| { - format!( - "Failed to resolve the file where attribute {} is defined", - attribute_name - ) - })?; + let location = location.relative(nixpkgs_path).with_context(|| { + format!( + "Failed to resolve the file where attribute {attribute_name} is defined" + ) + })?; // Figure out whether it's an attribute definition of the form // `= callPackage `, returning the arguments if so. @@ -377,13 +364,12 @@ fn by_name( optional_syntactic_call_package, definition, location, - relative_location_file, ) } else { // If manual definitions don't have a location, it's likely `mapAttrs`'d // over, e.g. if it's defined in aliases.nix. // We can't verify whether its of the expected `callPackage`, so error out. - to_validation(ByNameErrorKind::CannotDetermineAttributeLocation) + npv_103::ByNameCannotDetermineAttributeLocation::new(attribute_name).into() } } }; @@ -411,58 +397,53 @@ fn by_name_override( is_semantic_call_package: bool, optional_syntactic_call_package: Option, definition: String, - location: Location, - relative_location_file: RelativePathBuf, + location: location::Location, ) -> validation::Validation> { - let expected_package_path = structure::relative_file_for_package(attribute_name); - - let to_problem = |kind| { - NixpkgsProblem::ByNameOverride(ByNameOverrideError { - package_name: attribute_name.to_owned(), - expected_package_path: expected_package_path.to_owned(), - file: relative_location_file, - line: location.line, - column: location.column, + let Some(syntactic_call_package) = optional_syntactic_call_package else { + // Something like ` = foo` + return npv_104::ByNameOverrideOfNonSyntacticCallPackage::new( + attribute_name, + location, definition, - kind, - }) + ) + .into(); }; - // At this point, we completed two different checks for whether it's a `callPackage` - match (is_semantic_call_package, optional_syntactic_call_package) { - // Something like ` = foo` - (_, None) => to_problem(ByNameOverrideErrorKind::NonSyntacticCallPackage).into(), - + if !is_semantic_call_package { // Something like ` = pythonPackages.callPackage ...` - (false, Some(_)) => to_problem(ByNameOverrideErrorKind::NonToplevelCallPackage).into(), - - // Something like ` = pkgs.callPackage ...` - (true, Some(syntactic_call_package)) => { - if let Some(actual_package_path) = syntactic_call_package.relative_path { - if actual_package_path != expected_package_path { - // Wrong path - to_problem(ByNameOverrideErrorKind::WrongCallPackagePath { - actual_path: actual_package_path, - }) - .into() - } else { - // Manual definitions with empty arguments are not allowed anymore, - // but existing ones should continue to be allowed. - let manual_definition_ratchet = if syntactic_call_package.empty_arg { - // This is the state to migrate away from. - Loose(to_problem(ByNameOverrideErrorKind::EmptyArgument)) - } else { - // This is the state to migrate to. - Tight - }; + return npv_105::ByNameOverrideOfNonTopLevelPackage::new( + attribute_name, + location, + definition, + ) + .into(); + } - Success(manual_definition_ratchet) - } - } else { - // No path... - to_problem(ByNameOverrideErrorKind::NonPath).into() - } - } + let Some(actual_package_path) = syntactic_call_package.relative_path else { + return npv_108::ByNameOverrideContainsEmptyPath::new(attribute_name, location, definition) + .into(); + }; + + let expected_package_path = structure::relative_file_for_package(attribute_name); + if actual_package_path != expected_package_path { + return npv_106::ByNameOverrideContainsWrongCallPackagePath::new( + attribute_name, + actual_package_path, + location, + ) + .into(); + } + + // Manual definitions with empty arguments are not allowed anymore, but existing ones should + // continue to be allowed. This is the state to migrate away from. + if syntactic_call_package.empty_arg { + Success(Loose( + npv_107::ByNameOverrideContainsEmptyArgument::new(attribute_name, location, definition) + .into(), + )) + } else { + // This is the state to migrate to. + Success(Tight) } } @@ -530,8 +511,8 @@ fn handle_non_by_name_attribute( // Parse the Nix file in the location let nix_file = nix_file_store.get(&location.file)?; - // The relative path of the Nix file, for error messages - let relative_location_file = location.relative_file(nixpkgs_path).with_context(|| { + // The relative location of the Nix file, for error messages + let location = location.relative(nixpkgs_path).with_context(|| { format!("Failed to resolve the file where attribute {attribute_name} is defined") })?; @@ -567,7 +548,7 @@ fn handle_non_by_name_attribute( (true, Some(syntactic_call_package)) => { // It's only possible to migrate such a definitions if.. match syntactic_call_package.relative_path { - Some(ref rel_path) if rel_path.starts_with(utils::BASE_SUBPATH) => { + Some(ref rel_path) if rel_path.starts_with(BASE_SUBPATH) => { // ..the path is not already within `pkgs/by-name` like // // foo-variant = callPackage ../by-name/fo/foo/package.nix { @@ -585,7 +566,7 @@ fn handle_non_by_name_attribute( _ => { // Otherwise, the path is outside `pkgs/by-name`, which means it can be // migrated. - Loose((syntactic_call_package, relative_location_file)) + Loose((syntactic_call_package, location.file)) } } } diff --git a/src/utils.rs b/src/location.rs similarity index 77% rename from src/utils.rs rename to src/location.rs index 8645cc9..85257c7 100644 --- a/src/utils.rs +++ b/src/location.rs @@ -1,24 +1,21 @@ -use anyhow::Context; -use std::fs; -use std::io; -use std::path::Path; +use relative_path::RelativePathBuf; -pub const BASE_SUBPATH: &str = "pkgs/by-name"; -pub const PACKAGE_NIX_FILENAME: &str = "package.nix"; - -/// Deterministic file listing so that tests are reproducible. -pub fn read_dir_sorted(base_dir: &Path) -> anyhow::Result> { - let listing = base_dir - .read_dir() - .with_context(|| format!("Could not list directory {}", base_dir.display()))?; - - let mut shard_entries = listing - .collect::>>() - .with_context(|| format!("Could not list directory {}", base_dir.display()))?; - - shard_entries.sort_by_key(|entry| entry.file_name()); +/// A location that's suitable for error messages. +#[derive(Clone, Debug)] +pub struct Location { + pub file: RelativePathBuf, + pub line: usize, + pub column: usize, +} - Ok(shard_entries) +impl Location { + pub fn new(file: impl Into, line: usize, column: usize) -> Self { + Self { + file: file.into(), + line, + column, + } + } } /// A simple utility for calculating the line for a string offset. diff --git a/src/main.rs b/src/main.rs index c1b3fe9..f2ea1b0 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,11 +1,11 @@ mod eval; +mod location; mod nix_file; -mod nixpkgs_problem; +mod problem; mod ratchet; mod references; mod status; mod structure; -mod utils; mod validation; use anyhow::Context as _; @@ -102,7 +102,7 @@ fn check_nixpkgs(nixpkgs_path: &Path) -> validation::Result { ) })?; - if !nixpkgs_path.join(utils::BASE_SUBPATH).exists() { + if !nixpkgs_path.join(structure::BASE_SUBPATH).exists() { // No pkgs/by-name directory, always valid return Ok(Success(ratchet::Nixpkgs::default())); } @@ -120,12 +120,14 @@ fn check_nixpkgs(nixpkgs_path: &Path) -> validation::Result { #[cfg(test)] mod tests { - use anyhow::Context; use std::fs; use std::path::Path; + + use anyhow::Context; + use pretty_assertions::StrComparison; use tempfile::{tempdir_in, TempDir}; - use super::{process, utils::BASE_SUBPATH}; + use super::{process, structure::BASE_SUBPATH}; #[test] fn tests_dir() -> anyhow::Result<()> { @@ -245,9 +247,8 @@ mod tests { if !expected_errors_regex.is_match(&actual_errors) { panic!( - "Failed test case {name}, expected these errors:\n=======\n{}\n=======\n\ - but got these:\n=======\n{}\n=======", - expected_errors, actual_errors + "Failed test case {name}: {}", + StrComparison::new(expected_errors, &actual_errors) ); } } diff --git a/src/nix_file.rs b/src/nix_file.rs index 6a5c665..2b5447c 100644 --- a/src/nix_file.rs +++ b/src/nix_file.rs @@ -1,6 +1,6 @@ //! This is a utility module for interacting with the syntax of Nix files -use crate::utils::LineIndex; +use crate::location::LineIndex; use anyhow::Context; use itertools::Either::{self, Left, Right}; use relative_path::RelativePathBuf; diff --git a/src/nixpkgs_problem.rs b/src/nixpkgs_problem.rs deleted file mode 100644 index f5046de..0000000 --- a/src/nixpkgs_problem.rs +++ /dev/null @@ -1,470 +0,0 @@ -use crate::structure; -use crate::utils::PACKAGE_NIX_FILENAME; -use indoc::writedoc; -use relative_path::RelativePath; -use relative_path::RelativePathBuf; -use std::ffi::OsString; -use std::fmt; - -/// Any problem that can occur when checking Nixpkgs -/// All paths are relative to Nixpkgs such that the error messages can't be influenced by Nixpkgs absolute -/// location -#[derive(Clone)] -pub enum NixpkgsProblem { - Shard(ShardError), - Package(PackageError), - ByName(ByNameError), - ByNameOverride(ByNameOverrideError), - Path(PathError), - NixFile(NixFileError), - TopLevelPackage(TopLevelPackageError), - NixEval(NixEvalError), -} - -/// A file structure error involving a shard (e.g. `fo` is the shard in the path `pkgs/by-name/fo/foo/package.nix`) -#[derive(Clone)] -pub struct ShardError { - pub shard_name: String, - pub kind: ShardErrorKind, -} - -#[derive(Clone)] -pub enum ShardErrorKind { - ShardNonDir, - InvalidShardName, - CaseSensitiveDuplicate { first: OsString, second: OsString }, -} - -/// A file structure error involving the package name and/or path. -#[derive(Clone)] -pub struct PackageError { - pub relative_package_dir: RelativePathBuf, - pub kind: PackageErrorKind, -} - -#[derive(Clone)] -pub enum PackageErrorKind { - PackageNonDir { - package_name: String, - }, - InvalidPackageName { - invalid_package_name: String, - }, - IncorrectShard { - correct_relative_package_dir: RelativePathBuf, - }, - PackageNixNonExistent, - PackageNixDir, -} - -/// An error related to checks involving by-name attributes. For example, attribute `foo` in -/// `pkgs/by-name/fo/foo/package.nix`. -#[derive(Clone)] -pub struct ByNameError { - pub attribute_name: String, - pub kind: ByNameErrorKind, -} - -#[derive(Clone)] -pub enum ByNameErrorKind { - UndefinedAttr, - NonDerivation, - InternalCallPackageUsed, - CannotDetermineAttributeLocation, -} - -/// An error related to packages in `pkgs/by-name` that are manually overridden, e.g. in -/// all-packages.nix -#[derive(Clone)] -pub struct ByNameOverrideError { - pub package_name: String, - pub expected_package_path: RelativePathBuf, - pub file: RelativePathBuf, - pub line: usize, - pub column: usize, - pub definition: String, - pub kind: ByNameOverrideErrorKind, -} - -#[derive(Clone)] -pub enum ByNameOverrideErrorKind { - NonSyntacticCallPackage, - NonToplevelCallPackage, - WrongCallPackagePath { actual_path: RelativePathBuf }, - EmptyArgument, - NonPath, -} - -/// An error that results from checks that verify a specific path does not reference outside the -/// package directory. -#[derive(Clone)] -pub struct PathError { - pub relative_package_dir: RelativePathBuf, - pub subpath: RelativePathBuf, - pub kind: PathErrorKind, -} - -#[derive(Clone)] -pub enum PathErrorKind { - OutsideSymlink, - UnresolvableSymlink { io_error: String }, -} - -/// An error that results from checks that verify a nix file that contains a path expression does -/// not reference outside the package. -#[derive(Clone)] -pub struct NixFileError { - pub relative_package_dir: RelativePathBuf, - pub subpath: RelativePathBuf, - pub line: usize, - pub text: String, - pub kind: NixFileErrorKind, -} - -#[derive(Clone)] -pub enum NixFileErrorKind { - PathInterpolation, - SearchPath, - OutsidePathReference, - UnresolvablePathReference { io_error: String }, -} - -/// An error related to the introduction/move of a top-level package not using `pkgs/by-name`, but -/// it should. -#[derive(Clone)] -pub struct TopLevelPackageError { - pub package_name: String, - pub call_package_path: Option, - pub file: RelativePathBuf, - pub is_new: bool, - pub is_empty: bool, -} - -/// A Nix evaluation error for some package in `pkgs/by-name` -#[derive(Clone)] -pub struct NixEvalError { - pub stderr: String, -} - -impl fmt::Display for NixpkgsProblem { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - match self { - NixpkgsProblem::Shard(ShardError { - shard_name, - kind, - }) => { - let relative_shard_path = structure::relative_dir_for_shard(shard_name); - match kind { - ShardErrorKind::ShardNonDir => - write!( - f, - "- {relative_shard_path}: This is a file, but it should be a directory.", - ), - ShardErrorKind::InvalidShardName => - write!( - f, - "- {relative_shard_path}: Invalid directory name \"{shard_name}\", must be at most 2 ASCII characters consisting of a-z, 0-9, \"-\" or \"_\".", - ), - ShardErrorKind::CaseSensitiveDuplicate { first, second } => - write!( - f, - "- {relative_shard_path}: Duplicate case-sensitive package directories {first:?} and {second:?}.", - ), - } - } - NixpkgsProblem::Package(PackageError { - relative_package_dir, - kind, - }) => { - match kind { - PackageErrorKind::PackageNonDir { package_name } => { - let relative_package_dir = structure::relative_dir_for_package(package_name); - write!( - f, - "- {relative_package_dir}: This path is a file, but it should be a directory.", - ) - } - PackageErrorKind::InvalidPackageName { invalid_package_name } => - write!( - f, - "- {relative_package_dir}: Invalid package directory name \"{invalid_package_name}\", must be ASCII characters consisting of a-z, A-Z, 0-9, \"-\" or \"_\".", - ), - PackageErrorKind::IncorrectShard { correct_relative_package_dir } => - write!( - f, - "- {relative_package_dir}: Incorrect directory location, should be {correct_relative_package_dir} instead.", - ), - PackageErrorKind::PackageNixNonExistent => - write!( - f, - "- {relative_package_dir}: Missing required \"{PACKAGE_NIX_FILENAME}\" file.", - ), - PackageErrorKind::PackageNixDir => - write!( - f, - "- {relative_package_dir}: \"{PACKAGE_NIX_FILENAME}\" must be a file.", - ), - } - } - NixpkgsProblem::ByName(ByNameError { - attribute_name, - kind, - }) => { - match kind { - ByNameErrorKind::UndefinedAttr => { - let relative_package_file = structure::relative_file_for_package(attribute_name); - write!( - f, - "- pkgs.{attribute_name}: This attribute is not defined but it should be defined automatically as {relative_package_file}", - ) - } - ByNameErrorKind::NonDerivation => { - let relative_package_file = structure::relative_file_for_package(attribute_name); - write!( - f, - "- pkgs.{attribute_name}: This attribute defined by {relative_package_file} is not a derivation", - ) - } - ByNameErrorKind::InternalCallPackageUsed => - write!( - f, - "- pkgs.{attribute_name}: This attribute is defined using `_internalCallByNamePackageFile`, which is an internal function not intended for manual use.", - ), - ByNameErrorKind::CannotDetermineAttributeLocation => - write!( - f, - "- pkgs.{attribute_name}: Cannot determine the location of this attribute using `builtins.unsafeGetAttrPos`.", - ), - } - } - NixpkgsProblem::ByNameOverride(ByNameOverrideError { - package_name, - expected_package_path, - file, - line, - column, - definition, - kind, - }) => { - let relative_package_dir = structure::relative_dir_for_package(package_name); - let expected_path_expr = create_path_expr(file, expected_package_path); - let indented_definition = indent_definition(*column, definition.clone()); - - match kind { - ByNameOverrideErrorKind::NonSyntacticCallPackage => { - - writedoc!( - f, - " - - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like - - {package_name} = callPackage {expected_path_expr} {{ /* ... */ }}; - - However, in this PR, it isn't defined that way. See the definition in {file}:{line} - - {indented_definition} - ", - ) - } - ByNameOverrideErrorKind::NonToplevelCallPackage => - writedoc!( - f, - " - - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like - - {package_name} = callPackage {expected_path_expr} {{ /* ... */ }}; - - However, in this PR, a different `callPackage` is used. See the definition in {file}:{line}: - - {indented_definition} - ", - ), - ByNameOverrideErrorKind::WrongCallPackagePath { actual_path } => { - let actual_path_expr = create_path_expr(file, actual_path); - writedoc!( - f, - " - - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like - - {package_name} = callPackage {expected_path_expr} {{ /* ... */ }}; - - However, in this PR, the first `callPackage` argument is the wrong path. See the definition in {file}:{line}: - - {package_name} = callPackage {actual_path_expr} {{ /* ... */ }}; - ", - ) - } - ByNameOverrideErrorKind::EmptyArgument => - writedoc!( - f, - " - - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like - - {package_name} = callPackage {expected_path_expr} {{ /* ... */ }}; - - However, in this PR, the second argument is empty. See the definition in {file}:{line}: - - {indented_definition} - - Such a definition is provided automatically and therefore not necessary. Please remove it. - ", - ), - ByNameOverrideErrorKind::NonPath => - writedoc!( - f, - " - - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like - - {package_name} = callPackage {expected_path_expr} {{ /* ... */ }}; - - However, in this PR, the first `callPackage` argument is not a path. See the definition in {file}:{line}: - - {indented_definition} - ", - ), - } - }, - NixpkgsProblem::Path(PathError { - relative_package_dir, - subpath, - kind, - }) => { - match kind { - PathErrorKind::OutsideSymlink => - write!( - f, - "- {relative_package_dir}: Path {subpath} is a symlink pointing to a path outside the directory of that package.", - ), - PathErrorKind::UnresolvableSymlink { io_error } => - write!( - f, - "- {relative_package_dir}: Path {subpath} is a symlink which cannot be resolved: {io_error}.", - ), - } - }, - NixpkgsProblem::NixFile(NixFileError { - relative_package_dir, - subpath, - line, - text, - kind - }) => { - match kind { - NixFileErrorKind::PathInterpolation => - write!( - f, - "- {relative_package_dir}: File {subpath} at line {line} contains the path expression \"{text}\", which is not yet supported and may point outside the directory of that package.", - ), - NixFileErrorKind::SearchPath => - write!( - f, - "- {relative_package_dir}: File {subpath} at line {line} contains the nix search path expression \"{text}\" which may point outside the directory of that package.", - ), - NixFileErrorKind::OutsidePathReference => - writedoc!( - f, - " - - {relative_package_dir}: File {subpath} at line {line} contains the path expression \"{text}\" which may point outside the directory of that package. - This is undesirable because it creates dependencies between internal paths, making it harder to reorganise Nixpkgs in the future. - Alternatives include: - - If you are creating a new version of a package with a common file between versions, consider following the recommendation in https://github.com/NixOS/nixpkgs/tree/master/pkgs/by-name#recommendation-for-new-packages-with-multiple-versions. - - If the path being referenced could be considered a stable interface with multiple uses, consider exposing it via a `pkgs` attribute, then taking it as a attribute argument in {PACKAGE_NIX_FILENAME}. - - If the path being referenced is internal and has multiple uses, consider passing the file as an explicit `callPackage` argument in `pkgs/top-level/all-packages.nix`. - - If the path being referenced is internal and will need to be modified independently of the original, consider copying it into the {relative_package_dir} directory. - " - ), - NixFileErrorKind::UnresolvablePathReference { io_error } => - write!( - f, - "- {relative_package_dir}: File {subpath} at line {line} contains the path expression \"{text}\" which cannot be resolved: {io_error}.", - ), - } - }, - NixpkgsProblem::TopLevelPackage(TopLevelPackageError { - package_name, - call_package_path, - file, - is_new, - is_empty, - }) => { - let call_package_arg = - if let Some(path) = &call_package_path { - format!("./{}", path) - } else { - "...".into() - }; - let relative_package_file = structure::relative_file_for_package(package_name); - - match (is_new, is_empty) { - (false, true) => - writedoc!( - f, - " - - Attribute `pkgs.{package_name}` was previously defined in {relative_package_file}, but is now manually defined as `callPackage {call_package_arg} {{ /* ... */ }}` in {file}. - Please move the package back and remove the manual `callPackage`. - ", - ), - (false, false) => - // This can happen if users mistakenly assume that for custom arguments, - // pkgs/by-name can't be used. - writedoc!( - f, - " - - Attribute `pkgs.{package_name}` was previously defined in {relative_package_file}, but is now manually defined as `callPackage {call_package_arg} {{ ... }}` in {file}. - While the manual `callPackage` is still needed, it's not necessary to move the package files. - ", - ), - (true, true) => - writedoc!( - f, - " - - Attribute `pkgs.{package_name}` is a new top-level package using `pkgs.callPackage {call_package_arg} {{ /* ... */ }}`. - Please define it in {relative_package_file} instead. - See `pkgs/by-name/README.md` for more details. - Since the second `callPackage` argument is `{{ }}`, no manual `callPackage` in {file} is needed anymore. - ", - ), - (true, false) => - writedoc!( - f, - " - - Attribute `pkgs.{package_name}` is a new top-level package using `pkgs.callPackage {call_package_arg} {{ /* ... */ }}`. - Please define it in {relative_package_file} instead. - See `pkgs/by-name/README.md` for more details. - Since the second `callPackage` argument is not `{{ }}`, the manual `callPackage` in {file} is still needed. - ", - ), - } - }, - NixpkgsProblem::NixEval(NixEvalError { stderr }) => { - f.write_str(stderr)?; - write!(f, "- Nix evaluation failed for some package in `pkgs/by-name`, see error above") - }, - } - } -} - -fn indent_definition(column: usize, definition: String) -> String { - // The entire code should be indented 4 spaces - textwrap::indent( - // But first we want to strip the code's natural indentation - &textwrap::dedent( - // The definition _doesn't_ include the leading spaces, but we can - // recover those from the column - &format!("{}{definition}", " ".repeat(column - 1)), - ), - " ", - ) -} - -/// Creates a Nix path expression that when put into Nix file `from_file`, would point to the `to_file`. -fn create_path_expr( - from_file: impl AsRef, - to_file: impl AsRef, -) -> String { - // This `expect` calls should never trigger because we only call this function with files. - // That's why we `expect` them! - let from = from_file.as_ref().parent().expect("a parent for this path"); - let rel = from.relative(to_file); - format!("./{rel}") -} diff --git a/src/problem/mod.rs b/src/problem/mod.rs new file mode 100644 index 0000000..981c0ea --- /dev/null +++ b/src/problem/mod.rs @@ -0,0 +1,151 @@ +use derive_enum_from_into::EnumFrom; +use derive_more::Display; +use relative_path::RelativePath; + +pub mod npv_100; +pub mod npv_101; +pub mod npv_102; +pub mod npv_103; +pub mod npv_104; +pub mod npv_105; +pub mod npv_106; +pub mod npv_107; +pub mod npv_108; +pub mod npv_109; +pub mod npv_110; +pub mod npv_111; + +pub mod npv_120; +pub mod npv_121; +pub mod npv_122; +pub mod npv_123; +pub mod npv_124; +pub mod npv_125; +pub mod npv_126; + +pub mod npv_140; +pub mod npv_141; +pub mod npv_142; +pub mod npv_143; +pub mod npv_144; + +pub mod npv_160; +pub mod npv_161; +pub mod npv_162; +pub mod npv_163; + +#[derive(Clone, Display, EnumFrom)] +pub enum Problem { + /// NPV-100: attribute is not defined but it should be defined automatically + ByNameUndefinedAttribute(npv_100::ByNameUndefinedAttribute), + + /// NPV-101: attribute is not a derivation + ByNameNonDerivation(npv_101::ByNameNonDerivation), + + /// NPV-102: attribute uses `_internalCallByNamePackageFile` + ByNameInternalCallPackageUsed(npv_102::ByNameInternalCallPackageUsed), + + /// NPV-103: attribute name position cannot be determined + ByNameCannotDetermineAttributeLocation(npv_103::ByNameCannotDetermineAttributeLocation), + + /// NPV-104: non-syntactic override of by-name package + ByNameOverrideOfNonSyntacticCallPackage(npv_104::ByNameOverrideOfNonSyntacticCallPackage), + + /// NPV-105: by-name override of ill-defined callPackage + ByNameOverrideOfNonTopLevelPackage(npv_105::ByNameOverrideOfNonTopLevelPackage), + + /// NPV-106: by-name override contains wrong callPackage path + ByNameOverrideContainsWrongCallPackagePath(npv_106::ByNameOverrideContainsWrongCallPackagePath), + + /// NPV-107: by-name override contains empty argument + ByNameOverrideContainsEmptyArgument(npv_107::ByNameOverrideContainsEmptyArgument), + + /// NPV-108: by-name override contains empty path + ByNameOverrideContainsEmptyPath(npv_108::ByNameOverrideContainsEmptyPath), + + /// NPV-109: by-name shard is not a directory + ByNameShardIsNotDirectory(npv_109::ByNameShardIsNotDirectory), + + /// NPV-110: by-name shard is invalid + ByNameShardIsInvalid(npv_110::ByNameShardIsInvalid), + + /// NPV-111: by-name shard is case-sensitive duplicate + ByNameShardIsCaseSensitiveDuplicate(npv_111::ByNameShardIsCaseSensitiveDuplicate), + + /// NPV-120: Nix evaluation failed + NixEvalError(npv_120::NixEvalError), + + /// NPV-121: Nix file contains interpolated path + NixFileContainsPathInterpolation(npv_121::NixFileContainsPathInterpolation), + + /// NPV-122: Nix file contains search path + NixFileContainsSearchPath(npv_122::NixFileContainsSearchPath), + + /// NPV-123: Nix file contains path expression outside of directory + NixFileContainsPathOutsideDirectory(npv_123::NixFileContainsPathOutsideDirectory), + + /// NPV-124: Nix file contains unresolvable path expression + NixFileContainsUnresolvablePath(npv_124::NixFileContainsUnresolvablePath), + + /// NPV-125: Package contains symlink pointing outside its directory + PackageContainsSymlinkPointingOutside(npv_125::PackageContainsSymlinkPointingOutside), + + /// NPV-126: Package contains unresolvable symlink + PackageContainsUnresolvableSymlink(npv_126::PackageContainsUnresolvableSymlink), + + /// NPV-140: Package directory is not directory + PackageDirectoryIsNotDirectory(npv_140::PackageDirectoryIsNotDirectory), + + /// NPV-141: Package name is not valid + InvalidPackageDirectoryName(npv_141::InvalidPackageDirectoryName), + + /// NPV-142: Package is in the wrong by-name shard + PackageInWrongShard(npv_142::PackageInWrongShard), + + /// NPV-143: `package.nix` is missing + PackageNixMissing(npv_143::PackageNixMissing), + + /// NPV-144: `package.nix` is not a file + PackageNixIsNotFile(npv_144::PackageNixIsNotFile), + + /// NPV-160: top-level package moved out of by-name + TopLevelPackageMovedOutOfByName(npv_160::TopLevelPackageMovedOutOfByName), + + /// NPV-161: top-level package moved out of by-name with custom arguments + TopLevelPackageMovedOutOfByNameWithCustomArguments( + npv_161::TopLevelPackageMovedOutOfByNameWithCustomArguments, + ), + + /// NPV-162: new top-level package should be in by-name + NewTopLevelPackageShouldBeByName(npv_162::NewTopLevelPackageShouldBeByName), + + /// NPV-163: new top-level package should be in by-name with a custom argument + NewTopLevelPackageShouldBeByNameWithCustomArgument( + npv_163::NewTopLevelPackageShouldBeByNameWithCustomArgument, + ), +} + +fn indent_definition(column: usize, definition: &str) -> String { + // The entire code should be indented 4 spaces + textwrap::indent( + // But first we want to strip the code's natural indentation + &textwrap::dedent( + // The definition _doesn't_ include the leading spaces, but we can + // recover those from the column + &format!("{}{definition}", " ".repeat(column - 1)), + ), + " ", + ) +} + +/// Creates a Nix path expression that when put into Nix file `from_file`, would point to the `to_file`. +fn create_path_expr( + from_file: impl AsRef, + to_file: impl AsRef, +) -> String { + // This `expect` calls should never trigger because we only call this function with files. + // That's why we `expect` them! + let from = from_file.as_ref().parent().expect("a parent for this path"); + let rel = from.relative(to_file); + format!("./{rel}") +} diff --git a/src/problem/npv_100.rs b/src/problem/npv_100.rs new file mode 100644 index 0000000..4c832f4 --- /dev/null +++ b/src/problem/npv_100.rs @@ -0,0 +1,22 @@ +use std::fmt; + +use derive_new::new; + +use crate::structure; + +#[derive(Clone, new)] +pub struct ByNameUndefinedAttribute { + #[new(into)] + attribute_name: String, +} + +impl fmt::Display for ByNameUndefinedAttribute { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let Self { attribute_name } = self; + let relative_package_file = structure::relative_file_for_package(attribute_name); + write!( + f, + "- pkgs.{attribute_name}: This attribute is not defined but it should be defined automatically as {relative_package_file}", + ) + } +} diff --git a/src/problem/npv_101.rs b/src/problem/npv_101.rs new file mode 100644 index 0000000..1b92f31 --- /dev/null +++ b/src/problem/npv_101.rs @@ -0,0 +1,22 @@ +use std::fmt; + +use derive_new::new; + +use crate::structure; + +#[derive(Clone, new)] +pub struct ByNameNonDerivation { + #[new(into)] + attribute_name: String, +} + +impl fmt::Display for ByNameNonDerivation { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let Self { attribute_name } = self; + let relative_package_file = structure::relative_file_for_package(attribute_name); + write!( + f, + "- pkgs.{attribute_name}: This attribute defined by {relative_package_file} is not a derivation", + ) + } +} diff --git a/src/problem/npv_102.rs b/src/problem/npv_102.rs new file mode 100644 index 0000000..859233d --- /dev/null +++ b/src/problem/npv_102.rs @@ -0,0 +1,19 @@ +use std::fmt; + +use derive_new::new; + +#[derive(Clone, new)] +pub struct ByNameInternalCallPackageUsed { + #[new(into)] + attribute_name: String, +} + +impl fmt::Display for ByNameInternalCallPackageUsed { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let Self { attribute_name } = self; + write!( + f, + "- pkgs.{attribute_name}: This attribute is defined using `_internalCallByNamePackageFile`, which is an internal function not intended for manual use.", + ) + } +} diff --git a/src/problem/npv_103.rs b/src/problem/npv_103.rs new file mode 100644 index 0000000..e70cea9 --- /dev/null +++ b/src/problem/npv_103.rs @@ -0,0 +1,19 @@ +use std::fmt; + +use derive_new::new; + +#[derive(Clone, new)] +pub struct ByNameCannotDetermineAttributeLocation { + #[new(into)] + attribute_name: String, +} + +impl fmt::Display for ByNameCannotDetermineAttributeLocation { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let Self { attribute_name } = self; + write!( + f, + "- pkgs.{attribute_name}: Cannot determine the location of this attribute using `builtins.unsafeGetAttrPos`.", + ) + } +} diff --git a/src/problem/npv_104.rs b/src/problem/npv_104.rs new file mode 100644 index 0000000..3aae406 --- /dev/null +++ b/src/problem/npv_104.rs @@ -0,0 +1,46 @@ +use std::fmt; + +use derive_new::new; +use indoc::writedoc; + +use crate::location::Location; +use crate::structure; + +use super::{create_path_expr, indent_definition}; + +#[derive(Clone, new)] +pub struct ByNameOverrideOfNonSyntacticCallPackage { + #[new(into)] + package_name: String, + location: Location, + #[new(into)] + definition: String, +} + +impl fmt::Display for ByNameOverrideOfNonSyntacticCallPackage { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let Self { + package_name, + location, + definition, + } = self; + let Location { file, line, column } = location; + let expected_package_path = structure::relative_file_for_package(package_name); + let relative_package_dir = structure::relative_dir_for_package(package_name); + let expected_path_expr = create_path_expr(file, expected_package_path); + let indented_definition = indent_definition(*column, definition); + + writedoc!( + f, + " + - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like + + {package_name} = callPackage {expected_path_expr} {{ /* ... */ }}; + + However, in this PR, it isn't defined that way. See the definition in {file}:{line} + + {indented_definition} + ", + ) + } +} diff --git a/src/problem/npv_105.rs b/src/problem/npv_105.rs new file mode 100644 index 0000000..1fcae39 --- /dev/null +++ b/src/problem/npv_105.rs @@ -0,0 +1,46 @@ +use std::fmt; + +use derive_new::new; +use indoc::writedoc; + +use crate::location::Location; +use crate::structure; + +use super::{create_path_expr, indent_definition}; + +#[derive(Clone, new)] +pub struct ByNameOverrideOfNonTopLevelPackage { + #[new(into)] + package_name: String, + location: Location, + #[new(into)] + definition: String, +} + +impl fmt::Display for ByNameOverrideOfNonTopLevelPackage { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let Self { + package_name, + location, + definition, + } = self; + let Location { file, line, column } = location; + let relative_package_dir = structure::relative_dir_for_package(package_name); + let expected_package_path = structure::relative_file_for_package(package_name); + let expected_path_expr = create_path_expr(file, expected_package_path); + let indented_definition = indent_definition(*column, definition); + + writedoc!( + f, + " + - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like + + {package_name} = callPackage {expected_path_expr} {{ /* ... */ }}; + + However, in this PR, a different `callPackage` is used. See the definition in {file}:{line}: + + {indented_definition} + ", + ) + } +} diff --git a/src/problem/npv_106.rs b/src/problem/npv_106.rs new file mode 100644 index 0000000..727210a --- /dev/null +++ b/src/problem/npv_106.rs @@ -0,0 +1,46 @@ +use std::fmt; + +use derive_new::new; +use indoc::writedoc; +use relative_path::RelativePathBuf; + +use crate::location::Location; +use crate::structure; + +use super::create_path_expr; + +#[derive(Clone, new)] +pub struct ByNameOverrideContainsWrongCallPackagePath { + #[new(into)] + package_name: String, + #[new(into)] + actual_path: RelativePathBuf, + location: Location, +} + +impl fmt::Display for ByNameOverrideContainsWrongCallPackagePath { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let Self { + package_name, + location, + actual_path, + } = self; + let Location { file, line, .. } = location; + let expected_package_path = structure::relative_file_for_package(package_name); + let expected_path_expr = create_path_expr(file, expected_package_path); + let relative_package_dir = structure::relative_dir_for_package(package_name); + let actual_path_expr = create_path_expr(file, actual_path); + writedoc!( + f, + " + - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like + + {package_name} = callPackage {expected_path_expr} {{ /* ... */ }}; + + However, in this PR, the first `callPackage` argument is the wrong path. See the definition in {file}:{line}: + + {package_name} = callPackage {actual_path_expr} {{ /* ... */ }}; + ", + ) + } +} diff --git a/src/problem/npv_107.rs b/src/problem/npv_107.rs new file mode 100644 index 0000000..e62ead9 --- /dev/null +++ b/src/problem/npv_107.rs @@ -0,0 +1,48 @@ +use std::fmt; + +use derive_new::new; +use indoc::writedoc; + +use crate::location::Location; +use crate::structure; + +use super::{create_path_expr, indent_definition}; + +#[derive(Clone, new)] +pub struct ByNameOverrideContainsEmptyArgument { + #[new(into)] + package_name: String, + location: Location, + #[new(into)] + definition: String, +} + +impl fmt::Display for ByNameOverrideContainsEmptyArgument { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let Self { + package_name, + location, + definition, + } = self; + let Location { file, line, column } = location; + let expected_package_path = structure::relative_file_for_package(package_name); + let expected_path_expr = create_path_expr(file, expected_package_path); + let relative_package_dir = structure::relative_dir_for_package(package_name); + let indented_definition = indent_definition(*column, definition); + + writedoc!( + f, + " + - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like + + {package_name} = callPackage {expected_path_expr} {{ /* ... */ }}; + + However, in this PR, the second argument is empty. See the definition in {file}:{line}: + + {indented_definition} + + Such a definition is provided automatically and therefore not necessary. Please remove it. + ", + ) + } +} diff --git a/src/problem/npv_108.rs b/src/problem/npv_108.rs new file mode 100644 index 0000000..a73e899 --- /dev/null +++ b/src/problem/npv_108.rs @@ -0,0 +1,46 @@ +use std::fmt; + +use derive_new::new; +use indoc::writedoc; + +use crate::location::Location; +use crate::structure; + +use super::{create_path_expr, indent_definition}; + +#[derive(Clone, new)] +pub struct ByNameOverrideContainsEmptyPath { + #[new(into)] + package_name: String, + location: Location, + #[new(into)] + definition: String, +} + +impl fmt::Display for ByNameOverrideContainsEmptyPath { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let Self { + package_name, + location, + definition, + } = self; + let Location { file, line, column } = location; + let relative_package_dir = structure::relative_dir_for_package(package_name); + let expected_package_path = structure::relative_file_for_package(package_name); + let expected_path_expr = create_path_expr(file, expected_package_path); + let indented_definition = indent_definition(*column, definition); + + writedoc!( + f, + " + - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like + + {package_name} = callPackage {expected_path_expr} {{ /* ... */ }}; + + However, in this PR, the first `callPackage` argument is not a path. See the definition in {file}:{line}: + + {indented_definition} + ", + ) + } +} diff --git a/src/problem/npv_109.rs b/src/problem/npv_109.rs new file mode 100644 index 0000000..67a1a3e --- /dev/null +++ b/src/problem/npv_109.rs @@ -0,0 +1,21 @@ +use std::fmt; + +use derive_new::new; + +use crate::structure; + +#[derive(Clone, new)] +pub struct ByNameShardIsNotDirectory { + #[new(into)] + shard_name: String, +} + +impl fmt::Display for ByNameShardIsNotDirectory { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let relative_shard_path = structure::relative_dir_for_shard(&self.shard_name); + write!( + f, + "- {relative_shard_path}: This is a file, but it should be a directory.", + ) + } +} diff --git a/src/problem/npv_110.rs b/src/problem/npv_110.rs new file mode 100644 index 0000000..53aba50 --- /dev/null +++ b/src/problem/npv_110.rs @@ -0,0 +1,22 @@ +use std::fmt; + +use derive_new::new; + +use crate::structure; + +#[derive(Clone, new)] +pub struct ByNameShardIsInvalid { + #[new(into)] + shard_name: String, +} + +impl fmt::Display for ByNameShardIsInvalid { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let shard_name = &self.shard_name; + let relative_shard_path = structure::relative_dir_for_shard(shard_name); + write!( + f, + "- {relative_shard_path}: Invalid directory name \"{shard_name}\", must be at most 2 ASCII characters consisting of a-z, 0-9, \"-\" or \"_\".", + ) + } +} diff --git a/src/problem/npv_111.rs b/src/problem/npv_111.rs new file mode 100644 index 0000000..5bee9b9 --- /dev/null +++ b/src/problem/npv_111.rs @@ -0,0 +1,26 @@ +use std::ffi::OsString; +use std::fmt; + +use derive_new::new; + +use crate::structure; + +#[derive(Clone, new)] +pub struct ByNameShardIsCaseSensitiveDuplicate { + #[new(into)] + shard_name: String, + first: OsString, + second: OsString, +} + +impl fmt::Display for ByNameShardIsCaseSensitiveDuplicate { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let relative_shard_path = structure::relative_dir_for_shard(&self.shard_name); + let first = self.first.to_string_lossy(); + let second = self.second.to_string_lossy(); + write!( + f, + "- {relative_shard_path}: Duplicate case-sensitive package directories \"{first}\" and \"{second}\"." + ) + } +} diff --git a/src/problem/npv_120.rs b/src/problem/npv_120.rs new file mode 100644 index 0000000..db8ffb9 --- /dev/null +++ b/src/problem/npv_120.rs @@ -0,0 +1,19 @@ +use std::fmt; + +use derive_new::new; + +#[derive(Clone, new)] +pub struct NixEvalError { + #[new(into)] + stderr: String, +} + +impl fmt::Display for NixEvalError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + f.write_str(&self.stderr)?; + write!( + f, + "- Nix evaluation failed for some package in `pkgs/by-name`, see error above" + ) + } +} diff --git a/src/problem/npv_121.rs b/src/problem/npv_121.rs new file mode 100644 index 0000000..70df091 --- /dev/null +++ b/src/problem/npv_121.rs @@ -0,0 +1,30 @@ +use std::fmt; + +use derive_new::new; +use relative_path::RelativePathBuf; + +#[derive(Clone, new)] +pub struct NixFileContainsPathInterpolation { + #[new(into)] + relative_package_dir: RelativePathBuf, + #[new(into)] + subpath: RelativePathBuf, + line: usize, + #[new(into)] + text: String, +} + +impl fmt::Display for NixFileContainsPathInterpolation { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let Self { + relative_package_dir, + subpath, + line, + text, + } = self; + write!( + f, + "- {relative_package_dir}: File {subpath} at line {line} contains the path expression \"{text}\", which is not yet supported and may point outside the directory of that package.", + ) + } +} diff --git a/src/problem/npv_122.rs b/src/problem/npv_122.rs new file mode 100644 index 0000000..3178a50 --- /dev/null +++ b/src/problem/npv_122.rs @@ -0,0 +1,30 @@ +use std::fmt; + +use derive_new::new; +use relative_path::RelativePathBuf; + +#[derive(Clone, new)] +pub struct NixFileContainsSearchPath { + #[new(into)] + relative_package_dir: RelativePathBuf, + #[new(into)] + subpath: RelativePathBuf, + line: usize, + #[new(into)] + text: String, +} + +impl fmt::Display for NixFileContainsSearchPath { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let Self { + relative_package_dir, + subpath, + line, + text, + } = self; + write!( + f, + "- {relative_package_dir}: File {subpath} at line {line} contains the nix search path expression \"{text}\" which may point outside the directory of that package.", + ) + } +} diff --git a/src/problem/npv_123.rs b/src/problem/npv_123.rs new file mode 100644 index 0000000..fea7bd2 --- /dev/null +++ b/src/problem/npv_123.rs @@ -0,0 +1,41 @@ +use std::fmt; + +use derive_new::new; +use indoc::writedoc; +use relative_path::RelativePathBuf; + +use crate::structure::PACKAGE_NIX_FILENAME; + +#[derive(Clone, new)] +pub struct NixFileContainsPathOutsideDirectory { + #[new(into)] + relative_package_dir: RelativePathBuf, + #[new(into)] + subpath: RelativePathBuf, + line: usize, + #[new(into)] + text: String, +} + +impl fmt::Display for NixFileContainsPathOutsideDirectory { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let Self { + relative_package_dir, + subpath, + line, + text, + } = self; + writedoc!( + f, + " + - {relative_package_dir}: File {subpath} at line {line} contains the path expression \"{text}\" which may point outside the directory of that package. + This is undesirable because it creates dependencies between internal paths, making it harder to reorganise Nixpkgs in the future. + Alternatives include: + - If you are creating a new version of a package with a common file between versions, consider following the recommendation in https://github.com/NixOS/nixpkgs/tree/master/pkgs/by-name#recommendation-for-new-packages-with-multiple-versions. + - If the path being referenced could be considered a stable interface with multiple uses, consider exposing it via a `pkgs` attribute, then taking it as a attribute argument in {PACKAGE_NIX_FILENAME}. + - If the path being referenced is internal and has multiple uses, consider passing the file as an explicit `callPackage` argument in `pkgs/top-level/all-packages.nix`. + - If the path being referenced is internal and will need to be modified independently of the original, consider copying it into the {relative_package_dir} directory. + " + ) + } +} diff --git a/src/problem/npv_124.rs b/src/problem/npv_124.rs new file mode 100644 index 0000000..6c9d4f5 --- /dev/null +++ b/src/problem/npv_124.rs @@ -0,0 +1,34 @@ +use std::sync::Arc; +use std::{fmt, io}; + +use derive_new::new; +use relative_path::RelativePathBuf; + +#[derive(Clone, new)] +pub struct NixFileContainsUnresolvablePath { + #[new(into)] + relative_package_dir: RelativePathBuf, + #[new(into)] + subpath: RelativePathBuf, + line: usize, + #[new(into)] + text: String, + #[new(into)] + io_error: Arc, +} + +impl fmt::Display for NixFileContainsUnresolvablePath { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let Self { + relative_package_dir, + subpath, + line, + text, + io_error, + } = self; + write!( + f, + "- {relative_package_dir}: File {subpath} at line {line} contains the path expression \"{text}\" which cannot be resolved: {io_error}.", + ) + } +} diff --git a/src/problem/npv_125.rs b/src/problem/npv_125.rs new file mode 100644 index 0000000..da7fd1c --- /dev/null +++ b/src/problem/npv_125.rs @@ -0,0 +1,25 @@ +use std::fmt; + +use derive_new::new; +use relative_path::RelativePathBuf; + +#[derive(Clone, new)] +pub struct PackageContainsSymlinkPointingOutside { + #[new(into)] + relative_package_dir: RelativePathBuf, + #[new(into)] + subpath: RelativePathBuf, +} + +impl fmt::Display for PackageContainsSymlinkPointingOutside { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let Self { + relative_package_dir, + subpath, + } = self; + write!( + f, + "- {relative_package_dir}: Path {subpath} is a symlink pointing to a path outside the directory of that package.", + ) + } +} diff --git a/src/problem/npv_126.rs b/src/problem/npv_126.rs new file mode 100644 index 0000000..338287b --- /dev/null +++ b/src/problem/npv_126.rs @@ -0,0 +1,29 @@ +use std::sync::Arc; +use std::{fmt, io}; + +use derive_new::new; +use relative_path::RelativePathBuf; + +#[derive(Clone, new)] +pub struct PackageContainsUnresolvableSymlink { + #[new(into)] + relative_package_dir: RelativePathBuf, + #[new(into)] + subpath: RelativePathBuf, + #[new(into)] + io_error: Arc, +} + +impl fmt::Display for PackageContainsUnresolvableSymlink { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let Self { + relative_package_dir, + subpath, + io_error, + } = self; + write!( + f, + "- {relative_package_dir}: Path {subpath} is a symlink which cannot be resolved: {io_error}.", + ) + } +} diff --git a/src/problem/npv_140.rs b/src/problem/npv_140.rs new file mode 100644 index 0000000..eb03d25 --- /dev/null +++ b/src/problem/npv_140.rs @@ -0,0 +1,22 @@ +use std::fmt; + +use derive_new::new; + +use crate::structure; + +#[derive(Clone, new)] +pub struct PackageDirectoryIsNotDirectory { + #[new(into)] + package_name: String, +} + +impl fmt::Display for PackageDirectoryIsNotDirectory { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let Self { package_name } = self; + let relative_package_dir = structure::relative_dir_for_package(package_name); + write!( + f, + "- {relative_package_dir}: This path is a file, but it should be a directory.", + ) + } +} diff --git a/src/problem/npv_141.rs b/src/problem/npv_141.rs new file mode 100644 index 0000000..7d22b8a --- /dev/null +++ b/src/problem/npv_141.rs @@ -0,0 +1,25 @@ +use std::fmt; + +use derive_new::new; +use relative_path::RelativePathBuf; + +#[derive(Clone, new)] +pub struct InvalidPackageDirectoryName { + #[new(into)] + package_name: String, + #[new(into)] + relative_package_dir: RelativePathBuf, +} + +impl fmt::Display for InvalidPackageDirectoryName { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let Self { + package_name, + relative_package_dir, + } = self; + write!( + f, + "- {relative_package_dir}: Invalid package directory name \"{package_name}\", must be ASCII characters consisting of a-z, A-Z, 0-9, \"-\" or \"_\".", + ) + } +} diff --git a/src/problem/npv_142.rs b/src/problem/npv_142.rs new file mode 100644 index 0000000..fd3e2b3 --- /dev/null +++ b/src/problem/npv_142.rs @@ -0,0 +1,28 @@ +use std::fmt; + +use derive_new::new; +use relative_path::RelativePathBuf; + +use crate::structure; + +#[derive(Clone, new)] +pub struct PackageInWrongShard { + #[new(into)] + package_name: String, + #[new(into)] + relative_package_dir: RelativePathBuf, +} + +impl fmt::Display for PackageInWrongShard { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let Self { + package_name, + relative_package_dir, + } = self; + let correct_relative_package_dir = structure::relative_dir_for_package(package_name); + write!( + f, + "- {relative_package_dir}: Incorrect directory location, should be {correct_relative_package_dir} instead.", + ) + } +} diff --git a/src/problem/npv_143.rs b/src/problem/npv_143.rs new file mode 100644 index 0000000..fd49c78 --- /dev/null +++ b/src/problem/npv_143.rs @@ -0,0 +1,22 @@ +use std::fmt; + +use derive_new::new; + +use crate::structure::{self, PACKAGE_NIX_FILENAME}; + +#[derive(Clone, new)] +pub struct PackageNixMissing { + #[new(into)] + package_name: String, +} + +impl fmt::Display for PackageNixMissing { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let Self { package_name } = self; + let relative_package_dir = structure::relative_dir_for_package(package_name); + write!( + f, + "- {relative_package_dir}: Missing required \"{PACKAGE_NIX_FILENAME}\" file.", + ) + } +} diff --git a/src/problem/npv_144.rs b/src/problem/npv_144.rs new file mode 100644 index 0000000..740e822 --- /dev/null +++ b/src/problem/npv_144.rs @@ -0,0 +1,22 @@ +use std::fmt; + +use derive_new::new; + +use crate::structure::{self, PACKAGE_NIX_FILENAME}; + +#[derive(Clone, new)] +pub struct PackageNixIsNotFile { + #[new(into)] + package_name: String, +} + +impl fmt::Display for PackageNixIsNotFile { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let Self { package_name } = self; + let relative_package_dir = structure::relative_dir_for_package(package_name); + write!( + f, + "- {relative_package_dir}: \"{PACKAGE_NIX_FILENAME}\" must be a file.", + ) + } +} diff --git a/src/problem/npv_160.rs b/src/problem/npv_160.rs new file mode 100644 index 0000000..9bf9571 --- /dev/null +++ b/src/problem/npv_160.rs @@ -0,0 +1,40 @@ +use std::fmt; + +use derive_new::new; +use indoc::writedoc; +use relative_path::RelativePathBuf; + +use crate::structure; + +#[derive(Clone, new)] +pub struct TopLevelPackageMovedOutOfByName { + #[new(into)] + package_name: String, + #[new(into)] + call_package_path: Option, + #[new(into)] + file: RelativePathBuf, +} + +impl fmt::Display for TopLevelPackageMovedOutOfByName { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let Self { + package_name, + call_package_path, + file, + } = self; + let relative_package_file = structure::relative_file_for_package(package_name); + let call_package_arg = if let Some(path) = call_package_path { + format!("./{}", path) + } else { + "...".into() + }; + writedoc!( + f, + " + - Attribute `pkgs.{package_name}` was previously defined in {relative_package_file}, but is now manually defined as `callPackage {call_package_arg} {{ /* ... */ }}` in {file}. + Please move the package back and remove the manual `callPackage`. + ", + ) + } +} diff --git a/src/problem/npv_161.rs b/src/problem/npv_161.rs new file mode 100644 index 0000000..36f5dbc --- /dev/null +++ b/src/problem/npv_161.rs @@ -0,0 +1,40 @@ +use std::fmt; + +use derive_new::new; +use indoc::writedoc; +use relative_path::RelativePathBuf; + +use crate::structure; + +#[derive(Clone, new)] +pub struct TopLevelPackageMovedOutOfByNameWithCustomArguments { + #[new(into)] + package_name: String, + #[new(into)] + call_package_path: Option, + #[new(into)] + file: RelativePathBuf, +} + +impl fmt::Display for TopLevelPackageMovedOutOfByNameWithCustomArguments { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let Self { + package_name, + call_package_path, + file, + } = self; + let relative_package_file = structure::relative_file_for_package(package_name); + let call_package_arg = if let Some(path) = call_package_path { + format!("./{}", path) + } else { + "...".into() + }; + writedoc!( + f, + " + - Attribute `pkgs.{package_name}` was previously defined in {relative_package_file}, but is now manually defined as `callPackage {call_package_arg} {{ ... }}` in {file}. + While the manual `callPackage` is still needed, it's not necessary to move the package files. + ", + ) + } +} diff --git a/src/problem/npv_162.rs b/src/problem/npv_162.rs new file mode 100644 index 0000000..0e3dae3 --- /dev/null +++ b/src/problem/npv_162.rs @@ -0,0 +1,42 @@ +use std::fmt; + +use derive_new::new; +use indoc::writedoc; +use relative_path::RelativePathBuf; + +use crate::structure; + +#[derive(Clone, new)] +pub struct NewTopLevelPackageShouldBeByName { + #[new(into)] + package_name: String, + #[new(into)] + call_package_path: Option, + #[new(into)] + file: RelativePathBuf, +} + +impl fmt::Display for NewTopLevelPackageShouldBeByName { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let Self { + package_name, + call_package_path, + file, + } = self; + let relative_package_file = structure::relative_file_for_package(package_name); + let call_package_arg = if let Some(path) = call_package_path { + format!("./{}", path) + } else { + "...".into() + }; + writedoc!( + f, + " + - Attribute `pkgs.{package_name}` is a new top-level package using `pkgs.callPackage {call_package_arg} {{ /* ... */ }}`. + Please define it in {relative_package_file} instead. + See `pkgs/by-name/README.md` for more details. + Since the second `callPackage` argument is `{{ }}`, no manual `callPackage` in {file} is needed anymore. + ", + ) + } +} diff --git a/src/problem/npv_163.rs b/src/problem/npv_163.rs new file mode 100644 index 0000000..dde218a --- /dev/null +++ b/src/problem/npv_163.rs @@ -0,0 +1,42 @@ +use std::fmt; + +use derive_new::new; +use indoc::writedoc; +use relative_path::RelativePathBuf; + +use crate::structure; + +#[derive(Clone, new)] +pub struct NewTopLevelPackageShouldBeByNameWithCustomArgument { + #[new(into)] + package_name: String, + #[new(into)] + call_package_path: Option, + #[new(into)] + file: RelativePathBuf, +} + +impl fmt::Display for NewTopLevelPackageShouldBeByNameWithCustomArgument { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let Self { + package_name, + call_package_path, + file, + } = self; + let relative_package_file = structure::relative_file_for_package(package_name); + let call_package_arg = if let Some(path) = call_package_path { + format!("./{}", path) + } else { + "...".into() + }; + writedoc!( + f, + " + - Attribute `pkgs.{package_name}` is a new top-level package using `pkgs.callPackage {call_package_arg} {{ /* ... */ }}`. + Please define it in {relative_package_file} instead. + See `pkgs/by-name/README.md` for more details. + Since the second `callPackage` argument is not `{{ }}`, the manual `callPackage` in {file} is still needed. + ", + ) + } +} diff --git a/src/ratchet.rs b/src/ratchet.rs index 4e6ce04..205db93 100644 --- a/src/ratchet.rs +++ b/src/ratchet.rs @@ -2,11 +2,13 @@ //! //! Each type has a `compare` method that validates the ratchet checks for that item. +use std::collections::HashMap; + +use relative_path::RelativePathBuf; + use crate::nix_file::CallPackageArgumentInfo; -use crate::nixpkgs_problem::{NixpkgsProblem, TopLevelPackageError}; +use crate::problem::{npv_160, npv_161, npv_162, npv_163, Problem}; use crate::validation::{self, Validation, Validation::Success}; -use relative_path::RelativePathBuf; -use std::collections::HashMap; /// The ratchet value for the entirety of Nixpkgs. #[derive(Default)] @@ -58,7 +60,7 @@ impl Package { } /// The ratchet state of a generic ratchet check. -pub enum RatchetState { +pub enum RatchetState { /// The ratchet is loose. It can be tightened more. In other words, this is the legacy state /// we're trying to move away from. /// @@ -75,32 +77,28 @@ pub enum RatchetState { NonApplicable, } -/// A trait that can convert an attribute-specific error context into a NixpkgsProblem. -pub trait ToNixpkgsProblem { +/// A trait that can convert an attribute-specific error context into a Problem. +pub trait ToProblem { /// Context relating to the Nixpkgs that is being transitioned _to_. type ToContext; - /// How to convert an attribute-specific error context into a NixpkgsProblem. - fn to_nixpkgs_problem( - name: &str, - optional_from: Option<()>, - to: &Self::ToContext, - ) -> NixpkgsProblem; + /// How to convert an attribute-specific error context into a Problem. + fn to_problem(name: &str, optional_from: Option<()>, to: &Self::ToContext) -> Problem; } -impl RatchetState { +impl RatchetState { /// Compare the previous ratchet state of an attribute to the new state. /// The previous state may be `None` in case the attribute is new. fn compare(name: &str, optional_from: Option<&Self>, to: &Self) -> Validation<()> { match (optional_from, to) { // Loosening a ratchet is not allowed. (Some(RatchetState::Tight), RatchetState::Loose(loose_context)) => { - Context::to_nixpkgs_problem(name, Some(()), loose_context).into() + Context::to_problem(name, Some(()), loose_context).into() } // Introducing a loose ratchet is also not allowed. (None, RatchetState::Loose(loose_context)) => { - Context::to_nixpkgs_problem(name, None, loose_context).into() + Context::to_problem(name, None, loose_context).into() } // Everything else is allowed, including: @@ -130,14 +128,10 @@ impl RatchetState { /// custom argument overrides. pub enum ManualDefinition {} -impl ToNixpkgsProblem for ManualDefinition { - type ToContext = NixpkgsProblem; +impl ToProblem for ManualDefinition { + type ToContext = Problem; - fn to_nixpkgs_problem( - _name: &str, - _optional_from: Option<()>, - to: &Self::ToContext, - ) -> NixpkgsProblem { + fn to_problem(_name: &str, _optional_from: Option<()>, to: &Self::ToContext) -> Problem { (*to).clone() } } @@ -149,20 +143,35 @@ impl ToNixpkgsProblem for ManualDefinition { /// to `pkgs/top-level/all-packages.nix`. pub enum UsesByName {} -impl ToNixpkgsProblem for UsesByName { +impl ToProblem for UsesByName { type ToContext = (CallPackageArgumentInfo, RelativePathBuf); - fn to_nixpkgs_problem( - name: &str, - optional_from: Option<()>, - (to, file): &Self::ToContext, - ) -> NixpkgsProblem { - NixpkgsProblem::TopLevelPackage(TopLevelPackageError { - package_name: name.to_owned(), - call_package_path: to.relative_path.clone(), - file: file.to_owned(), - is_new: optional_from.is_none(), - is_empty: to.empty_arg, - }) + fn to_problem(name: &str, optional_from: Option<()>, (to, file): &Self::ToContext) -> Problem { + let is_new = optional_from.is_none(); + let is_empty = to.empty_arg; + match (is_new, is_empty) { + (false, true) => { + npv_160::TopLevelPackageMovedOutOfByName::new(name, to.relative_path.clone(), file) + .into() + } + // This can happen if users mistakenly assume that `pkgs/by-name` can't be used + // for custom arguments. + (false, false) => npv_161::TopLevelPackageMovedOutOfByNameWithCustomArguments::new( + name, + to.relative_path.clone(), + file, + ) + .into(), + (true, true) => { + npv_162::NewTopLevelPackageShouldBeByName::new(name, to.relative_path.clone(), file) + .into() + } + (true, false) => npv_163::NewTopLevelPackageShouldBeByNameWithCustomArgument::new( + name, + to.relative_path.clone(), + file, + ) + .into(), + } } } diff --git a/src/references.rs b/src/references.rs index 2016561..3994718 100644 --- a/src/references.rs +++ b/src/references.rs @@ -1,15 +1,14 @@ -use crate::nixpkgs_problem::{ - NixFileError, NixFileErrorKind, NixpkgsProblem, PathError, PathErrorKind, -}; -use crate::utils; -use crate::validation::{self, ResultIteratorExt, Validation::Success}; -use crate::NixFileStore; -use relative_path::RelativePath; +use std::ffi::OsStr; +use std::path::Path; use anyhow::Context; +use relative_path::RelativePath; use rowan::ast::AstNode; -use std::ffi::OsStr; -use std::path::Path; + +use crate::problem::{npv_121, npv_122, npv_123, npv_124, npv_125, npv_126}; +use crate::structure::read_dir_sorted; +use crate::validation::{self, ResultIteratorExt, Validation::Success}; +use crate::NixFileStore; /// Check that every package directory in pkgs/by-name doesn't link to outside that directory. /// Both symlinks and Nix path expressions are checked. @@ -49,35 +48,32 @@ fn check_path( subpath: &RelativePath, ) -> validation::Result<()> { let path = subpath.to_path(absolute_package_dir); - let to_validation = |kind| -> validation::Validation<()> { - NixpkgsProblem::Path(PathError { - relative_package_dir: relative_package_dir.to_owned(), - subpath: subpath.to_owned(), - kind, - }) - .into() - }; Ok(if path.is_symlink() { // Check whether the symlink resolves to outside the package directory. match path.canonicalize() { Ok(target) => { - // No need to handle the case of it being inside the directory, since we scan - // through the entire directory recursively in any case. + // No need to handle the case of it being inside the directory, + // since we scan through the entire directory recursively in any case. if let Err(_prefix_error) = target.strip_prefix(absolute_package_dir) { - to_validation(PathErrorKind::OutsideSymlink) + npv_125::PackageContainsSymlinkPointingOutside::new( + relative_package_dir, + subpath, + ) + .into() } else { Success(()) } } - Err(io_error) => to_validation(PathErrorKind::UnresolvableSymlink { - io_error: io_error.to_string(), - }), + Err(err) => { + npv_126::PackageContainsUnresolvableSymlink::new(relative_package_dir, subpath, err) + .into() + } } } else if path.is_dir() { // Recursively check each entry validation::sequence_( - utils::read_dir_sorted(&path)? + read_dir_sorted(&path)? .into_iter() .map(|entry| { check_path( @@ -136,28 +132,38 @@ fn check_nix_file( return Success(()); }; - let to_validation = |kind| -> validation::Validation<()> { - NixpkgsProblem::NixFile(NixFileError { - relative_package_dir: relative_package_dir.to_owned(), - subpath: subpath.to_owned(), - line, - text, - kind, - }) - .into() - }; - use crate::nix_file::ResolvedPath; match nix_file.static_resolve_path(path, absolute_package_dir) { - ResolvedPath::Interpolated => to_validation(NixFileErrorKind::PathInterpolation), - ResolvedPath::SearchPath => to_validation(NixFileErrorKind::SearchPath), - ResolvedPath::Outside => to_validation(NixFileErrorKind::OutsidePathReference), - ResolvedPath::Unresolvable(e) => { - to_validation(NixFileErrorKind::UnresolvablePathReference { - io_error: e.to_string(), - }) - } + ResolvedPath::Interpolated => npv_121::NixFileContainsPathInterpolation::new( + relative_package_dir, + subpath, + line, + text, + ) + .into(), + ResolvedPath::SearchPath => npv_122::NixFileContainsSearchPath::new( + relative_package_dir, + subpath, + line, + text, + ) + .into(), + ResolvedPath::Outside => npv_123::NixFileContainsPathOutsideDirectory::new( + relative_package_dir, + subpath, + line, + text, + ) + .into(), + ResolvedPath::Unresolvable(err) => npv_124::NixFileContainsUnresolvablePath::new( + relative_package_dir, + subpath, + line, + text, + err, + ) + .into(), ResolvedPath::Within(..) => { // No need to handle the case of it being inside the directory, since we scan // through the entire directory recursively in any case. diff --git a/src/status.rs b/src/status.rs index 3a23222..5a70633 100644 --- a/src/status.rs +++ b/src/status.rs @@ -3,7 +3,7 @@ use std::process::ExitCode; use colored::Colorize as _; -use crate::nixpkgs_problem::NixpkgsProblem; +use crate::problem::Problem; pub enum Status { /// It's all green. @@ -14,22 +14,22 @@ pub enum Status { /// The base branch fails, the PR doesn't fix it, and the PR may also introduce additional /// problems. - BranchStillBroken(Vec), + BranchStillBroken(Vec), /// This PR introduces the problems listed. Please fix them before merging, otherwise the base /// branch would break. - ProblemsIntroduced(Vec), + ProblemsIntroduced(Vec), /// This PR introduces additional instances of discouraged patterns. Merging is discouraged but /// would not break the base branch. - DiscouragedPatternedIntroduced(Vec), + DiscouragedPatternedIntroduced(Vec), /// Some other error occurred. Error(anyhow::Error), } impl Status { - fn errors(&self) -> Option<&Vec> { + fn errors(&self) -> Option<&Vec> { match self { Self::ValidatedSuccessfully | Self::BranchHealed | Self::Error(..) => None, Self::BranchStillBroken(errors) diff --git a/src/structure.rs b/src/structure.rs index d70e0fa..d862bb0 100644 --- a/src/structure.rs +++ b/src/structure.rs @@ -1,23 +1,37 @@ -use crate::nixpkgs_problem::{ - NixpkgsProblem, PackageError, PackageErrorKind, ShardError, ShardErrorKind, -}; -use crate::references; -use crate::utils; -use crate::utils::{BASE_SUBPATH, PACKAGE_NIX_FILENAME}; -use crate::validation::{self, ResultIteratorExt, Validation::Success}; -use crate::NixFileStore; -use itertools::concat; +use std::fs::DirEntry; +use std::path::Path; + +use anyhow::Context; +use itertools::{concat, process_results}; use lazy_static::lazy_static; use regex::Regex; use relative_path::RelativePathBuf; -use std::fs::DirEntry; -use std::path::Path; + +use crate::problem::{npv_109, npv_110, npv_111, npv_140, npv_141, npv_142, npv_143, npv_144}; +use crate::references; +use crate::validation::{self, ResultIteratorExt, Validation::Success}; +use crate::NixFileStore; + +pub const BASE_SUBPATH: &str = "pkgs/by-name"; +pub const PACKAGE_NIX_FILENAME: &str = "package.nix"; lazy_static! { static ref SHARD_NAME_REGEX: Regex = Regex::new(r"^[a-z0-9_-]{1,2}$").unwrap(); static ref PACKAGE_NAME_REGEX: Regex = Regex::new(r"^[a-zA-Z0-9_-]+$").unwrap(); } +/// Deterministic file listing so that tests are reproducible. +pub fn read_dir_sorted(base_dir: &Path) -> anyhow::Result> { + let ctx = || format!("Could not list directory {}", base_dir.display()); + let listing = base_dir.read_dir().with_context(ctx)?; + + process_results(listing, |listing| { + use itertools::Itertools; + Itertools::collect_vec(listing.sorted_by_key(|entry| entry.file_name())) + }) + .with_context(ctx) +} + // Some utility functions for the basic structure pub fn shard_for_package(package_name: &str) -> String { @@ -44,7 +58,7 @@ pub fn check_structure( ) -> validation::Result> { let base_dir = path.join(BASE_SUBPATH); - let shard_results = utils::read_dir_sorted(&base_dir)? + let shard_results = read_dir_sorted(&base_dir)? .into_iter() .map(|shard_entry| -> validation::Result<_> { let shard_path = shard_entry.path(); @@ -54,26 +68,18 @@ pub fn check_structure( // README.md is allowed to be a file and not checked Success(vec![]) } else if !shard_path.is_dir() { - NixpkgsProblem::Shard(ShardError { - shard_name: shard_name.clone(), - kind: ShardErrorKind::ShardNonDir, - }) - .into() - // We can't check for any other errors if it's a file, since there's no + // We can't check for any other errors if it's not a directory, since there are no // subdirectories to check. + npv_109::ByNameShardIsNotDirectory::new(shard_name).into() } else { let shard_name_valid = SHARD_NAME_REGEX.is_match(&shard_name); let result = if !shard_name_valid { - NixpkgsProblem::Shard(ShardError { - shard_name: shard_name.clone(), - kind: ShardErrorKind::InvalidShardName, - }) - .into() + npv_110::ByNameShardIsInvalid::new(shard_name.clone()).into() } else { Success(()) }; - let entries = utils::read_dir_sorted(&shard_path)?; + let entries = read_dir_sorted(&shard_path)?; let duplicate_results = entries .iter() @@ -82,13 +88,11 @@ pub fn check_structure( l.file_name().to_ascii_lowercase() == r.file_name().to_ascii_lowercase() }) .map(|(l, r)| { - NixpkgsProblem::Shard(ShardError { - shard_name: shard_name.clone(), - kind: ShardErrorKind::CaseSensitiveDuplicate { - first: l.file_name(), - second: r.file_name(), - }, - }) + npv_111::ByNameShardIsCaseSensitiveDuplicate::new( + shard_name.clone(), + l.file_name(), + r.file_name(), + ) .into() }); @@ -128,37 +132,30 @@ fn check_package( let relative_package_dir = RelativePathBuf::from(format!("{BASE_SUBPATH}/{shard_name}/{package_name}")); - let to_validation = |kind| -> validation::Validation<()> { - NixpkgsProblem::Package(PackageError { - relative_package_dir: relative_package_dir.clone(), - kind, - }) - .into() - }; - Ok(if !package_path.is_dir() { - to_validation(PackageErrorKind::PackageNonDir { - package_name: package_name.clone(), - }) - .map(|_| package_name) + npv_140::PackageDirectoryIsNotDirectory::new(package_name).into() } else { let package_name_valid = PACKAGE_NAME_REGEX.is_match(&package_name); let result = if !package_name_valid { - to_validation(PackageErrorKind::InvalidPackageName { - invalid_package_name: package_name.clone(), - }) + npv_141::InvalidPackageDirectoryName::new( + package_name.clone(), + relative_package_dir.clone(), + ) + .into() } else { Success(()) }; let correct_relative_package_dir = relative_dir_for_package(&package_name); let result = result.and(if relative_package_dir != correct_relative_package_dir { - // Only show this error if we have a valid shard and package name. If one of those is - // wrong, you should fix that first. + // Only show this error if we have a valid shard and package name. + // If one of those is wrong, you should fix that first. if shard_name_valid && package_name_valid { - to_validation(PackageErrorKind::IncorrectShard { - correct_relative_package_dir: correct_relative_package_dir.clone(), - }) + npv_142::PackageInWrongShard::new( + package_name.clone(), + relative_package_dir.clone(), + ) + .into() } else { Success(()) } @@ -168,9 +165,9 @@ fn check_package( let package_nix_path = package_path.join(PACKAGE_NIX_FILENAME); let result = result.and(if !package_nix_path.exists() { - to_validation(PackageErrorKind::PackageNixNonExistent) - } else if package_nix_path.is_dir() { - to_validation(PackageErrorKind::PackageNixDir) + npv_143::PackageNixMissing::new(package_name.clone()).into() + } else if !package_nix_path.is_file() { + npv_144::PackageNixIsNotFile::new(package_name.clone()).into() } else { Success(()) }); diff --git a/src/validation.rs b/src/validation.rs index ea1fa65..51c3668 100644 --- a/src/validation.rs +++ b/src/validation.rs @@ -1,4 +1,4 @@ -use crate::nixpkgs_problem::NixpkgsProblem; +use crate::problem::Problem; use itertools::concat; use itertools::{ Either::{Left, Right}, @@ -12,14 +12,14 @@ use Validation::*; /// /// This leans on . pub enum Validation { - Failure(Vec), + Failure(Vec), Success(A), } -impl From for Validation { +impl> From

for Validation { /// Create a `Validation` from a single check problem - fn from(value: NixpkgsProblem) -> Self { - Failure(vec![value]) + fn from(value: P) -> Self { + Failure(vec![value.into()]) } } @@ -29,7 +29,7 @@ impl From for Validation { /// Such failures are not caused by the files in Nixpkgs. /// This hints at a bug in the code or a problem with the deployment. /// -/// - Ok(Failure(Vec)): A non-fatal validation problem with the Nixpkgs files. +/// - Ok(Failure(Vec)): A non-fatal validation problem with the Nixpkgs files. /// Further checks can be run even with this result type. /// Such problems can be fixed by changing the Nixpkgs files. /// @@ -74,7 +74,7 @@ impl Validation { impl Validation<()> { /// Combine two validations, both of which need to be successful for the return value to be - /// successful. The `NixpkgsProblem`s of both sides are returned concatenated. + /// successful. The `Problem`s of both sides are returned concatenated. pub fn and(self, other: Validation) -> Validation { match (self, other) { (Success(_), Success(right_value)) => Success(right_value), @@ -91,14 +91,15 @@ impl Validation<()> { /// successful, in which case the returned validation value contains a `Vec` of each individual /// value. /// -/// Otherwise, the `NixpkgsProblem`s of all validations are returned concatenated. +/// Otherwise, the `Problem`s of all validations are returned concatenated. pub fn sequence(check_results: impl IntoIterator>) -> Validation> { - let (errors, values): (Vec>, Vec) = check_results - .into_iter() - .partition_map(|validation| match validation { - Failure(err) => Left(err), - Success(value) => Right(value), - }); + let (errors, values): (Vec>, Vec) = + check_results + .into_iter() + .partition_map(|validation| match validation { + Failure(err) => Left(err), + Success(value) => Right(value), + }); // To combine the errors from the results we flatten all the error Vec's into a new Vec // This is not very efficient, but doesn't matter because generally we should have no errors