diff options
Diffstat (limited to 'pkgs/test/nixpkgs-check-by-name/src')
-rw-r--r-- | pkgs/test/nixpkgs-check-by-name/src/eval.nix | 47 | ||||
-rw-r--r-- | pkgs/test/nixpkgs-check-by-name/src/eval.rs | 116 | ||||
-rw-r--r-- | pkgs/test/nixpkgs-check-by-name/src/main.rs | 70 | ||||
-rw-r--r-- | pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs | 218 | ||||
-rw-r--r-- | pkgs/test/nixpkgs-check-by-name/src/references.rs | 247 | ||||
-rw-r--r-- | pkgs/test/nixpkgs-check-by-name/src/structure.rs | 266 | ||||
-rw-r--r-- | pkgs/test/nixpkgs-check-by-name/src/utils.rs | 27 | ||||
-rw-r--r-- | pkgs/test/nixpkgs-check-by-name/src/validation.rs | 102 |
8 files changed, 742 insertions, 351 deletions
diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.nix b/pkgs/test/nixpkgs-check-by-name/src/eval.nix index 7c0ae755215..bf9f19d8e46 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.nix +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.nix @@ -18,19 +18,42 @@ let callPackage = fn: args: let result = super.callPackage fn args; + variantInfo._attributeVariant = { + # These names are used by the deserializer on the Rust side + CallPackage.path = + if builtins.isPath fn then + toString fn + else + null; + CallPackage.empty_arg = + args == { }; + }; in if builtins.isAttrs result then # If this was the last overlay to be applied, we could just only return the `_callPackagePath`, # but that's not the case because stdenv has another overlays on top of user-provided ones. # So to not break the stdenv build we need to return the mostly proper result here - result // { - _callPackagePath = fn; - } + result // variantInfo else # It's very rare that callPackage doesn't return an attribute set, but it can occur. - { - _callPackagePath = fn; + variantInfo; + + _internalCallByNamePackageFile = file: + let + result = super._internalCallByNamePackageFile file; + variantInfo._attributeVariant = { + # This name is used by the deserializer on the Rust side + AutoCalled = null; }; + in + if builtins.isAttrs result then + # If this was the last overlay to be applied, we could just only return the `_callPackagePath`, + # but that's not the case because stdenv has another overlays on top of user-provided ones. + # So to not break the stdenv build we need to return the mostly proper result here + result // variantInfo + else + # It's very rare that callPackage doesn't return an attribute set, but it can occur. + variantInfo; }; pkgs = import nixpkgsPath { @@ -39,14 +62,14 @@ let overlays = [ callPackageOverlay ]; }; - attrInfo = attr: { + attrInfo = attr: + let + value = pkgs.${attr}; + in + { # These names are used by the deserializer on the Rust side - call_package_path = - if pkgs.${attr} ? _callPackagePath && builtins.isPath pkgs.${attr}._callPackagePath then - toString pkgs.${attr}._callPackagePath - else - null; - is_derivation = pkgs.lib.isDerivation pkgs.${attr}; + variant = value._attributeVariant or { Other = null; }; + is_derivation = pkgs.lib.isDerivation value; }; attrInfos = builtins.listToAttrs (map (name: { diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index 17e22495b22..e4f986748b4 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs @@ -1,11 +1,12 @@ +use crate::nixpkgs_problem::NixpkgsProblem; use crate::structure; -use crate::utils::ErrorWriter; +use crate::validation::{self, Validation::Success}; +use crate::Version; use std::path::Path; use anyhow::Context; use serde::Deserialize; use std::collections::HashMap; -use std::io; use std::path::PathBuf; use std::process; use tempfile::NamedTempFile; @@ -13,20 +14,38 @@ use tempfile::NamedTempFile; /// Attribute set of this structure is returned by eval.nix #[derive(Deserialize)] struct AttributeInfo { - call_package_path: Option<PathBuf>, + variant: AttributeVariant, is_derivation: bool, } +#[derive(Deserialize)] +enum AttributeVariant { + /// The attribute is auto-called as pkgs.callPackage using pkgs/by-name, + /// and it is not overridden by a definition in all-packages.nix + AutoCalled, + /// The attribute is defined as a pkgs.callPackage <path> <args>, + /// and overridden by all-packages.nix + CallPackage { + /// The <path> argument or None if it's not a path + path: Option<PathBuf>, + /// true if <args> is { } + empty_arg: bool, + }, + /// The attribute is not defined as pkgs.callPackage + Other, +} + const EXPR: &str = include_str!("eval.nix"); /// Check that the Nixpkgs attribute values corresponding to the packages in pkgs/by-name are /// of the form `callPackage <package_file> { ... }`. /// See the `eval.nix` file for how this is achieved on the Nix side -pub fn check_values<W: io::Write>( - error_writer: &mut ErrorWriter<W>, - nixpkgs: &structure::Nixpkgs, +pub fn check_values( + version: Version, + nixpkgs_path: &Path, + package_names: Vec<String>, eval_accessible_paths: Vec<&Path>, -) -> anyhow::Result<()> { +) -> validation::Result<()> { // Write the list of packages we need to check into a temporary JSON file. // This can then get read by the Nix evaluation. let attrs_file = NamedTempFile::new().context("Failed to create a temporary file")?; @@ -36,7 +55,7 @@ pub fn check_values<W: io::Write>( // entry is needed. let attrs_file_path = attrs_file.path().canonicalize()?; - serde_json::to_writer(&attrs_file, &nixpkgs.package_names).context(format!( + serde_json::to_writer(&attrs_file, &package_names).context(format!( "Failed to serialise the package names to the temporary path {}", attrs_file_path.display() ))?; @@ -68,9 +87,9 @@ pub fn check_values<W: io::Write>( .arg(&attrs_file_path) // Same for the nixpkgs to test .args(["--arg", "nixpkgsPath"]) - .arg(&nixpkgs.path) + .arg(nixpkgs_path) .arg("-I") - .arg(&nixpkgs.path); + .arg(nixpkgs_path); // Also add extra paths that need to be accessible for path in eval_accessible_paths { @@ -92,39 +111,54 @@ pub fn check_values<W: io::Write>( String::from_utf8_lossy(&result.stdout) ))?; - for package_name in &nixpkgs.package_names { - let relative_package_file = structure::Nixpkgs::relative_file_for_package(package_name); - let absolute_package_file = nixpkgs.path.join(&relative_package_file); + Ok(validation::sequence_(package_names.iter().map( + |package_name| { + let relative_package_file = structure::relative_file_for_package(package_name); + let absolute_package_file = nixpkgs_path.join(&relative_package_file); - if let Some(attribute_info) = actual_files.get(package_name) { - let is_expected_file = - if let Some(call_package_path) = &attribute_info.call_package_path { - absolute_package_file == *call_package_path - } else { - false + if let Some(attribute_info) = actual_files.get(package_name) { + let valid = match &attribute_info.variant { + AttributeVariant::AutoCalled => true, + AttributeVariant::CallPackage { path, empty_arg } => { + let correct_file = if let Some(call_package_path) = path { + absolute_package_file == *call_package_path + } else { + false + }; + // Only check for the argument to be non-empty if the version is V1 or + // higher + let non_empty = if version >= Version::V1 { + !empty_arg + } else { + true + }; + correct_file && non_empty + } + AttributeVariant::Other => false, }; - if !is_expected_file { - error_writer.write(&format!( - "pkgs.{package_name}: This attribute is not defined as `pkgs.callPackage {} {{ ... }}`.", - relative_package_file.display() - ))?; - continue; - } - - if !attribute_info.is_derivation { - error_writer.write(&format!( - "pkgs.{package_name}: This attribute defined by {} is not a derivation", - relative_package_file.display() - ))?; + if !valid { + NixpkgsProblem::WrongCallPackage { + relative_package_file: relative_package_file.clone(), + package_name: package_name.clone(), + } + .into() + } else if !attribute_info.is_derivation { + NixpkgsProblem::NonDerivation { + relative_package_file: relative_package_file.clone(), + package_name: package_name.clone(), + } + .into() + } else { + Success(()) + } + } else { + NixpkgsProblem::UndefinedAttr { + relative_package_file: relative_package_file.clone(), + package_name: package_name.clone(), + } + .into() } - } else { - error_writer.write(&format!( - "pkgs.{package_name}: This attribute is not defined but it should be defined automatically as {}", - relative_package_file.display() - ))?; - continue; - } - } - Ok(()) + }, + ))) } diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index 8c663061bb0..4cabf8f446f 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs @@ -1,28 +1,45 @@ mod eval; +mod nixpkgs_problem; mod references; mod structure; mod utils; +mod validation; +use crate::structure::check_structure; +use crate::validation::Validation::Failure; +use crate::validation::Validation::Success; use anyhow::Context; -use clap::Parser; +use clap::{Parser, ValueEnum}; use colored::Colorize; use std::io; use std::path::{Path, PathBuf}; use std::process::ExitCode; -use structure::Nixpkgs; -use utils::ErrorWriter; /// Program to check the validity of pkgs/by-name #[derive(Parser, Debug)] #[command(about)] -struct Args { +pub struct Args { /// Path to nixpkgs nixpkgs: PathBuf, + /// The version of the checks + /// Increasing this may cause failures for a Nixpkgs that succeeded before + /// TODO: Remove default once Nixpkgs CI passes this argument + #[arg(long, value_enum, default_value_t = Version::V0)] + version: Version, +} + +/// The version of the checks +#[derive(Debug, Clone, PartialEq, PartialOrd, ValueEnum)] +pub enum Version { + /// Initial version + V0, + /// Empty argument check + V1, } fn main() -> ExitCode { let args = Args::parse(); - match check_nixpkgs(&args.nixpkgs, vec![], &mut io::stderr()) { + match check_nixpkgs(&args.nixpkgs, args.version, vec![], &mut io::stderr()) { Ok(true) => { eprintln!("{}", "Validated successfully".green()); ExitCode::SUCCESS @@ -49,10 +66,11 @@ fn main() -> ExitCode { /// /// # Return value /// - `Err(e)` if an I/O-related error `e` occurred. -/// - `Ok(false)` if the structure is invalid, all the structural errors have been written to `error_writer`. -/// - `Ok(true)` if the structure is valid, nothing will have been written to `error_writer`. +/// - `Ok(false)` if there are problems, all of which will be written to `error_writer`. +/// - `Ok(true)` if there are no problems pub fn check_nixpkgs<W: io::Write>( nixpkgs_path: &Path, + version: Version, eval_accessible_paths: Vec<&Path>, error_writer: &mut W, ) -> anyhow::Result<bool> { @@ -61,31 +79,39 @@ pub fn check_nixpkgs<W: io::Write>( nixpkgs_path.display() ))?; - // Wraps the error_writer to print everything in red, and tracks whether anything was printed - // at all. Later used to figure out if the structure was valid or not. - let mut error_writer = ErrorWriter::new(error_writer); - - if !nixpkgs_path.join(structure::BASE_SUBPATH).exists() { + let check_result = if !nixpkgs_path.join(utils::BASE_SUBPATH).exists() { eprintln!( "Given Nixpkgs path does not contain a {} subdirectory, no check necessary.", - structure::BASE_SUBPATH + utils::BASE_SUBPATH ); + Success(()) } else { - let nixpkgs = Nixpkgs::new(&nixpkgs_path, &mut error_writer)?; + match check_structure(&nixpkgs_path)? { + Failure(errors) => Failure(errors), + Success(package_names) => + // Only if we could successfully parse the structure, we do the evaluation checks + { + eval::check_values(version, &nixpkgs_path, package_names, eval_accessible_paths)? + } + } + }; - if error_writer.empty { - // Only if we could successfully parse the structure, we do the semantic checks - eval::check_values(&mut error_writer, &nixpkgs, eval_accessible_paths)?; - references::check_references(&mut error_writer, &nixpkgs)?; + match check_result { + Failure(errors) => { + for error in errors { + writeln!(error_writer, "{}", error.to_string().red())? + } + Ok(false) } + Success(_) => Ok(true), } - Ok(error_writer.empty) } #[cfg(test)] mod tests { use crate::check_nixpkgs; - use crate::structure; + use crate::utils; + use crate::Version; use anyhow::Context; use std::fs; use std::path::Path; @@ -129,7 +155,7 @@ mod tests { return Ok(()); } - let base = path.join(structure::BASE_SUBPATH); + let base = path.join(utils::BASE_SUBPATH); fs::create_dir_all(base.join("fo/foo"))?; fs::write(base.join("fo/foo/package.nix"), "{ someDrv }: someDrv")?; @@ -174,7 +200,7 @@ mod tests { // We don't want coloring to mess up the tests let writer = temp_env::with_var("NO_COLOR", Some("1"), || -> anyhow::Result<_> { let mut writer = vec![]; - check_nixpkgs(&path, vec![&extra_nix_path], &mut writer) + check_nixpkgs(&path, Version::V1, vec![&extra_nix_path], &mut writer) .context(format!("Failed test case {name}"))?; Ok(writer) })?; diff --git a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs new file mode 100644 index 00000000000..2344a8cc132 --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs @@ -0,0 +1,218 @@ +use crate::utils::PACKAGE_NIX_FILENAME; +use rnix::parser::ParseError; +use std::ffi::OsString; +use std::fmt; +use std::io; +use std::path::PathBuf; + +/// Any problem that can occur when checking Nixpkgs +pub enum NixpkgsProblem { + ShardNonDir { + relative_shard_path: PathBuf, + }, + InvalidShardName { + relative_shard_path: PathBuf, + shard_name: String, + }, + PackageNonDir { + relative_package_dir: PathBuf, + }, + CaseSensitiveDuplicate { + relative_shard_path: PathBuf, + first: OsString, + second: OsString, + }, + InvalidPackageName { + relative_package_dir: PathBuf, + package_name: String, + }, + IncorrectShard { + relative_package_dir: PathBuf, + correct_relative_package_dir: PathBuf, + }, + PackageNixNonExistent { + relative_package_dir: PathBuf, + }, + PackageNixDir { + relative_package_dir: PathBuf, + }, + UndefinedAttr { + relative_package_file: PathBuf, + package_name: String, + }, + WrongCallPackage { + relative_package_file: PathBuf, + package_name: String, + }, + NonDerivation { + relative_package_file: PathBuf, + package_name: String, + }, + OutsideSymlink { + relative_package_dir: PathBuf, + subpath: PathBuf, + }, + UnresolvableSymlink { + relative_package_dir: PathBuf, + subpath: PathBuf, + io_error: io::Error, + }, + CouldNotParseNix { + relative_package_dir: PathBuf, + subpath: PathBuf, + error: ParseError, + }, + PathInterpolation { + relative_package_dir: PathBuf, + subpath: PathBuf, + line: usize, + text: String, + }, + SearchPath { + relative_package_dir: PathBuf, + subpath: PathBuf, + line: usize, + text: String, + }, + OutsidePathReference { + relative_package_dir: PathBuf, + subpath: PathBuf, + line: usize, + text: String, + }, + UnresolvablePathReference { + relative_package_dir: PathBuf, + subpath: PathBuf, + line: usize, + text: String, + io_error: io::Error, + }, +} + +impl fmt::Display for NixpkgsProblem { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + NixpkgsProblem::ShardNonDir { relative_shard_path } => + write!( + f, + "{}: This is a file, but it should be a directory.", + relative_shard_path.display(), + ), + NixpkgsProblem::InvalidShardName { relative_shard_path, shard_name } => + write!( + f, + "{}: Invalid directory name \"{shard_name}\", must be at most 2 ASCII characters consisting of a-z, 0-9, \"-\" or \"_\".", + relative_shard_path.display() + ), + NixpkgsProblem::PackageNonDir { relative_package_dir } => + write!( + f, + "{}: This path is a file, but it should be a directory.", + relative_package_dir.display(), + ), + NixpkgsProblem::CaseSensitiveDuplicate { relative_shard_path, first, second } => + write!( + f, + "{}: Duplicate case-sensitive package directories {first:?} and {second:?}.", + relative_shard_path.display(), + ), + NixpkgsProblem::InvalidPackageName { relative_package_dir, package_name } => + write!( + f, + "{}: Invalid package directory name \"{package_name}\", must be ASCII characters consisting of a-z, A-Z, 0-9, \"-\" or \"_\".", + relative_package_dir.display(), + ), + NixpkgsProblem::IncorrectShard { relative_package_dir, correct_relative_package_dir } => + write!( + f, + "{}: Incorrect directory location, should be {} instead.", + relative_package_dir.display(), + correct_relative_package_dir.display(), + ), + NixpkgsProblem::PackageNixNonExistent { relative_package_dir } => + write!( + f, + "{}: Missing required \"{PACKAGE_NIX_FILENAME}\" file.", + relative_package_dir.display(), + ), + NixpkgsProblem::PackageNixDir { relative_package_dir } => + write!( + f, + "{}: \"{PACKAGE_NIX_FILENAME}\" must be a file.", + relative_package_dir.display(), + ), + NixpkgsProblem::UndefinedAttr { relative_package_file, package_name } => + write!( + f, + "pkgs.{package_name}: This attribute is not defined but it should be defined automatically as {}", + relative_package_file.display() + ), + NixpkgsProblem::WrongCallPackage { relative_package_file, package_name } => + write!( + f, + "pkgs.{package_name}: This attribute is manually defined (most likely in pkgs/top-level/all-packages.nix), which is only allowed if the definition is of the form `pkgs.callPackage {} {{ ... }}` with a non-empty second argument.", + relative_package_file.display() + ), + NixpkgsProblem::NonDerivation { relative_package_file, package_name } => + write!( + f, + "pkgs.{package_name}: This attribute defined by {} is not a derivation", + relative_package_file.display() + ), + NixpkgsProblem::OutsideSymlink { relative_package_dir, subpath } => + write!( + f, + "{}: Path {} is a symlink pointing to a path outside the directory of that package.", + relative_package_dir.display(), + subpath.display(), + ), + NixpkgsProblem::UnresolvableSymlink { relative_package_dir, subpath, io_error } => + write!( + f, + "{}: Path {} is a symlink which cannot be resolved: {io_error}.", + relative_package_dir.display(), + subpath.display(), + ), + NixpkgsProblem::CouldNotParseNix { relative_package_dir, subpath, error } => + write!( + f, + "{}: File {} could not be parsed by rnix: {}", + relative_package_dir.display(), + subpath.display(), + error, + ), + NixpkgsProblem::PathInterpolation { relative_package_dir, subpath, line, text } => + write!( + f, + "{}: File {} at line {line} contains the path expression \"{}\", which is not yet supported and may point outside the directory of that package.", + relative_package_dir.display(), + subpath.display(), + text + ), + NixpkgsProblem::SearchPath { relative_package_dir, subpath, line, text } => + write!( + f, + "{}: File {} at line {line} contains the nix search path expression \"{}\" which may point outside the directory of that package.", + relative_package_dir.display(), + subpath.display(), + text + ), + NixpkgsProblem::OutsidePathReference { relative_package_dir, subpath, line, text } => + write!( + f, + "{}: File {} at line {line} contains the path expression \"{}\" which may point outside the directory of that package.", + relative_package_dir.display(), + subpath.display(), + text, + ), + NixpkgsProblem::UnresolvablePathReference { relative_package_dir, subpath, line, text, io_error } => + write!( + f, + "{}: File {} at line {line} contains the path expression \"{}\" which cannot be resolved: {io_error}.", + relative_package_dir.display(), + subpath.display(), + text, + ), + } + } +} diff --git a/pkgs/test/nixpkgs-check-by-name/src/references.rs b/pkgs/test/nixpkgs-check-by-name/src/references.rs index 16dc60729c4..0561a9b22e8 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/references.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/references.rs @@ -1,105 +1,98 @@ -use crate::structure::Nixpkgs; +use crate::nixpkgs_problem::NixpkgsProblem; use crate::utils; -use crate::utils::{ErrorWriter, LineIndex}; +use crate::utils::LineIndex; +use crate::validation::{self, ResultIteratorExt, Validation::Success}; use anyhow::Context; use rnix::{Root, SyntaxKind::NODE_PATH}; use std::ffi::OsStr; use std::fs::read_to_string; -use std::io; -use std::path::{Path, PathBuf}; - -/// Small helper so we don't need to pass in the same arguments to all functions -struct PackageContext<'a, W: io::Write> { - error_writer: &'a mut ErrorWriter<W>, - /// The package directory relative to Nixpkgs, such as `pkgs/by-name/fo/foo` - relative_package_dir: &'a PathBuf, - /// The absolute package directory - absolute_package_dir: &'a PathBuf, -} +use std::path::Path; /// Check that every package directory in pkgs/by-name doesn't link to outside that directory. /// Both symlinks and Nix path expressions are checked. -pub fn check_references<W: io::Write>( - error_writer: &mut ErrorWriter<W>, - nixpkgs: &Nixpkgs, -) -> anyhow::Result<()> { - // Check the directories for each package separately - for package_name in &nixpkgs.package_names { - let relative_package_dir = Nixpkgs::relative_dir_for_package(package_name); - let mut context = PackageContext { - error_writer, - relative_package_dir: &relative_package_dir, - absolute_package_dir: &nixpkgs.path.join(&relative_package_dir), - }; - - // The empty argument here is the subpath under the package directory to check - // An empty one means the package directory itself - check_path(&mut context, Path::new("")).context(format!( - "While checking the references in package directory {}", - relative_package_dir.display() - ))?; - } - Ok(()) +pub fn check_references( + relative_package_dir: &Path, + absolute_package_dir: &Path, +) -> validation::Result<()> { + // The empty argument here is the subpath under the package directory to check + // An empty one means the package directory itself + check_path(relative_package_dir, absolute_package_dir, Path::new("")).context(format!( + "While checking the references in package directory {}", + relative_package_dir.display() + )) } /// Checks for a specific path to not have references outside -fn check_path<W: io::Write>(context: &mut PackageContext<W>, subpath: &Path) -> anyhow::Result<()> { - let path = context.absolute_package_dir.join(subpath); +fn check_path( + relative_package_dir: &Path, + absolute_package_dir: &Path, + subpath: &Path, +) -> validation::Result<()> { + let path = absolute_package_dir.join(subpath); - if path.is_symlink() { + 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 anyways - if let Err(_prefix_error) = target.strip_prefix(context.absolute_package_dir) { - context.error_writer.write(&format!( - "{}: Path {} is a symlink pointing to a path outside the directory of that package.", - context.relative_package_dir.display(), - subpath.display(), - ))?; + if let Err(_prefix_error) = target.strip_prefix(absolute_package_dir) { + NixpkgsProblem::OutsideSymlink { + relative_package_dir: relative_package_dir.to_path_buf(), + subpath: subpath.to_path_buf(), + } + .into() + } else { + Success(()) } } - Err(e) => { - context.error_writer.write(&format!( - "{}: Path {} is a symlink which cannot be resolved: {e}.", - context.relative_package_dir.display(), - subpath.display(), - ))?; + Err(io_error) => NixpkgsProblem::UnresolvableSymlink { + relative_package_dir: relative_package_dir.to_path_buf(), + subpath: subpath.to_path_buf(), + io_error, } + .into(), } } else if path.is_dir() { // Recursively check each entry - for entry in utils::read_dir_sorted(&path)? { - let entry_subpath = subpath.join(entry.file_name()); - check_path(context, &entry_subpath) - .context(format!("Error while recursing into {}", subpath.display()))? - } + validation::sequence_( + utils::read_dir_sorted(&path)? + .into_iter() + .map(|entry| { + let entry_subpath = subpath.join(entry.file_name()); + check_path(relative_package_dir, absolute_package_dir, &entry_subpath) + .context(format!("Error while recursing into {}", subpath.display())) + }) + .collect_vec()?, + ) } else if path.is_file() { // Only check Nix files if let Some(ext) = path.extension() { if ext == OsStr::new("nix") { - check_nix_file(context, subpath).context(format!( - "Error while checking Nix file {}", - subpath.display() - ))? + check_nix_file(relative_package_dir, absolute_package_dir, subpath).context( + format!("Error while checking Nix file {}", subpath.display()), + )? + } else { + Success(()) } + } else { + Success(()) } } else { // This should never happen, git doesn't support other file types anyhow::bail!("Unsupported file type for path {}", subpath.display()); - } - Ok(()) + }) } /// Check whether a nix file contains path expression references pointing outside the package /// directory -fn check_nix_file<W: io::Write>( - context: &mut PackageContext<W>, +fn check_nix_file( + relative_package_dir: &Path, + absolute_package_dir: &Path, subpath: &Path, -) -> anyhow::Result<()> { - let path = context.absolute_package_dir.join(subpath); +) -> validation::Result<()> { + let path = absolute_package_dir.join(subpath); let parent_dir = path.parent().context(format!( "Could not get parent of path {}", subpath.display() @@ -110,75 +103,73 @@ fn check_nix_file<W: io::Write>( let root = Root::parse(&contents); if let Some(error) = root.errors().first() { - context.error_writer.write(&format!( - "{}: File {} could not be parsed by rnix: {}", - context.relative_package_dir.display(), - subpath.display(), - error, - ))?; - return Ok(()); + return Ok(NixpkgsProblem::CouldNotParseNix { + relative_package_dir: relative_package_dir.to_path_buf(), + subpath: subpath.to_path_buf(), + error: error.clone(), + } + .into()); } let line_index = LineIndex::new(&contents); - for node in root.syntax().descendants() { - // We're only interested in Path expressions - if node.kind() != NODE_PATH { - continue; - } - - let text = node.text().to_string(); - let line = line_index.line(node.text_range().start().into()); - - // Filters out ./foo/${bar}/baz - // TODO: We can just check ./foo - if node.children().count() != 0 { - context.error_writer.write(&format!( - "{}: File {} at line {line} contains the path expression \"{}\", which is not yet supported and may point outside the directory of that package.", - context.relative_package_dir.display(), - subpath.display(), - text - ))?; - continue; - } - - // Filters out search paths like <nixpkgs> - if text.starts_with('<') { - context.error_writer.write(&format!( - "{}: File {} at line {line} contains the nix search path expression \"{}\" which may point outside the directory of that package.", - context.relative_package_dir.display(), - subpath.display(), - text - ))?; - continue; - } - - // Resolves the reference of the Nix path - // turning `../baz` inside `/foo/bar/default.nix` to `/foo/baz` - match parent_dir.join(Path::new(&text)).canonicalize() { - Ok(target) => { - // Then checking if it's still in the package directory - // No need to handle the case of it being inside the directory, since we scan through the - // entire directory recursively anyways - if let Err(_prefix_error) = target.strip_prefix(context.absolute_package_dir) { - context.error_writer.write(&format!( - "{}: File {} at line {line} contains the path expression \"{}\" which may point outside the directory of that package.", - context.relative_package_dir.display(), - subpath.display(), - text, - ))?; + Ok(validation::sequence_(root.syntax().descendants().map( + |node| { + let text = node.text().to_string(); + let line = line_index.line(node.text_range().start().into()); + + if node.kind() != NODE_PATH { + // We're only interested in Path expressions + Success(()) + } else if node.children().count() != 0 { + // Filters out ./foo/${bar}/baz + // TODO: We can just check ./foo + NixpkgsProblem::PathInterpolation { + relative_package_dir: relative_package_dir.to_path_buf(), + subpath: subpath.to_path_buf(), + line, + text, } - } - Err(e) => { - context.error_writer.write(&format!( - "{}: File {} at line {line} contains the path expression \"{}\" which cannot be resolved: {e}.", - context.relative_package_dir.display(), - subpath.display(), + .into() + } else if text.starts_with('<') { + // Filters out search paths like <nixpkgs> + NixpkgsProblem::SearchPath { + relative_package_dir: relative_package_dir.to_path_buf(), + subpath: subpath.to_path_buf(), + line, text, - ))?; + } + .into() + } else { + // Resolves the reference of the Nix path + // turning `../baz` inside `/foo/bar/default.nix` to `/foo/baz` + match parent_dir.join(Path::new(&text)).canonicalize() { + Ok(target) => { + // Then checking if it's still in the package directory + // No need to handle the case of it being inside the directory, since we scan through the + // entire directory recursively anyways + if let Err(_prefix_error) = target.strip_prefix(absolute_package_dir) { + NixpkgsProblem::OutsidePathReference { + relative_package_dir: relative_package_dir.to_path_buf(), + subpath: subpath.to_path_buf(), + line, + text, + } + .into() + } else { + Success(()) + } + } + Err(e) => NixpkgsProblem::UnresolvablePathReference { + relative_package_dir: relative_package_dir.to_path_buf(), + subpath: subpath.to_path_buf(), + line, + text, + io_error: e, + } + .into(), + } } - }; - } - - Ok(()) + }, + ))) } diff --git a/pkgs/test/nixpkgs-check-by-name/src/structure.rs b/pkgs/test/nixpkgs-check-by-name/src/structure.rs index ea80128e487..4051ca037c9 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/structure.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/structure.rs @@ -1,152 +1,170 @@ +use crate::nixpkgs_problem::NixpkgsProblem; +use crate::references; use crate::utils; -use crate::utils::ErrorWriter; +use crate::utils::{BASE_SUBPATH, PACKAGE_NIX_FILENAME}; +use crate::validation::{self, ResultIteratorExt, Validation::Success}; +use itertools::concat; use lazy_static::lazy_static; use regex::Regex; -use std::collections::HashMap; -use std::io; +use std::fs::DirEntry; use std::path::{Path, PathBuf}; -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(); } -/// Contains information about the structure of the pkgs/by-name directory of a Nixpkgs -pub struct Nixpkgs { - /// The path to nixpkgs - pub path: PathBuf, - /// The names of all packages declared in pkgs/by-name - pub package_names: Vec<String>, -} - -impl Nixpkgs { - // Some utility functions for the basic structure +// Some utility functions for the basic structure - pub fn shard_for_package(package_name: &str) -> String { - package_name.to_lowercase().chars().take(2).collect() - } - - pub fn relative_dir_for_shard(shard_name: &str) -> PathBuf { - PathBuf::from(format!("{BASE_SUBPATH}/{shard_name}")) - } +pub fn shard_for_package(package_name: &str) -> String { + package_name.to_lowercase().chars().take(2).collect() +} - pub fn relative_dir_for_package(package_name: &str) -> PathBuf { - Nixpkgs::relative_dir_for_shard(&Nixpkgs::shard_for_package(package_name)) - .join(package_name) - } +pub fn relative_dir_for_shard(shard_name: &str) -> PathBuf { + PathBuf::from(format!("{BASE_SUBPATH}/{shard_name}")) +} - pub fn relative_file_for_package(package_name: &str) -> PathBuf { - Nixpkgs::relative_dir_for_package(package_name).join(PACKAGE_NIX_FILENAME) - } +pub fn relative_dir_for_package(package_name: &str) -> PathBuf { + relative_dir_for_shard(&shard_for_package(package_name)).join(package_name) } -impl Nixpkgs { - /// Read the structure of a Nixpkgs directory, displaying errors on the writer. - /// May return early with I/O errors. - pub fn new<W: io::Write>( - path: &Path, - error_writer: &mut ErrorWriter<W>, - ) -> anyhow::Result<Nixpkgs> { - let base_dir = path.join(BASE_SUBPATH); +pub fn relative_file_for_package(package_name: &str) -> PathBuf { + relative_dir_for_package(package_name).join(PACKAGE_NIX_FILENAME) +} - let mut package_names = Vec::new(); +/// Check the structure of Nixpkgs, returning the attribute names that are defined in +/// `pkgs/by-name` +pub fn check_structure(path: &Path) -> validation::Result<Vec<String>> { + let base_dir = path.join(BASE_SUBPATH); - for shard_entry in utils::read_dir_sorted(&base_dir)? { + let shard_results = utils::read_dir_sorted(&base_dir)? + .into_iter() + .map(|shard_entry| -> validation::Result<_> { let shard_path = shard_entry.path(); let shard_name = shard_entry.file_name().to_string_lossy().into_owned(); - let relative_shard_path = Nixpkgs::relative_dir_for_shard(&shard_name); + let relative_shard_path = relative_dir_for_shard(&shard_name); - if shard_name == "README.md" { + Ok(if shard_name == "README.md" { // README.md is allowed to be a file and not checked - continue; - } - - if !shard_path.is_dir() { - error_writer.write(&format!( - "{}: This is a file, but it should be a directory.", - relative_shard_path.display(), - ))?; - // we can't check for any other errors if it's a file, since there's no subdirectories to check - continue; - } - - let shard_name_valid = SHARD_NAME_REGEX.is_match(&shard_name); - if !shard_name_valid { - error_writer.write(&format!( - "{}: Invalid directory name \"{shard_name}\", must be at most 2 ASCII characters consisting of a-z, 0-9, \"-\" or \"_\".", - relative_shard_path.display() - ))?; - } - - let mut unique_package_names = HashMap::new(); - - for package_entry in utils::read_dir_sorted(&shard_path)? { - let package_path = package_entry.path(); - let package_name = package_entry.file_name().to_string_lossy().into_owned(); - let relative_package_dir = - PathBuf::from(format!("{BASE_SUBPATH}/{shard_name}/{package_name}")); - - if !package_path.is_dir() { - error_writer.write(&format!( - "{}: This path is a file, but it should be a directory.", - relative_package_dir.display(), - ))?; - continue; - } - if let Some(duplicate_package_name) = - unique_package_names.insert(package_name.to_lowercase(), package_name.clone()) - { - error_writer.write(&format!( - "{}: Duplicate case-sensitive package directories \"{duplicate_package_name}\" and \"{package_name}\".", - relative_shard_path.display(), - ))?; + Success(vec![]) + } else if !shard_path.is_dir() { + NixpkgsProblem::ShardNonDir { + relative_shard_path: relative_shard_path.clone(), } - - let package_name_valid = PACKAGE_NAME_REGEX.is_match(&package_name); - if !package_name_valid { - error_writer.write(&format!( - "{}: Invalid package directory name \"{package_name}\", must be ASCII characters consisting of a-z, A-Z, 0-9, \"-\" or \"_\".", - relative_package_dir.display(), - ))?; - } - - let correct_relative_package_dir = Nixpkgs::relative_dir_for_package(&package_name); - if relative_package_dir != correct_relative_package_dir { - // Only show this error if we have a valid shard and package name - // Because if one of those is wrong, you should fix that first - if shard_name_valid && package_name_valid { - error_writer.write(&format!( - "{}: Incorrect directory location, should be {} instead.", - relative_package_dir.display(), - correct_relative_package_dir.display(), - ))?; + .into() + // we can't check for any other errors if it's a file, since there's no subdirectories to check + } else { + let shard_name_valid = SHARD_NAME_REGEX.is_match(&shard_name); + let result = if !shard_name_valid { + NixpkgsProblem::InvalidShardName { + relative_shard_path: relative_shard_path.clone(), + shard_name: shard_name.clone(), } - } + .into() + } else { + Success(()) + }; + + let entries = utils::read_dir_sorted(&shard_path)?; + + let duplicate_results = entries + .iter() + .zip(entries.iter().skip(1)) + .filter(|(l, r)| { + l.file_name().to_ascii_lowercase() == r.file_name().to_ascii_lowercase() + }) + .map(|(l, r)| { + NixpkgsProblem::CaseSensitiveDuplicate { + relative_shard_path: relative_shard_path.clone(), + first: l.file_name(), + second: r.file_name(), + } + .into() + }); + + let result = result.and(validation::sequence_(duplicate_results)); + + let package_results = entries + .into_iter() + .map(|package_entry| { + check_package(path, &shard_name, shard_name_valid, package_entry) + }) + .collect_vec()?; + + result.and(validation::sequence(package_results)) + }) + }) + .collect_vec()?; - let package_nix_path = package_path.join(PACKAGE_NIX_FILENAME); - if !package_nix_path.exists() { - error_writer.write(&format!( - "{}: Missing required \"{PACKAGE_NIX_FILENAME}\" file.", - relative_package_dir.display(), - ))?; - } else if package_nix_path.is_dir() { - error_writer.write(&format!( - "{}: \"{PACKAGE_NIX_FILENAME}\" must be a file.", - relative_package_dir.display(), - ))?; - } + // Combine the package names conatained within each shard into a longer list + Ok(validation::sequence(shard_results).map(concat)) +} - package_names.push(package_name.clone()); - } +fn check_package( + path: &Path, + shard_name: &str, + shard_name_valid: bool, + package_entry: DirEntry, +) -> validation::Result<String> { + let package_path = package_entry.path(); + let package_name = package_entry.file_name().to_string_lossy().into_owned(); + let relative_package_dir = PathBuf::from(format!("{BASE_SUBPATH}/{shard_name}/{package_name}")); + + Ok(if !package_path.is_dir() { + NixpkgsProblem::PackageNonDir { + relative_package_dir: relative_package_dir.clone(), } - - Ok(Nixpkgs { - path: path.to_owned(), - package_names, - }) - } + .into() + } else { + let package_name_valid = PACKAGE_NAME_REGEX.is_match(&package_name); + let result = if !package_name_valid { + NixpkgsProblem::InvalidPackageName { + relative_package_dir: relative_package_dir.clone(), + package_name: package_name.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 + // Because if one of those is wrong, you should fix that first + if shard_name_valid && package_name_valid { + NixpkgsProblem::IncorrectShard { + relative_package_dir: relative_package_dir.clone(), + correct_relative_package_dir: correct_relative_package_dir.clone(), + } + .into() + } else { + Success(()) + } + } else { + Success(()) + }); + + let package_nix_path = package_path.join(PACKAGE_NIX_FILENAME); + let result = result.and(if !package_nix_path.exists() { + NixpkgsProblem::PackageNixNonExistent { + relative_package_dir: relative_package_dir.clone(), + } + .into() + } else if package_nix_path.is_dir() { + NixpkgsProblem::PackageNixDir { + relative_package_dir: relative_package_dir.clone(), + } + .into() + } else { + Success(()) + }); + + let result = result.and(references::check_references( + &relative_package_dir, + &path.join(&relative_package_dir), + )?); + + result.map(|_| package_name.clone()) + }) } diff --git a/pkgs/test/nixpkgs-check-by-name/src/utils.rs b/pkgs/test/nixpkgs-check-by-name/src/utils.rs index 325c736eca9..5cc4a0863ba 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/utils.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/utils.rs @@ -1,9 +1,11 @@ use anyhow::Context; -use colored::Colorize; use std::fs; use std::io; use std::path::Path; +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<Vec<fs::DirEntry>> { let listing = base_dir @@ -47,26 +49,3 @@ impl LineIndex { } } } - -/// A small wrapper around a generic io::Write specifically for errors: -/// - Print everything in red to signal it's an error -/// - Keep track of whether anything was printed at all, so that -/// it can be queried whether any errors were encountered at all -pub struct ErrorWriter<W> { - pub writer: W, - pub empty: bool, -} - -impl<W: io::Write> ErrorWriter<W> { - pub fn new(writer: W) -> ErrorWriter<W> { - ErrorWriter { - writer, - empty: true, - } - } - - pub fn write(&mut self, string: &str) -> io::Result<()> { - self.empty = false; - writeln!(self.writer, "{}", string.red()) - } -} diff --git a/pkgs/test/nixpkgs-check-by-name/src/validation.rs b/pkgs/test/nixpkgs-check-by-name/src/validation.rs new file mode 100644 index 00000000000..e7279385152 --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/src/validation.rs @@ -0,0 +1,102 @@ +use crate::nixpkgs_problem::NixpkgsProblem; +use itertools::concat; +use itertools::{ + Either::{Left, Right}, + Itertools, +}; +use Validation::*; + +/// The validation result of a check. +/// Instead of exiting at the first failure, +/// this type can accumulate multiple failures. +/// This can be achieved using the functions `and`, `sequence` and `sequence_` +/// +/// This leans on https://hackage.haskell.org/package/validation +pub enum Validation<A> { + Failure(Vec<NixpkgsProblem>), + Success(A), +} + +impl<A> From<NixpkgsProblem> for Validation<A> { + /// Create a `Validation<A>` from a single check problem + fn from(value: NixpkgsProblem) -> Self { + Failure(vec![value]) + } +} + +/// A type alias representing the result of a check, either: +/// - Err(anyhow::Error): A fatal failure, typically I/O errors. +/// 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<NixpkgsProblem>)): 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. +/// - Ok(Success(A)): A successful (potentially intermediate) result with an arbitrary value. +/// No fatal errors have occurred and no validation problems have been found with Nixpkgs. +pub type Result<A> = anyhow::Result<Validation<A>>; + +pub trait ResultIteratorExt<A, E>: Sized + Iterator<Item = std::result::Result<A, E>> { + fn collect_vec(self) -> std::result::Result<Vec<A>, E>; +} + +impl<I, A, E> ResultIteratorExt<A, E> for I +where + I: Sized + Iterator<Item = std::result::Result<A, E>>, +{ + /// A convenience version of `collect` specialised to a vector + fn collect_vec(self) -> std::result::Result<Vec<A>, E> { + self.collect() + } +} + +impl<A> Validation<A> { + /// Map a `Validation<A>` to a `Validation<B>` by applying a function to the + /// potentially contained value in case of success. + pub fn map<B>(self, f: impl FnOnce(A) -> B) -> Validation<B> { + match self { + Failure(err) => Failure(err), + Success(value) => Success(f(value)), + } + } +} + +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. + pub fn and<A>(self, other: Validation<A>) -> Validation<A> { + match (self, other) { + (Success(_), Success(right_value)) => Success(right_value), + (Failure(errors), Success(_)) => Failure(errors), + (Success(_), Failure(errors)) => Failure(errors), + (Failure(errors_l), Failure(errors_r)) => Failure(concat([errors_l, errors_r])), + } + } +} + +/// Combine many validations into a single one. +/// All given validations need to be successful in order for the returned validation to be 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. +pub fn sequence<A>(check_results: impl IntoIterator<Item = Validation<A>>) -> Validation<Vec<A>> { + let (errors, values): (Vec<Vec<NixpkgsProblem>>, Vec<A>) = 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 + let flattened_errors = errors.into_iter().concat(); + + if flattened_errors.is_empty() { + Success(values) + } else { + Failure(flattened_errors) + } +} + +/// Like `sequence`, but without any containing value, for convenience +pub fn sequence_(validations: impl IntoIterator<Item = Validation<()>>) -> Validation<()> { + sequence(validations).map(|_| ()) +} |