From 37f8f6681c761a5788b633f1da8f1f8a940bfabc Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Thu, 19 Oct 2023 02:24:29 +0200 Subject: [PATCH 01/28] tests.nixpkgs-check-by-name: Intermediate error type refactoring prep Currently the tool prints problems right as it is checking the code without an intermediate error representation. However for various reasons it would be beneficial to have an intermediate error type: - It makes the code cleaner, having all errors in one place - It allows printing the error in different ways, e.g. for a future --json mode This commit prepares for an incremental refactoring for an intermediate error/problem representation. Most notable is that we want to be able to collect multiple errors/problems and not just exit on the first one. We introduce the type alias CheckResult and CheckError (later renamed to NixpkgsProblem), where CheckError allows collecting multiple CheckErrors using the utility function flatten_check_results (later renamed to check_result::sequence) The write_check_result function is only temporarily introduced to help refactoring, it's removed again in later commits. --- pkgs/test/nixpkgs-check-by-name/Cargo.lock | 16 +++++ pkgs/test/nixpkgs-check-by-name/Cargo.toml | 1 + .../nixpkgs-check-by-name/src/check_result.rs | 61 +++++++++++++++++++ pkgs/test/nixpkgs-check-by-name/src/main.rs | 1 + 4 files changed, 79 insertions(+) create mode 100644 pkgs/test/nixpkgs-check-by-name/src/check_result.rs diff --git a/pkgs/test/nixpkgs-check-by-name/Cargo.lock b/pkgs/test/nixpkgs-check-by-name/Cargo.lock index aa4459c7cff8..fc3aeb9fd79b 100644 --- a/pkgs/test/nixpkgs-check-by-name/Cargo.lock +++ b/pkgs/test/nixpkgs-check-by-name/Cargo.lock @@ -162,6 +162,12 @@ version = "3.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7704b5fdd17b18ae31c4c1da5a2e0305a2bf17b5249300a9ee9ed7b72114c636" +[[package]] +name = "either" +version = "1.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a26ae43d7bcc3b814de94796a5e736d4029efb0ee900c12e2d54c993ad1a1e07" + [[package]] name = "errno" version = "0.3.2" @@ -218,6 +224,15 @@ dependencies = [ "windows-sys", ] +[[package]] +name = "itertools" +version = "0.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b1c173a5686ce8bfa551b3563d0c2170bf24ca44da99c7ca4bfdab5418c3fe57" +dependencies = [ + "either", +] + [[package]] name = "itoa" version = "1.0.9" @@ -274,6 +289,7 @@ dependencies = [ "anyhow", "clap", "colored", + "itertools", "lazy_static", "regex", "rnix", diff --git a/pkgs/test/nixpkgs-check-by-name/Cargo.toml b/pkgs/test/nixpkgs-check-by-name/Cargo.toml index 70b44d048200..1e6eaa1106d5 100644 --- a/pkgs/test/nixpkgs-check-by-name/Cargo.toml +++ b/pkgs/test/nixpkgs-check-by-name/Cargo.toml @@ -13,6 +13,7 @@ serde = { version = "1.0.185", features = ["derive"] } anyhow = "1.0" lazy_static = "1.4.0" colored = "2.0.4" +itertools = "0.11.0" [dev-dependencies] temp-env = "0.3.5" diff --git a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs new file mode 100644 index 000000000000..df894df45c71 --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs @@ -0,0 +1,61 @@ +use crate::ErrorWriter; +use itertools::{Either, Itertools}; +use std::fmt; +use std::io; +use std::path::PathBuf; + +pub enum CheckError {} + +impl CheckError { + pub fn into_result(self) -> CheckResult { + Ok(Either::Left(vec![self])) + } +} + +impl fmt::Display for CheckError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match *self {} + } +} + +pub fn write_check_result( + error_writer: &mut ErrorWriter, + check_result: CheckResult, +) -> anyhow::Result> { + match check_result? { + Either::Left(errors) => { + for error in errors { + error_writer.write(&error.to_string())? + } + Ok(None) + } + Either::Right(value) => Ok(Some(value)), + } +} + +pub fn pass(value: A) -> CheckResult { + Ok(Either::Right(value)) +} + +pub type CheckResult = anyhow::Result, A>>; + +pub fn flatten_check_results( + check_results: impl IntoIterator>, + value_transform: impl Fn(Vec) -> O, +) -> CheckResult { + let (errors, values): (Vec<_>, Vec<_>) = check_results + .into_iter() + .collect::>>()? + .into_iter() + .partition_map(|r| r); + + // 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().flatten().collect::>(); + + if flattened_errors.is_empty() { + Ok(Either::Right(value_transform(values))) + } else { + Ok(Either::Left(flattened_errors)) + } +} diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index 5d28077ae4ae..ee98c5a1a893 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs @@ -1,3 +1,4 @@ +mod check_result; mod eval; mod references; mod structure; From ed56d74c089d6b058a28eaf4a5ef04b190ed3651 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Thu, 19 Oct 2023 02:25:24 +0200 Subject: [PATCH 02/28] tests.nixpkgs-check-by-name: Intermediate path reference errors --- .../nixpkgs-check-by-name/src/check_result.rs | 35 +++++++++++++++++-- .../nixpkgs-check-by-name/src/references.rs | 32 ++++++++++------- 2 files changed, 52 insertions(+), 15 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs index df894df45c71..79d3dbe67662 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs @@ -4,7 +4,21 @@ use std::fmt; use std::io; use std::path::PathBuf; -pub enum CheckError {} +pub enum CheckError { + 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 CheckError { pub fn into_result(self) -> CheckResult { @@ -14,7 +28,24 @@ impl CheckError { impl fmt::Display for CheckError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - match *self {} + match self { + CheckError::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, + ), + CheckError::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 16dc60729c42..30eaee0d7484 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/references.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/references.rs @@ -1,3 +1,4 @@ +use crate::check_result::{pass, write_check_result, CheckError}; use crate::structure::Nixpkgs; use crate::utils; use crate::utils::{ErrorWriter, LineIndex}; @@ -155,29 +156,34 @@ fn check_nix_file( // 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() { + let check_result = 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(), + CheckError::OutsidePathReference { + relative_package_dir: context.relative_package_dir.clone(), + subpath: subpath.to_path_buf(), + line, text, - ))?; + } + .into_result() + } else { + pass(()) } } - 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(), - text, - ))?; + Err(e) => CheckError::UnresolvablePathReference { + relative_package_dir: context.relative_package_dir.clone(), + subpath: subpath.to_path_buf(), + line, + text, + io_error: e, } + .into_result(), }; + + write_check_result(context.error_writer, check_result)?; } Ok(()) From a755aa7d0251e4282ebdfdd33cbb00382b9a004c Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Thu, 19 Oct 2023 02:32:09 +0200 Subject: [PATCH 03/28] tests.nixpkgs-check-by-name: Intermediate SearchPath error --- .../nixpkgs-check-by-name/src/check_result.rs | 14 ++++ .../nixpkgs-check-by-name/src/references.rs | 64 +++++++++---------- 2 files changed, 46 insertions(+), 32 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs index 79d3dbe67662..cc004e012931 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs @@ -5,6 +5,12 @@ use std::io; use std::path::PathBuf; pub enum CheckError { + SearchPath { + relative_package_dir: PathBuf, + subpath: PathBuf, + line: usize, + text: String, + }, OutsidePathReference { relative_package_dir: PathBuf, subpath: PathBuf, @@ -29,6 +35,14 @@ impl CheckError { impl fmt::Display for CheckError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { + CheckError::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 + ), CheckError::OutsidePathReference { relative_package_dir, subpath, line, text } => write!( f, diff --git a/pkgs/test/nixpkgs-check-by-name/src/references.rs b/pkgs/test/nixpkgs-check-by-name/src/references.rs index 30eaee0d7484..dacac9c9ee5c 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/references.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/references.rs @@ -144,43 +144,43 @@ fn check_nix_file( } // Filters out search paths like - 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` - let check_result = 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) { - CheckError::OutsidePathReference { - relative_package_dir: context.relative_package_dir.clone(), - subpath: subpath.to_path_buf(), - line, - text, - } - .into_result() - } else { - pass(()) - } - } - Err(e) => CheckError::UnresolvablePathReference { + let check_result = if text.starts_with('<') { + CheckError::SearchPath { relative_package_dir: context.relative_package_dir.clone(), subpath: subpath.to_path_buf(), line, text, - io_error: e, } - .into_result(), + .into_result() + } 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(context.absolute_package_dir) { + CheckError::OutsidePathReference { + relative_package_dir: context.relative_package_dir.clone(), + subpath: subpath.to_path_buf(), + line, + text, + } + .into_result() + } else { + pass(()) + } + } + Err(e) => CheckError::UnresolvablePathReference { + relative_package_dir: context.relative_package_dir.clone(), + subpath: subpath.to_path_buf(), + line, + text, + io_error: e, + } + .into_result(), + } }; write_check_result(context.error_writer, check_result)?; From 96f6a350fa74e995dbbf750b17cad2d6cbb3186e Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Thu, 19 Oct 2023 02:42:22 +0200 Subject: [PATCH 04/28] tests.nixpkgs-check-by-name: Intermediate PathInterpolation error --- .../nixpkgs-check-by-name/src/check_result.rs | 14 ++++++++++++ .../nixpkgs-check-by-name/src/references.rs | 22 +++++++++---------- 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs index cc004e012931..ed00f383f193 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs @@ -5,6 +5,12 @@ use std::io; use std::path::PathBuf; pub enum CheckError { + PathInterpolation { + relative_package_dir: PathBuf, + subpath: PathBuf, + line: usize, + text: String, + }, SearchPath { relative_package_dir: PathBuf, subpath: PathBuf, @@ -35,6 +41,14 @@ impl CheckError { impl fmt::Display for CheckError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { + CheckError::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 + ), CheckError::SearchPath { relative_package_dir, subpath, line, text } => write!( f, diff --git a/pkgs/test/nixpkgs-check-by-name/src/references.rs b/pkgs/test/nixpkgs-check-by-name/src/references.rs index dacac9c9ee5c..6cc933e721a3 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/references.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/references.rs @@ -133,18 +133,16 @@ fn check_nix_file( // 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 - let check_result = if text.starts_with('<') { + let check_result = if node.children().count() != 0 { + CheckError::PathInterpolation { + relative_package_dir: context.relative_package_dir.clone(), + subpath: subpath.to_path_buf(), + line, + text, + } + .into_result() + } else if text.starts_with('<') { + // Filters out search paths like CheckError::SearchPath { relative_package_dir: context.relative_package_dir.clone(), subpath: subpath.to_path_buf(), From 9a3abc4383da1a430525902b87db02ddcfc279ee Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Thu, 19 Oct 2023 02:50:15 +0200 Subject: [PATCH 05/28] tests.nixpkgs-check-by-name: Intermediate CouldNotParseNix error --- .../nixpkgs-check-by-name/src/check_result.rs | 14 ++++++ .../nixpkgs-check-by-name/src/references.rs | 49 +++++++++---------- 2 files changed, 37 insertions(+), 26 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs index ed00f383f193..42468a29e809 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs @@ -1,10 +1,16 @@ use crate::ErrorWriter; use itertools::{Either, Itertools}; +use rnix::parser::ParseError; use std::fmt; use std::io; use std::path::PathBuf; pub enum CheckError { + CouldNotParseNix { + relative_package_dir: PathBuf, + subpath: PathBuf, + error: ParseError, + }, PathInterpolation { relative_package_dir: PathBuf, subpath: PathBuf, @@ -41,6 +47,14 @@ impl CheckError { impl fmt::Display for CheckError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { + CheckError::CouldNotParseNix { relative_package_dir, subpath, error } => + write!( + f, + "{}: File {} could not be parsed by rnix: {}", + relative_package_dir.display(), + subpath.display(), + error, + ), CheckError::PathInterpolation { relative_package_dir, subpath, line, text } => write!( f, diff --git a/pkgs/test/nixpkgs-check-by-name/src/references.rs b/pkgs/test/nixpkgs-check-by-name/src/references.rs index 6cc933e721a3..9c88507ff99b 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/references.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/references.rs @@ -1,4 +1,6 @@ -use crate::check_result::{pass, write_check_result, CheckError}; +use crate::check_result::{ + flatten_check_results, pass, write_check_result, CheckError, CheckResult, +}; use crate::structure::Nixpkgs; use crate::utils; use crate::utils::{ErrorWriter, LineIndex}; @@ -81,10 +83,11 @@ fn check_path(context: &mut PackageContext, subpath: &Path) -> // Only check Nix files if let Some(ext) = path.extension() { if ext == OsStr::new("nix") { - check_nix_file(context, subpath).context(format!( + let check_result = check_nix_file(context, subpath).context(format!( "Error while checking Nix file {}", subpath.display() - ))? + )); + write_check_result(context.error_writer, check_result)?; } } } else { @@ -99,7 +102,7 @@ fn check_path(context: &mut PackageContext, subpath: &Path) -> fn check_nix_file( context: &mut PackageContext, subpath: &Path, -) -> anyhow::Result<()> { +) -> CheckResult<()> { let path = context.absolute_package_dir.join(subpath); let parent_dir = path.parent().context(format!( "Could not get parent of path {}", @@ -111,29 +114,26 @@ fn check_nix_file( 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 CheckError::CouldNotParseNix { + relative_package_dir: context.relative_package_dir.clone(), + subpath: subpath.to_path_buf(), + error: error.clone(), + } + .into_result(); } 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 check_results = root.syntax().descendants().map(|node| { 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 - let check_result = if node.children().count() != 0 { + if node.kind() != NODE_PATH { + // We're only interested in Path expressions + pass(()) + } else if node.children().count() != 0 { + // Filters out ./foo/${bar}/baz + // TODO: We can just check ./foo CheckError::PathInterpolation { relative_package_dir: context.relative_package_dir.clone(), subpath: subpath.to_path_buf(), @@ -179,10 +179,7 @@ fn check_nix_file( } .into_result(), } - }; - - write_check_result(context.error_writer, check_result)?; - } - - Ok(()) + } + }); + flatten_check_results(check_results, |_| ()) } From 4897b63ae67d6f56a0d9a7f0db421467ebe8828b Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Thu, 19 Oct 2023 23:07:54 +0200 Subject: [PATCH 06/28] tests.nixpkgs-check-by-name: Intermediate Symlink errors --- .../nixpkgs-check-by-name/src/check_result.rs | 23 ++++++++++ .../nixpkgs-check-by-name/src/references.rs | 42 +++++++++++-------- 2 files changed, 47 insertions(+), 18 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs index 42468a29e809..32d43edc153a 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs @@ -6,6 +6,15 @@ use std::io; use std::path::PathBuf; pub enum CheckError { + 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, @@ -47,6 +56,20 @@ impl CheckError { impl fmt::Display for CheckError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { + CheckError::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(), + ), + CheckError::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(), + ), CheckError::CouldNotParseNix { relative_package_dir, subpath, error } => write!( f, diff --git a/pkgs/test/nixpkgs-check-by-name/src/references.rs b/pkgs/test/nixpkgs-check-by-name/src/references.rs index 9c88507ff99b..f72074483b14 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/references.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/references.rs @@ -52,26 +52,28 @@ fn check_path(context: &mut PackageContext, subpath: &Path) -> if path.is_symlink() { // Check whether the symlink resolves to outside the package directory - match path.canonicalize() { + let check_result = 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(), - ))?; + CheckError::OutsideSymlink { + relative_package_dir: context.relative_package_dir.clone(), + subpath: subpath.to_path_buf(), + } + .into_result() + } else { + pass(()) } } - 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) => CheckError::UnresolvableSymlink { + relative_package_dir: context.relative_package_dir.clone(), + subpath: subpath.to_path_buf(), + io_error, } - } + .into_result(), + }; + write_check_result(context.error_writer, check_result)?; } else if path.is_dir() { // Recursively check each entry for entry in utils::read_dir_sorted(&path)? { @@ -81,15 +83,19 @@ fn check_path(context: &mut PackageContext, subpath: &Path) -> } } else if path.is_file() { // Only check Nix files - if let Some(ext) = path.extension() { + let check_result = if let Some(ext) = path.extension() { if ext == OsStr::new("nix") { - let check_result = check_nix_file(context, subpath).context(format!( + check_nix_file(context, subpath).context(format!( "Error while checking Nix file {}", subpath.display() - )); - write_check_result(context.error_writer, check_result)?; + )) + } else { + pass(()) } - } + } else { + pass(()) + }; + write_check_result(context.error_writer, check_result)?; } else { // This should never happen, git doesn't support other file types anyhow::bail!("Unsupported file type for path {}", subpath.display()); From 9a0ef886236eefb13ea84422c5c8ba7865ccf6f8 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Thu, 19 Oct 2023 23:19:13 +0200 Subject: [PATCH 07/28] tests.nixpkgs-check-by-name: Intermediate NonDerivation error --- .../nixpkgs-check-by-name/src/check_result.rs | 10 ++++ pkgs/test/nixpkgs-check-by-name/src/eval.rs | 17 +++--- pkgs/test/nixpkgs-check-by-name/src/main.rs | 3 +- .../nixpkgs-check-by-name/src/references.rs | 53 +++++++------------ 4 files changed, 43 insertions(+), 40 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs index 32d43edc153a..4f1ee9f674f6 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs @@ -6,6 +6,10 @@ use std::io; use std::path::PathBuf; pub enum CheckError { + NonDerivation { + relative_package_file: PathBuf, + package_name: String, + }, OutsideSymlink { relative_package_dir: PathBuf, subpath: PathBuf, @@ -56,6 +60,12 @@ impl CheckError { impl fmt::Display for CheckError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { + CheckError::NonDerivation { relative_package_file, package_name } => + write!( + f, + "pkgs.{package_name}: This attribute defined by {} is not a derivation", + relative_package_file.display() + ), CheckError::OutsideSymlink { relative_package_dir, subpath } => write!( f, diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index 26836501c97a..695d52c0a8f2 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs @@ -1,3 +1,4 @@ +use crate::check_result::{pass, write_check_result, CheckError}; use crate::structure; use crate::utils::ErrorWriter; use crate::Version; @@ -144,12 +145,16 @@ pub fn check_values( 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() - ))?; - } + let check_result = if !attribute_info.is_derivation { + CheckError::NonDerivation { + relative_package_file: relative_package_file.clone(), + package_name: package_name.clone(), + } + .into_result() + } else { + pass(()) + }; + write_check_result(error_writer, check_result)?; } else { error_writer.write(&format!( "pkgs.{package_name}: This attribute is not defined but it should be defined automatically as {}", diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index ee98c5a1a893..80ed29646f72 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs @@ -5,6 +5,7 @@ mod structure; mod utils; use anyhow::Context; +use check_result::write_check_result; use clap::{Parser, ValueEnum}; use colored::Colorize; use std::io; @@ -92,7 +93,7 @@ pub fn check_nixpkgs( if error_writer.empty { // Only if we could successfully parse the structure, we do the semantic checks eval::check_values(version, &mut error_writer, &nixpkgs, eval_accessible_paths)?; - references::check_references(&mut error_writer, &nixpkgs)?; + write_check_result(&mut error_writer, references::check_references(&nixpkgs))?; } } Ok(error_writer.empty) diff --git a/pkgs/test/nixpkgs-check-by-name/src/references.rs b/pkgs/test/nixpkgs-check-by-name/src/references.rs index f72074483b14..b932b556f54f 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/references.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/references.rs @@ -1,20 +1,16 @@ -use crate::check_result::{ - flatten_check_results, pass, write_check_result, CheckError, CheckResult, -}; +use crate::check_result::{flatten_check_results, pass, CheckError, CheckResult}; use crate::structure::Nixpkgs; use crate::utils; -use crate::utils::{ErrorWriter, LineIndex}; +use crate::utils::LineIndex; 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, +struct PackageContext<'a> { /// The package directory relative to Nixpkgs, such as `pkgs/by-name/fo/foo` relative_package_dir: &'a PathBuf, /// The absolute package directory @@ -23,36 +19,32 @@ struct PackageContext<'a, W: io::Write> { /// 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( - error_writer: &mut ErrorWriter, - nixpkgs: &Nixpkgs, -) -> anyhow::Result<()> { +pub fn check_references(nixpkgs: &Nixpkgs) -> CheckResult<()> { // Check the directories for each package separately - for package_name in &nixpkgs.package_names { + let check_results = nixpkgs.package_names.iter().map(|package_name| { let relative_package_dir = Nixpkgs::relative_dir_for_package(package_name); - let mut context = PackageContext { - error_writer, + let context = PackageContext { 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!( + check_path(&context, Path::new("")).context(format!( "While checking the references in package directory {}", relative_package_dir.display() - ))?; - } - Ok(()) + )) + }); + flatten_check_results(check_results, |_| ()) } /// Checks for a specific path to not have references outside -fn check_path(context: &mut PackageContext, subpath: &Path) -> anyhow::Result<()> { +fn check_path(context: &PackageContext, subpath: &Path) -> CheckResult<()> { let path = context.absolute_package_dir.join(subpath); if path.is_symlink() { // Check whether the symlink resolves to outside the package directory - let check_result = match path.canonicalize() { + 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 @@ -72,18 +64,18 @@ fn check_path(context: &mut PackageContext, subpath: &Path) -> io_error, } .into_result(), - }; - write_check_result(context.error_writer, check_result)?; + } } else if path.is_dir() { // Recursively check each entry - for entry in utils::read_dir_sorted(&path)? { + let check_results = utils::read_dir_sorted(&path)?.into_iter().map(|entry| { let entry_subpath = subpath.join(entry.file_name()); check_path(context, &entry_subpath) - .context(format!("Error while recursing into {}", subpath.display()))? - } + .context(format!("Error while recursing into {}", subpath.display())) + }); + flatten_check_results(check_results, |_| ()) } else if path.is_file() { // Only check Nix files - let check_result = if let Some(ext) = path.extension() { + if let Some(ext) = path.extension() { if ext == OsStr::new("nix") { check_nix_file(context, subpath).context(format!( "Error while checking Nix file {}", @@ -94,21 +86,16 @@ fn check_path(context: &mut PackageContext, subpath: &Path) -> } } else { pass(()) - }; - write_check_result(context.error_writer, check_result)?; + } } 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( - context: &mut PackageContext, - subpath: &Path, -) -> CheckResult<()> { +fn check_nix_file(context: &PackageContext, subpath: &Path) -> CheckResult<()> { let path = context.absolute_package_dir.join(subpath); let parent_dir = path.parent().context(format!( "Could not get parent of path {}", From b688da8189f69a760d140400f3f19bd188754b57 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Thu, 19 Oct 2023 23:46:06 +0200 Subject: [PATCH 08/28] tests.nixpkgs-check-by-name: Intermediate WrongCallPackage error --- .../nixpkgs-check-by-name/src/check_result.rs | 10 ++++++++++ pkgs/test/nixpkgs-check-by-name/src/eval.rs | 16 +++++++--------- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs index 4f1ee9f674f6..6b1d120de8fa 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs @@ -6,6 +6,10 @@ use std::io; use std::path::PathBuf; pub enum CheckError { + WrongCallPackage { + relative_package_file: PathBuf, + package_name: String, + }, NonDerivation { relative_package_file: PathBuf, package_name: String, @@ -60,6 +64,12 @@ impl CheckError { impl fmt::Display for CheckError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { + CheckError::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() + ), CheckError::NonDerivation { relative_package_file, package_name } => write!( f, diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index 695d52c0a8f2..d1c5135b7d85 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs @@ -137,15 +137,13 @@ pub fn check_values( AttributeVariant::Other => false, }; - if !valid { - error_writer.write(&format!( - "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() - ))?; - continue; - } - - let check_result = if !attribute_info.is_derivation { + let check_result = if !valid { + CheckError::WrongCallPackage { + relative_package_file: relative_package_file.clone(), + package_name: package_name.clone(), + } + .into_result() + } else if !attribute_info.is_derivation { CheckError::NonDerivation { relative_package_file: relative_package_file.clone(), package_name: package_name.clone(), From 4f17b9367d7535fc5308557b92e1b306662b3d3d Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Thu, 19 Oct 2023 23:48:31 +0200 Subject: [PATCH 09/28] tests.nixpkgs-check-by-name: Intermediate UndefinedAttr error --- .../nixpkgs-check-by-name/src/check_result.rs | 10 ++++++++++ pkgs/test/nixpkgs-check-by-name/src/eval.rs | 20 +++++++++---------- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs index 6b1d120de8fa..f546b1285450 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs @@ -6,6 +6,10 @@ use std::io; use std::path::PathBuf; pub enum CheckError { + UndefinedAttr { + relative_package_file: PathBuf, + package_name: String, + }, WrongCallPackage { relative_package_file: PathBuf, package_name: String, @@ -64,6 +68,12 @@ impl CheckError { impl fmt::Display for CheckError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { + CheckError::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() + ), CheckError::WrongCallPackage { relative_package_file, package_name } => write!( f, diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index d1c5135b7d85..81cf43a381ed 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs @@ -116,7 +116,7 @@ pub fn check_values( let relative_package_file = structure::Nixpkgs::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 check_result = if let Some(attribute_info) = actual_files.get(package_name) { let valid = match &attribute_info.variant { AttributeVariant::AutoCalled => true, AttributeVariant::CallPackage { path, empty_arg } => { @@ -137,7 +137,7 @@ pub fn check_values( AttributeVariant::Other => false, }; - let check_result = if !valid { + if !valid { CheckError::WrongCallPackage { relative_package_file: relative_package_file.clone(), package_name: package_name.clone(), @@ -151,15 +151,15 @@ pub fn check_values( .into_result() } else { pass(()) - }; - write_check_result(error_writer, check_result)?; + } } 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; - } + CheckError::UndefinedAttr { + relative_package_file: relative_package_file.clone(), + package_name: package_name.clone(), + } + .into_result() + }; + write_check_result(error_writer, check_result)?; } Ok(()) } From e3979d14cda13b4a6cb15ebccf6052cc67c0db4c Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Thu, 19 Oct 2023 23:54:34 +0200 Subject: [PATCH 10/28] tests.nixpkgs-check-by-name: Intermediate PackageNixDir error --- .../nixpkgs-check-by-name/src/check_result.rs | 10 ++++++++++ pkgs/test/nixpkgs-check-by-name/src/eval.rs | 20 ++++++++----------- pkgs/test/nixpkgs-check-by-name/src/main.rs | 20 ++++++++++++------- .../nixpkgs-check-by-name/src/structure.rs | 15 +++++++------- pkgs/test/nixpkgs-check-by-name/src/utils.rs | 3 +++ 5 files changed, 41 insertions(+), 27 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs index f546b1285450..13d780cfd56e 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs @@ -1,3 +1,4 @@ +use crate::utils::PACKAGE_NIX_FILENAME; use crate::ErrorWriter; use itertools::{Either, Itertools}; use rnix::parser::ParseError; @@ -6,6 +7,9 @@ use std::io; use std::path::PathBuf; pub enum CheckError { + PackageNixDir { + relative_package_dir: PathBuf, + }, UndefinedAttr { relative_package_file: PathBuf, package_name: String, @@ -68,6 +72,12 @@ impl CheckError { impl fmt::Display for CheckError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { + CheckError::PackageNixDir { relative_package_dir } => + write!( + f, + "{}: \"{PACKAGE_NIX_FILENAME}\" must be a file.", + relative_package_dir.display(), + ), CheckError::UndefinedAttr { relative_package_file, package_name } => write!( f, diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index 81cf43a381ed..eb90a295a577 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs @@ -1,13 +1,11 @@ -use crate::check_result::{pass, write_check_result, CheckError}; +use crate::check_result::{flatten_check_results, pass, CheckError, CheckResult}; use crate::structure; -use crate::utils::ErrorWriter; 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; @@ -41,12 +39,11 @@ 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 { ... }`. /// See the `eval.nix` file for how this is achieved on the Nix side -pub fn check_values( +pub fn check_values( version: Version, - error_writer: &mut ErrorWriter, nixpkgs: &structure::Nixpkgs, eval_accessible_paths: Vec<&Path>, -) -> anyhow::Result<()> { +) -> CheckResult<()> { // 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")?; @@ -112,11 +109,11 @@ pub fn check_values( String::from_utf8_lossy(&result.stdout) ))?; - for package_name in &nixpkgs.package_names { + let check_results = nixpkgs.package_names.iter().map(|package_name| { let relative_package_file = structure::Nixpkgs::relative_file_for_package(package_name); let absolute_package_file = nixpkgs.path.join(&relative_package_file); - let check_result = if let Some(attribute_info) = actual_files.get(package_name) { + if let Some(attribute_info) = actual_files.get(package_name) { let valid = match &attribute_info.variant { AttributeVariant::AutoCalled => true, AttributeVariant::CallPackage { path, empty_arg } => { @@ -158,8 +155,7 @@ pub fn check_values( package_name: package_name.clone(), } .into_result() - }; - write_check_result(error_writer, check_result)?; - } - Ok(()) + } + }); + flatten_check_results(check_results, |_| ()) } diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index 80ed29646f72..7de6eadf6564 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs @@ -5,7 +5,7 @@ mod structure; mod utils; use anyhow::Context; -use check_result::write_check_result; +use check_result::{flatten_check_results, write_check_result}; use clap::{Parser, ValueEnum}; use colored::Colorize; use std::io; @@ -82,18 +82,24 @@ pub fn check_nixpkgs( // 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() { + 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 ); } else { let nixpkgs = Nixpkgs::new(&nixpkgs_path, &mut error_writer)?; if error_writer.empty { // Only if we could successfully parse the structure, we do the semantic checks - eval::check_values(version, &mut error_writer, &nixpkgs, eval_accessible_paths)?; - write_check_result(&mut error_writer, references::check_references(&nixpkgs))?; + let check_result = flatten_check_results( + [ + eval::check_values(version, &nixpkgs, eval_accessible_paths), + references::check_references(&nixpkgs), + ], + |_| (), + ); + write_check_result(&mut error_writer, check_result)?; } } Ok(error_writer.empty) @@ -102,7 +108,7 @@ pub fn check_nixpkgs( #[cfg(test)] mod tests { use crate::check_nixpkgs; - use crate::structure; + use crate::utils; use crate::Version; use anyhow::Context; use std::fs; @@ -147,7 +153,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")?; diff --git a/pkgs/test/nixpkgs-check-by-name/src/structure.rs b/pkgs/test/nixpkgs-check-by-name/src/structure.rs index ea80128e487a..928d5ea088ec 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/structure.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/structure.rs @@ -1,14 +1,12 @@ +use crate::check_result::{write_check_result, CheckError}; use crate::utils; -use crate::utils::ErrorWriter; +use crate::utils::{ErrorWriter, BASE_SUBPATH, PACKAGE_NIX_FILENAME}; use lazy_static::lazy_static; use regex::Regex; use std::collections::HashMap; use std::io; 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(); @@ -134,10 +132,11 @@ impl Nixpkgs { 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(), - ))?; + let check_result = CheckError::PackageNixDir { + relative_package_dir: relative_package_dir.clone(), + } + .into_result::<()>(); + write_check_result(error_writer, check_result)?; } package_names.push(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 325c736eca98..728e50e72336 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/utils.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/utils.rs @@ -4,6 +4,9 @@ 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> { let listing = base_dir From 64f5eb616e673babd2b54cc07394b04b22242493 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 20 Oct 2023 00:10:29 +0200 Subject: [PATCH 11/28] tests.nixpkgs-check-by-name: Intermediate PackageNixNonExistent error --- .../nixpkgs-check-by-name/src/check_result.rs | 9 +++++++ .../nixpkgs-check-by-name/src/structure.rs | 25 +++++++++++-------- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs index 13d780cfd56e..882ad8c8e2ca 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs @@ -7,6 +7,9 @@ use std::io; use std::path::PathBuf; pub enum CheckError { + PackageNixNonExistent { + relative_package_dir: PathBuf, + }, PackageNixDir { relative_package_dir: PathBuf, }, @@ -72,6 +75,12 @@ impl CheckError { impl fmt::Display for CheckError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { + CheckError::PackageNixNonExistent { relative_package_dir } => + write!( + f, + "{}: Missing required \"{PACKAGE_NIX_FILENAME}\" file.", + relative_package_dir.display(), + ), CheckError::PackageNixDir { relative_package_dir } => write!( f, diff --git a/pkgs/test/nixpkgs-check-by-name/src/structure.rs b/pkgs/test/nixpkgs-check-by-name/src/structure.rs index 928d5ea088ec..5a60dc26954b 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/structure.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/structure.rs @@ -1,4 +1,4 @@ -use crate::check_result::{write_check_result, CheckError}; +use crate::check_result::{pass, write_check_result, CheckError}; use crate::utils; use crate::utils::{ErrorWriter, BASE_SUBPATH, PACKAGE_NIX_FILENAME}; use lazy_static::lazy_static; @@ -126,18 +126,21 @@ impl Nixpkgs { } 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() { - let check_result = CheckError::PackageNixDir { + let check_result = if !package_nix_path.exists() { + CheckError::PackageNixNonExistent { relative_package_dir: relative_package_dir.clone(), } - .into_result::<()>(); - write_check_result(error_writer, check_result)?; - } + .into_result() + } else if package_nix_path.is_dir() { + CheckError::PackageNixDir { + relative_package_dir: relative_package_dir.clone(), + } + .into_result() + } else { + pass(()) + }; + + write_check_result(error_writer, check_result)?; package_names.push(package_name.clone()); } From b011d53bda903f70d884ca42a43b65db9b430901 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 20 Oct 2023 00:30:18 +0200 Subject: [PATCH 12/28] tests.nixpkgs-check-by-name: Intermediate IncorrectShard error --- .../nixpkgs-check-by-name/src/check_result.rs | 11 ++++++++ .../nixpkgs-check-by-name/src/structure.rs | 25 ++++++++++++------- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs index 882ad8c8e2ca..e42652039c2c 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs @@ -7,6 +7,10 @@ use std::io; use std::path::PathBuf; pub enum CheckError { + IncorrectShard { + relative_package_dir: PathBuf, + correct_relative_package_dir: PathBuf, + }, PackageNixNonExistent { relative_package_dir: PathBuf, }, @@ -75,6 +79,13 @@ impl CheckError { impl fmt::Display for CheckError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { + CheckError::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(), + ), CheckError::PackageNixNonExistent { relative_package_dir } => write!( f, diff --git a/pkgs/test/nixpkgs-check-by-name/src/structure.rs b/pkgs/test/nixpkgs-check-by-name/src/structure.rs index 5a60dc26954b..8dc9ca61940a 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/structure.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/structure.rs @@ -1,4 +1,4 @@ -use crate::check_result::{pass, write_check_result, CheckError}; +use crate::check_result::{flatten_check_results, pass, write_check_result, CheckError}; use crate::utils; use crate::utils::{ErrorWriter, BASE_SUBPATH, PACKAGE_NIX_FILENAME}; use lazy_static::lazy_static; @@ -113,20 +113,24 @@ impl Nixpkgs { } let correct_relative_package_dir = Nixpkgs::relative_dir_for_package(&package_name); - if relative_package_dir != correct_relative_package_dir { + let shard_check_result = 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(), - ))?; + CheckError::IncorrectShard { + relative_package_dir: relative_package_dir.clone(), + correct_relative_package_dir: correct_relative_package_dir.clone(), + } + .into_result() + } else { + pass(()) } - } + } else { + pass(()) + }; let package_nix_path = package_path.join(PACKAGE_NIX_FILENAME); - let check_result = if !package_nix_path.exists() { + let package_nix_check_result = if !package_nix_path.exists() { CheckError::PackageNixNonExistent { relative_package_dir: relative_package_dir.clone(), } @@ -140,6 +144,9 @@ impl Nixpkgs { pass(()) }; + let check_result = + flatten_check_results([shard_check_result, package_nix_check_result], |_| ()); + write_check_result(error_writer, check_result)?; package_names.push(package_name.clone()); From e7d9cc96ed0f8edeb52a2dbeefdcf00e4aedf8ee Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 20 Oct 2023 00:33:39 +0200 Subject: [PATCH 13/28] tests.nixpkgs-check-by-name: Intermediate InvalidPackageName error --- .../nixpkgs-check-by-name/src/check_result.rs | 10 ++++++++ .../nixpkgs-check-by-name/src/structure.rs | 25 +++++++++++++------ 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs index e42652039c2c..ff90ea7c5092 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs @@ -7,6 +7,10 @@ use std::io; use std::path::PathBuf; pub enum CheckError { + InvalidPackageName { + relative_package_dir: PathBuf, + package_name: String, + }, IncorrectShard { relative_package_dir: PathBuf, correct_relative_package_dir: PathBuf, @@ -79,6 +83,12 @@ impl CheckError { impl fmt::Display for CheckError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { + CheckError::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(), + ), CheckError::IncorrectShard { relative_package_dir, correct_relative_package_dir } => write!( f, diff --git a/pkgs/test/nixpkgs-check-by-name/src/structure.rs b/pkgs/test/nixpkgs-check-by-name/src/structure.rs index 8dc9ca61940a..e2d03d83a53f 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/structure.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/structure.rs @@ -105,12 +105,15 @@ impl Nixpkgs { } 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 name_check_result = if !package_name_valid { + CheckError::InvalidPackageName { + relative_package_dir: relative_package_dir.clone(), + package_name: package_name.clone(), + } + .into_result() + } else { + pass(()) + }; let correct_relative_package_dir = Nixpkgs::relative_dir_for_package(&package_name); let shard_check_result = if relative_package_dir != correct_relative_package_dir { @@ -144,8 +147,14 @@ impl Nixpkgs { pass(()) }; - let check_result = - flatten_check_results([shard_check_result, package_nix_check_result], |_| ()); + let check_result = flatten_check_results( + [ + name_check_result, + shard_check_result, + package_nix_check_result, + ], + |_| (), + ); write_check_result(error_writer, check_result)?; From 935f82267a84a69a7aa66b2b849fc57f15d24685 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 20 Oct 2023 00:55:55 +0200 Subject: [PATCH 14/28] tests.nixpkgs-check-by-name: Intermediate CaseSensitiveDuplicate error --- .../nixpkgs-check-by-name/src/check_result.rs | 12 +++++++ .../nixpkgs-check-by-name/src/structure.rs | 33 ++++++++++++------- 2 files changed, 33 insertions(+), 12 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs index ff90ea7c5092..2bceadbf807c 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs @@ -2,11 +2,17 @@ use crate::utils::PACKAGE_NIX_FILENAME; use crate::ErrorWriter; use itertools::{Either, Itertools}; use rnix::parser::ParseError; +use std::ffi::OsString; use std::fmt; use std::io; use std::path::PathBuf; pub enum CheckError { + CaseSensitiveDuplicate { + relative_shard_path: PathBuf, + first: OsString, + second: OsString, + }, InvalidPackageName { relative_package_dir: PathBuf, package_name: String, @@ -83,6 +89,12 @@ impl CheckError { impl fmt::Display for CheckError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { + CheckError::CaseSensitiveDuplicate { relative_shard_path, first, second } => + write!( + f, + "{}: Duplicate case-sensitive package directories {first:?} and {second:?}.", + relative_shard_path.display(), + ), CheckError::InvalidPackageName { relative_package_dir, package_name } => write!( f, diff --git a/pkgs/test/nixpkgs-check-by-name/src/structure.rs b/pkgs/test/nixpkgs-check-by-name/src/structure.rs index e2d03d83a53f..f64f80d381aa 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/structure.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/structure.rs @@ -3,7 +3,6 @@ use crate::utils; use crate::utils::{ErrorWriter, BASE_SUBPATH, PACKAGE_NIX_FILENAME}; use lazy_static::lazy_static; use regex::Regex; -use std::collections::HashMap; use std::io; use std::path::{Path, PathBuf}; @@ -79,9 +78,28 @@ impl Nixpkgs { ))?; } - let mut unique_package_names = HashMap::new(); + let entries = utils::read_dir_sorted(&shard_path)?; - for package_entry in utils::read_dir_sorted(&shard_path)? { + let duplicate_check_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)| { + CheckError::CaseSensitiveDuplicate { + relative_shard_path: relative_shard_path.clone(), + first: l.file_name(), + second: r.file_name(), + } + .into_result::<()>() + }); + + let duplicate_check_result = flatten_check_results(duplicate_check_results, |_| ()); + + write_check_result(error_writer, duplicate_check_result)?; + + for package_entry in entries { let package_path = package_entry.path(); let package_name = package_entry.file_name().to_string_lossy().into_owned(); let relative_package_dir = @@ -95,15 +113,6 @@ impl Nixpkgs { 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(), - ))?; - } - let package_name_valid = PACKAGE_NAME_REGEX.is_match(&package_name); let name_check_result = if !package_name_valid { CheckError::InvalidPackageName { From 143e267ad24e7872ed8adede446d2a9f95e4c409 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 20 Oct 2023 01:03:48 +0200 Subject: [PATCH 15/28] tests.nixpkgs-check-by-name: Intermediate PackageNonDir error --- .../nixpkgs-check-by-name/src/check_result.rs | 9 ++ .../nixpkgs-check-by-name/src/structure.rs | 103 +++++++++--------- 2 files changed, 62 insertions(+), 50 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs index 2bceadbf807c..28d48fb59783 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs @@ -8,6 +8,9 @@ use std::io; use std::path::PathBuf; pub enum CheckError { + PackageNonDir { + relative_package_dir: PathBuf, + }, CaseSensitiveDuplicate { relative_shard_path: PathBuf, first: OsString, @@ -89,6 +92,12 @@ impl CheckError { impl fmt::Display for CheckError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { + CheckError::PackageNonDir { relative_package_dir } => + write!( + f, + "{}: This path is a file, but it should be a directory.", + relative_package_dir.display(), + ), CheckError::CaseSensitiveDuplicate { relative_shard_path, first, second } => write!( f, diff --git a/pkgs/test/nixpkgs-check-by-name/src/structure.rs b/pkgs/test/nixpkgs-check-by-name/src/structure.rs index f64f80d381aa..e50f2113e633 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/structure.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/structure.rs @@ -105,69 +105,72 @@ impl Nixpkgs { 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; - } - - let package_name_valid = PACKAGE_NAME_REGEX.is_match(&package_name); - let name_check_result = if !package_name_valid { - CheckError::InvalidPackageName { + let check_result = if !package_path.is_dir() { + CheckError::PackageNonDir { relative_package_dir: relative_package_dir.clone(), - package_name: package_name.clone(), } .into_result() } else { - pass(()) - }; - - let correct_relative_package_dir = Nixpkgs::relative_dir_for_package(&package_name); - let shard_check_result = 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 { - CheckError::IncorrectShard { + let package_name_valid = PACKAGE_NAME_REGEX.is_match(&package_name); + let name_check_result = if !package_name_valid { + CheckError::InvalidPackageName { relative_package_dir: relative_package_dir.clone(), - correct_relative_package_dir: correct_relative_package_dir.clone(), + package_name: package_name.clone(), } .into_result() } else { pass(()) - } - } else { - pass(()) - }; + }; - let package_nix_path = package_path.join(PACKAGE_NIX_FILENAME); - let package_nix_check_result = if !package_nix_path.exists() { - CheckError::PackageNixNonExistent { - relative_package_dir: relative_package_dir.clone(), - } - .into_result() - } else if package_nix_path.is_dir() { - CheckError::PackageNixDir { - relative_package_dir: relative_package_dir.clone(), - } - .into_result() - } else { - pass(()) - }; + let correct_relative_package_dir = + Nixpkgs::relative_dir_for_package(&package_name); + let shard_check_result = 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 { + CheckError::IncorrectShard { + relative_package_dir: relative_package_dir.clone(), + correct_relative_package_dir: correct_relative_package_dir.clone(), + } + .into_result() + } else { + pass(()) + } + } else { + pass(()) + }; - let check_result = flatten_check_results( - [ - name_check_result, - shard_check_result, - package_nix_check_result, - ], - |_| (), - ); + let package_nix_path = package_path.join(PACKAGE_NIX_FILENAME); + let package_nix_check_result = if !package_nix_path.exists() { + CheckError::PackageNixNonExistent { + relative_package_dir: relative_package_dir.clone(), + } + .into_result() + } else if package_nix_path.is_dir() { + CheckError::PackageNixDir { + relative_package_dir: relative_package_dir.clone(), + } + .into_result() + } else { + pass(()) + }; + + let check_result = flatten_check_results( + [ + name_check_result, + shard_check_result, + package_nix_check_result, + ], + |_| (), + ); + + package_names.push(package_name.clone()); + + check_result + }; write_check_result(error_writer, check_result)?; - - package_names.push(package_name.clone()); } } From b7ace0198cfc971a887358bdc8871c6f5c31cfb4 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 20 Oct 2023 01:15:18 +0200 Subject: [PATCH 16/28] tests.nixpkgs-check-by-name: Intermediate InvalidShardName error --- .../nixpkgs-check-by-name/src/check_result.rs | 10 +++++ .../nixpkgs-check-by-name/src/structure.rs | 37 +++++++++++-------- 2 files changed, 31 insertions(+), 16 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs index 28d48fb59783..ad3d949dcb49 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs @@ -8,6 +8,10 @@ use std::io; use std::path::PathBuf; pub enum CheckError { + InvalidShardName { + relative_shard_path: PathBuf, + shard_name: String, + }, PackageNonDir { relative_package_dir: PathBuf, }, @@ -92,6 +96,12 @@ impl CheckError { impl fmt::Display for CheckError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { + CheckError::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() + ), CheckError::PackageNonDir { relative_package_dir } => write!( f, diff --git a/pkgs/test/nixpkgs-check-by-name/src/structure.rs b/pkgs/test/nixpkgs-check-by-name/src/structure.rs index e50f2113e633..fed70c97b090 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/structure.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/structure.rs @@ -71,12 +71,17 @@ impl Nixpkgs { } 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 shard_name_valid_check_result = if !shard_name_valid { + CheckError::InvalidShardName { + relative_shard_path: relative_shard_path.clone(), + shard_name: shard_name.clone(), + } + .into_result() + } else { + pass(()) + }; + + write_check_result(error_writer, shard_name_valid_check_result)?; let entries = utils::read_dir_sorted(&shard_path)?; @@ -99,13 +104,13 @@ impl Nixpkgs { write_check_result(error_writer, duplicate_check_result)?; - for package_entry in entries { + let check_results = entries.into_iter().map(|package_entry| { 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}")); - let check_result = if !package_path.is_dir() { + if !package_path.is_dir() { CheckError::PackageNonDir { relative_package_dir: relative_package_dir.clone(), } @@ -156,21 +161,21 @@ impl Nixpkgs { pass(()) }; - let check_result = flatten_check_results( + flatten_check_results( [ name_check_result, shard_check_result, package_nix_check_result, ], - |_| (), - ); + |_| package_name.clone(), + ) + } + }); - package_names.push(package_name.clone()); + let check_result = flatten_check_results(check_results, |x| x); - check_result - }; - - write_check_result(error_writer, check_result)?; + if let Some(shard_package_names) = write_check_result(error_writer, check_result)? { + package_names.extend(shard_package_names) } } From 571eaed155dced7e29b4e2c6c40f63b0ce521a99 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 20 Oct 2023 01:25:05 +0200 Subject: [PATCH 17/28] tests.nixpkgs-check-by-name: Intermediate ShardNonDir error --- .../nixpkgs-check-by-name/src/check_result.rs | 9 + .../nixpkgs-check-by-name/src/structure.rs | 184 +++++++++--------- 2 files changed, 101 insertions(+), 92 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs index ad3d949dcb49..b970c7d63063 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs @@ -8,6 +8,9 @@ use std::io; use std::path::PathBuf; pub enum CheckError { + ShardNonDir { + relative_shard_path: PathBuf, + }, InvalidShardName { relative_shard_path: PathBuf, shard_name: String, @@ -96,6 +99,12 @@ impl CheckError { impl fmt::Display for CheckError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { + CheckError::ShardNonDir { relative_shard_path } => + write!( + f, + "{}: This is a file, but it should be a directory.", + relative_shard_path.display(), + ), CheckError::InvalidShardName { relative_shard_path, shard_name } => write!( f, diff --git a/pkgs/test/nixpkgs-check-by-name/src/structure.rs b/pkgs/test/nixpkgs-check-by-name/src/structure.rs index fed70c97b090..17eece94d61c 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/structure.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/structure.rs @@ -61,118 +61,118 @@ impl Nixpkgs { 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); - let shard_name_valid_check_result = if !shard_name_valid { - CheckError::InvalidShardName { + let check_result = if !shard_path.is_dir() { + CheckError::ShardNonDir { relative_shard_path: relative_shard_path.clone(), - shard_name: shard_name.clone(), } .into_result() + // we can't check for any other errors if it's a file, since there's no subdirectories to check } else { - pass(()) - }; - - write_check_result(error_writer, shard_name_valid_check_result)?; - - let entries = utils::read_dir_sorted(&shard_path)?; - - let duplicate_check_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)| { - CheckError::CaseSensitiveDuplicate { + let shard_name_valid = SHARD_NAME_REGEX.is_match(&shard_name); + let shard_name_valid_check_result = if !shard_name_valid { + CheckError::InvalidShardName { relative_shard_path: relative_shard_path.clone(), - first: l.file_name(), - second: r.file_name(), - } - .into_result::<()>() - }); - - let duplicate_check_result = flatten_check_results(duplicate_check_results, |_| ()); - - write_check_result(error_writer, duplicate_check_result)?; - - let check_results = entries.into_iter().map(|package_entry| { - 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() { - CheckError::PackageNonDir { - relative_package_dir: relative_package_dir.clone(), + shard_name: shard_name.clone(), } .into_result() } else { - let package_name_valid = PACKAGE_NAME_REGEX.is_match(&package_name); - let name_check_result = if !package_name_valid { - CheckError::InvalidPackageName { + pass(()) + }; + + write_check_result(error_writer, shard_name_valid_check_result)?; + + let entries = utils::read_dir_sorted(&shard_path)?; + + let duplicate_check_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)| { + CheckError::CaseSensitiveDuplicate { + relative_shard_path: relative_shard_path.clone(), + first: l.file_name(), + second: r.file_name(), + } + .into_result::<()>() + }); + + let duplicate_check_result = flatten_check_results(duplicate_check_results, |_| ()); + + write_check_result(error_writer, duplicate_check_result)?; + + let check_results = entries.into_iter().map(|package_entry| { + 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() { + CheckError::PackageNonDir { relative_package_dir: relative_package_dir.clone(), - package_name: package_name.clone(), } .into_result() } else { - pass(()) - }; - - let correct_relative_package_dir = - Nixpkgs::relative_dir_for_package(&package_name); - let shard_check_result = 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 { - CheckError::IncorrectShard { + let package_name_valid = PACKAGE_NAME_REGEX.is_match(&package_name); + let name_check_result = if !package_name_valid { + CheckError::InvalidPackageName { relative_package_dir: relative_package_dir.clone(), - correct_relative_package_dir: correct_relative_package_dir.clone(), + package_name: package_name.clone(), } .into_result() } else { pass(()) - } - } else { - pass(()) - }; + }; - let package_nix_path = package_path.join(PACKAGE_NIX_FILENAME); - let package_nix_check_result = if !package_nix_path.exists() { - CheckError::PackageNixNonExistent { - relative_package_dir: relative_package_dir.clone(), - } - .into_result() - } else if package_nix_path.is_dir() { - CheckError::PackageNixDir { - relative_package_dir: relative_package_dir.clone(), - } - .into_result() - } else { - pass(()) - }; + let correct_relative_package_dir = + Nixpkgs::relative_dir_for_package(&package_name); + let shard_check_result = + 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 { + CheckError::IncorrectShard { + relative_package_dir: relative_package_dir.clone(), + correct_relative_package_dir: correct_relative_package_dir + .clone(), + } + .into_result() + } else { + pass(()) + } + } else { + pass(()) + }; - flatten_check_results( - [ - name_check_result, - shard_check_result, - package_nix_check_result, - ], - |_| package_name.clone(), - ) - } - }); + let package_nix_path = package_path.join(PACKAGE_NIX_FILENAME); + let package_nix_check_result = if !package_nix_path.exists() { + CheckError::PackageNixNonExistent { + relative_package_dir: relative_package_dir.clone(), + } + .into_result() + } else if package_nix_path.is_dir() { + CheckError::PackageNixDir { + relative_package_dir: relative_package_dir.clone(), + } + .into_result() + } else { + pass(()) + }; - let check_result = flatten_check_results(check_results, |x| x); + flatten_check_results( + [ + name_check_result, + shard_check_result, + package_nix_check_result, + ], + |_| package_name.clone(), + ) + } + }); + + flatten_check_results(check_results, |x| x) + }; if let Some(shard_package_names) = write_check_result(error_writer, check_result)? { package_names.extend(shard_package_names) From bb89ca72dfd79d88f7e3de2460c96a0255ebac11 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 20 Oct 2023 01:26:22 +0200 Subject: [PATCH 18/28] tests.nixpkgs-check-by-name: Refactor --- .../nixpkgs-check-by-name/src/structure.rs | 230 +++++++++--------- 1 file changed, 118 insertions(+), 112 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/structure.rs b/pkgs/test/nixpkgs-check-by-name/src/structure.rs index 17eece94d61c..2f225ecd2ec3 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/structure.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/structure.rs @@ -49,139 +49,145 @@ impl Nixpkgs { ) -> anyhow::Result { let base_dir = path.join(BASE_SUBPATH); - let mut package_names = Vec::new(); + let check_results = utils::read_dir_sorted(&base_dir)? + .into_iter() + .map(|shard_entry| { + 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); - for shard_entry in utils::read_dir_sorted(&base_dir)? { - 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); - - if shard_name == "README.md" { - // README.md is allowed to be a file and not checked - continue; - } - - let check_result = if !shard_path.is_dir() { - CheckError::ShardNonDir { - relative_shard_path: relative_shard_path.clone(), - } - .into_result() - // 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 shard_name_valid_check_result = if !shard_name_valid { - CheckError::InvalidShardName { + if shard_name == "README.md" { + // README.md is allowed to be a file and not checked + pass(vec![]) + } else if !shard_path.is_dir() { + CheckError::ShardNonDir { relative_shard_path: relative_shard_path.clone(), - shard_name: shard_name.clone(), } .into_result() + // we can't check for any other errors if it's a file, since there's no subdirectories to check } else { - pass(()) - }; - - write_check_result(error_writer, shard_name_valid_check_result)?; - - let entries = utils::read_dir_sorted(&shard_path)?; - - let duplicate_check_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)| { - CheckError::CaseSensitiveDuplicate { + let shard_name_valid = SHARD_NAME_REGEX.is_match(&shard_name); + let shard_name_valid_check_result = if !shard_name_valid { + CheckError::InvalidShardName { relative_shard_path: relative_shard_path.clone(), - first: l.file_name(), - second: r.file_name(), - } - .into_result::<()>() - }); - - let duplicate_check_result = flatten_check_results(duplicate_check_results, |_| ()); - - write_check_result(error_writer, duplicate_check_result)?; - - let check_results = entries.into_iter().map(|package_entry| { - 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() { - CheckError::PackageNonDir { - relative_package_dir: relative_package_dir.clone(), + shard_name: shard_name.clone(), } .into_result() } else { - let package_name_valid = PACKAGE_NAME_REGEX.is_match(&package_name); - let name_check_result = if !package_name_valid { - CheckError::InvalidPackageName { + pass(()) + }; + + write_check_result(error_writer, shard_name_valid_check_result)?; + + let entries = utils::read_dir_sorted(&shard_path)?; + + let duplicate_check_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)| { + CheckError::CaseSensitiveDuplicate { + relative_shard_path: relative_shard_path.clone(), + first: l.file_name(), + second: r.file_name(), + } + .into_result::<()>() + }); + + let duplicate_check_result = + flatten_check_results(duplicate_check_results, |_| ()); + + write_check_result(error_writer, duplicate_check_result)?; + + let check_results = entries.into_iter().map(|package_entry| { + 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() { + CheckError::PackageNonDir { relative_package_dir: relative_package_dir.clone(), - package_name: package_name.clone(), } .into_result() } else { - pass(()) - }; - - let correct_relative_package_dir = - Nixpkgs::relative_dir_for_package(&package_name); - let shard_check_result = - 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 { - CheckError::IncorrectShard { - relative_package_dir: relative_package_dir.clone(), - correct_relative_package_dir: correct_relative_package_dir - .clone(), - } - .into_result() - } else { - pass(()) + let package_name_valid = PACKAGE_NAME_REGEX.is_match(&package_name); + let name_check_result = if !package_name_valid { + CheckError::InvalidPackageName { + relative_package_dir: relative_package_dir.clone(), + package_name: package_name.clone(), } + .into_result() } else { pass(()) }; - let package_nix_path = package_path.join(PACKAGE_NIX_FILENAME); - let package_nix_check_result = if !package_nix_path.exists() { - CheckError::PackageNixNonExistent { - relative_package_dir: relative_package_dir.clone(), - } - .into_result() - } else if package_nix_path.is_dir() { - CheckError::PackageNixDir { - relative_package_dir: relative_package_dir.clone(), - } - .into_result() - } else { - pass(()) - }; + let correct_relative_package_dir = + Nixpkgs::relative_dir_for_package(&package_name); + let shard_check_result = + 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 { + CheckError::IncorrectShard { + relative_package_dir: relative_package_dir.clone(), + correct_relative_package_dir: + correct_relative_package_dir.clone(), + } + .into_result() + } else { + pass(()) + } + } else { + pass(()) + }; - flatten_check_results( - [ - name_check_result, - shard_check_result, - package_nix_check_result, - ], - |_| package_name.clone(), - ) - } - }); + let package_nix_path = package_path.join(PACKAGE_NIX_FILENAME); + let package_nix_check_result = if !package_nix_path.exists() { + CheckError::PackageNixNonExistent { + relative_package_dir: relative_package_dir.clone(), + } + .into_result() + } else if package_nix_path.is_dir() { + CheckError::PackageNixDir { + relative_package_dir: relative_package_dir.clone(), + } + .into_result() + } else { + pass(()) + }; - flatten_check_results(check_results, |x| x) - }; + flatten_check_results( + [ + name_check_result, + shard_check_result, + package_nix_check_result, + ], + |_| package_name.clone(), + ) + } + }); - if let Some(shard_package_names) = write_check_result(error_writer, check_result)? { - package_names.extend(shard_package_names) - } + flatten_check_results(check_results, |x| x) + } + }); + + let check_result = flatten_check_results(check_results, |x| { + x.into_iter().flatten().collect::>() + }); + + if let Some(package_names) = write_check_result(error_writer, check_result)? { + Ok(Nixpkgs { + path: path.to_owned(), + package_names, + }) + } else { + Ok(Nixpkgs { + path: path.to_owned(), + package_names: vec![], + }) } - - Ok(Nixpkgs { - path: path.to_owned(), - package_names, - }) } } From 83b887504c9a4aaa4e25f89bbf3dc19074fba29e Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 20 Oct 2023 01:54:03 +0200 Subject: [PATCH 19/28] tests.nixpkgs-check-by-name: Support for combining check results --- .../nixpkgs-check-by-name/src/check_result.rs | 12 +++++++++++ .../nixpkgs-check-by-name/src/structure.rs | 21 +++++++++++-------- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs index b970c7d63063..818ba1a9afd7 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs @@ -1,5 +1,6 @@ use crate::utils::PACKAGE_NIX_FILENAME; use crate::ErrorWriter; +use itertools::concat; use itertools::{Either, Itertools}; use rnix::parser::ParseError; use std::ffi::OsString; @@ -245,6 +246,17 @@ pub fn pass(value: A) -> CheckResult { pub type CheckResult = anyhow::Result, A>>; +pub fn sequence_check_results(first: CheckResult<()>, second: CheckResult) -> CheckResult { + match (first?, second?) { + (Either::Right(_), Either::Right(right_value)) => pass(right_value), + (Either::Left(errors), Either::Right(_)) => Ok(Either::Left(errors)), + (Either::Right(_), Either::Left(errors)) => Ok(Either::Left(errors)), + (Either::Left(errors_l), Either::Left(errors_r)) => { + Ok(Either::Left(concat([errors_l, errors_r]))) + } + } +} + pub fn flatten_check_results( check_results: impl IntoIterator>, value_transform: impl Fn(Vec) -> O, diff --git a/pkgs/test/nixpkgs-check-by-name/src/structure.rs b/pkgs/test/nixpkgs-check-by-name/src/structure.rs index 2f225ecd2ec3..d1655dc2e3fe 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/structure.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/structure.rs @@ -1,4 +1,6 @@ -use crate::check_result::{flatten_check_results, pass, write_check_result, CheckError}; +use crate::check_result::{ + flatten_check_results, pass, sequence_check_results, write_check_result, CheckError, +}; use crate::utils; use crate::utils::{ErrorWriter, BASE_SUBPATH, PACKAGE_NIX_FILENAME}; use lazy_static::lazy_static; @@ -67,7 +69,7 @@ impl Nixpkgs { // 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 shard_name_valid_check_result = if !shard_name_valid { + let check_result = if !shard_name_valid { CheckError::InvalidShardName { relative_shard_path: relative_shard_path.clone(), shard_name: shard_name.clone(), @@ -77,8 +79,6 @@ impl Nixpkgs { pass(()) }; - write_check_result(error_writer, shard_name_valid_check_result)?; - let entries = utils::read_dir_sorted(&shard_path)?; let duplicate_check_results = entries @@ -96,10 +96,10 @@ impl Nixpkgs { .into_result::<()>() }); - let duplicate_check_result = - flatten_check_results(duplicate_check_results, |_| ()); - - write_check_result(error_writer, duplicate_check_result)?; + let check_result = sequence_check_results( + check_result, + flatten_check_results(duplicate_check_results, |_| ()), + ); let check_results = entries.into_iter().map(|package_entry| { let package_path = package_entry.path(); @@ -170,7 +170,10 @@ impl Nixpkgs { } }); - flatten_check_results(check_results, |x| x) + sequence_check_results( + check_result, + flatten_check_results(check_results, |x| x), + ) } }); From 0475238ec08c5b903eac37a13efd1b6f4b9617a3 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 20 Oct 2023 01:58:23 +0200 Subject: [PATCH 20/28] tests.nixpkgs-check-by-name: Make structural check a global function --- pkgs/test/nixpkgs-check-by-name/src/main.rs | 6 +- .../nixpkgs-check-by-name/src/structure.rs | 246 ++++++++---------- 2 files changed, 116 insertions(+), 136 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index 7de6eadf6564..6138284d2900 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs @@ -4,6 +4,7 @@ mod references; mod structure; mod utils; +use crate::structure::check_structure; use anyhow::Context; use check_result::{flatten_check_results, write_check_result}; use clap::{Parser, ValueEnum}; @@ -11,7 +12,6 @@ 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 @@ -88,9 +88,9 @@ pub fn check_nixpkgs( utils::BASE_SUBPATH ); } else { - let nixpkgs = Nixpkgs::new(&nixpkgs_path, &mut error_writer)?; + let check_result = check_structure(&nixpkgs_path); - if error_writer.empty { + if let Some(nixpkgs) = write_check_result(&mut error_writer, check_result)? { // Only if we could successfully parse the structure, we do the semantic checks let check_result = flatten_check_results( [ diff --git a/pkgs/test/nixpkgs-check-by-name/src/structure.rs b/pkgs/test/nixpkgs-check-by-name/src/structure.rs index d1655dc2e3fe..24586c6b533c 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/structure.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/structure.rs @@ -1,11 +1,10 @@ use crate::check_result::{ - flatten_check_results, pass, sequence_check_results, write_check_result, CheckError, + flatten_check_results, pass, sequence_check_results, CheckError, CheckResult, }; use crate::utils; -use crate::utils::{ErrorWriter, BASE_SUBPATH, PACKAGE_NIX_FILENAME}; +use crate::utils::{BASE_SUBPATH, PACKAGE_NIX_FILENAME}; use lazy_static::lazy_static; use regex::Regex; -use std::io; use std::path::{Path, PathBuf}; lazy_static! { @@ -42,155 +41,136 @@ impl Nixpkgs { } } -impl Nixpkgs { - /// Read the structure of a Nixpkgs directory, displaying errors on the writer. - /// May return early with I/O errors. - pub fn new( - path: &Path, - error_writer: &mut ErrorWriter, - ) -> anyhow::Result { - let base_dir = path.join(BASE_SUBPATH); +/// Read the structure of a Nixpkgs directory, displaying errors on the writer. +/// May return early with I/O errors. +pub fn check_structure(path: &Path) -> CheckResult { + let base_dir = path.join(BASE_SUBPATH); - let check_results = utils::read_dir_sorted(&base_dir)? - .into_iter() - .map(|shard_entry| { - 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 check_results = utils::read_dir_sorted(&base_dir)? + .into_iter() + .map(|shard_entry| { + 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); - if shard_name == "README.md" { - // README.md is allowed to be a file and not checked - pass(vec![]) - } else if !shard_path.is_dir() { - CheckError::ShardNonDir { + if shard_name == "README.md" { + // README.md is allowed to be a file and not checked + pass(vec![]) + } else if !shard_path.is_dir() { + CheckError::ShardNonDir { + relative_shard_path: relative_shard_path.clone(), + } + .into_result() + // 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 check_result = if !shard_name_valid { + CheckError::InvalidShardName { relative_shard_path: relative_shard_path.clone(), + shard_name: shard_name.clone(), } .into_result() - // 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 check_result = if !shard_name_valid { - CheckError::InvalidShardName { + pass(()) + }; + + let entries = utils::read_dir_sorted(&shard_path)?; + + let duplicate_check_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)| { + CheckError::CaseSensitiveDuplicate { relative_shard_path: relative_shard_path.clone(), - shard_name: shard_name.clone(), + first: l.file_name(), + second: r.file_name(), + } + .into_result::<()>() + }); + + let check_result = sequence_check_results( + check_result, + flatten_check_results(duplicate_check_results, |_| ()), + ); + + let check_results = entries.into_iter().map(|package_entry| { + 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() { + CheckError::PackageNonDir { + relative_package_dir: relative_package_dir.clone(), } .into_result() } else { - pass(()) - }; - - let entries = utils::read_dir_sorted(&shard_path)?; - - let duplicate_check_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)| { - CheckError::CaseSensitiveDuplicate { - relative_shard_path: relative_shard_path.clone(), - first: l.file_name(), - second: r.file_name(), + let package_name_valid = PACKAGE_NAME_REGEX.is_match(&package_name); + let name_check_result = if !package_name_valid { + CheckError::InvalidPackageName { + relative_package_dir: relative_package_dir.clone(), + package_name: package_name.clone(), } - .into_result::<()>() - }); + .into_result() + } else { + pass(()) + }; - let check_result = sequence_check_results( - check_result, - flatten_check_results(duplicate_check_results, |_| ()), - ); + let correct_relative_package_dir = + Nixpkgs::relative_dir_for_package(&package_name); + let shard_check_result = + 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 { + CheckError::IncorrectShard { + relative_package_dir: relative_package_dir.clone(), + correct_relative_package_dir: correct_relative_package_dir + .clone(), + } + .into_result() + } else { + pass(()) + } + } else { + pass(()) + }; - let check_results = entries.into_iter().map(|package_entry| { - 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() { - CheckError::PackageNonDir { + let package_nix_path = package_path.join(PACKAGE_NIX_FILENAME); + let package_nix_check_result = if !package_nix_path.exists() { + CheckError::PackageNixNonExistent { + relative_package_dir: relative_package_dir.clone(), + } + .into_result() + } else if package_nix_path.is_dir() { + CheckError::PackageNixDir { relative_package_dir: relative_package_dir.clone(), } .into_result() } else { - let package_name_valid = PACKAGE_NAME_REGEX.is_match(&package_name); - let name_check_result = if !package_name_valid { - CheckError::InvalidPackageName { - relative_package_dir: relative_package_dir.clone(), - package_name: package_name.clone(), - } - .into_result() - } else { - pass(()) - }; + pass(()) + }; - let correct_relative_package_dir = - Nixpkgs::relative_dir_for_package(&package_name); - let shard_check_result = - 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 { - CheckError::IncorrectShard { - relative_package_dir: relative_package_dir.clone(), - correct_relative_package_dir: - correct_relative_package_dir.clone(), - } - .into_result() - } else { - pass(()) - } - } else { - pass(()) - }; + flatten_check_results( + [ + name_check_result, + shard_check_result, + package_nix_check_result, + ], + |_| package_name.clone(), + ) + } + }); - let package_nix_path = package_path.join(PACKAGE_NIX_FILENAME); - let package_nix_check_result = if !package_nix_path.exists() { - CheckError::PackageNixNonExistent { - relative_package_dir: relative_package_dir.clone(), - } - .into_result() - } else if package_nix_path.is_dir() { - CheckError::PackageNixDir { - relative_package_dir: relative_package_dir.clone(), - } - .into_result() - } else { - pass(()) - }; - - flatten_check_results( - [ - name_check_result, - shard_check_result, - package_nix_check_result, - ], - |_| package_name.clone(), - ) - } - }); - - sequence_check_results( - check_result, - flatten_check_results(check_results, |x| x), - ) - } - }); - - let check_result = flatten_check_results(check_results, |x| { - x.into_iter().flatten().collect::>() + sequence_check_results(check_result, flatten_check_results(check_results, |x| x)) + } }); - if let Some(package_names) = write_check_result(error_writer, check_result)? { - Ok(Nixpkgs { - path: path.to_owned(), - package_names, - }) - } else { - Ok(Nixpkgs { - path: path.to_owned(), - package_names: vec![], - }) - } - } + flatten_check_results(check_results, |x| Nixpkgs { + path: path.to_owned(), + package_names: x.into_iter().flatten().collect::>(), + }) } From d65f3ddb890570dea21df41e386d0f6401c9ec3a Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 20 Oct 2023 02:18:21 +0200 Subject: [PATCH 21/28] tests.nixpkgs-check-by-name: Make reference check part of structural check --- pkgs/test/nixpkgs-check-by-name/src/main.rs | 12 ++----- .../nixpkgs-check-by-name/src/references.rs | 31 +++++++++---------- .../nixpkgs-check-by-name/src/structure.rs | 7 +++++ 3 files changed, 24 insertions(+), 26 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index 6138284d2900..ff223068697a 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs @@ -6,7 +6,7 @@ mod utils; use crate::structure::check_structure; use anyhow::Context; -use check_result::{flatten_check_results, write_check_result}; +use check_result::write_check_result; use clap::{Parser, ValueEnum}; use colored::Colorize; use std::io; @@ -91,14 +91,8 @@ pub fn check_nixpkgs( let check_result = check_structure(&nixpkgs_path); if let Some(nixpkgs) = write_check_result(&mut error_writer, check_result)? { - // Only if we could successfully parse the structure, we do the semantic checks - let check_result = flatten_check_results( - [ - eval::check_values(version, &nixpkgs, eval_accessible_paths), - references::check_references(&nixpkgs), - ], - |_| (), - ); + // Only if we could successfully parse the structure, we do the evaluation checks + let check_result = eval::check_values(version, &nixpkgs, eval_accessible_paths); write_check_result(&mut error_writer, check_result)?; } } diff --git a/pkgs/test/nixpkgs-check-by-name/src/references.rs b/pkgs/test/nixpkgs-check-by-name/src/references.rs index b932b556f54f..91f898fe3bfd 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/references.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/references.rs @@ -1,5 +1,4 @@ use crate::check_result::{flatten_check_results, pass, CheckError, CheckResult}; -use crate::structure::Nixpkgs; use crate::utils; use crate::utils::LineIndex; @@ -19,23 +18,21 @@ struct PackageContext<'a> { /// 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(nixpkgs: &Nixpkgs) -> CheckResult<()> { - // Check the directories for each package separately - let check_results = nixpkgs.package_names.iter().map(|package_name| { - let relative_package_dir = Nixpkgs::relative_dir_for_package(package_name); - let context = PackageContext { - relative_package_dir: &relative_package_dir, - absolute_package_dir: &nixpkgs.path.join(&relative_package_dir), - }; +pub fn check_references( + relative_package_dir: &Path, + absolute_package_dir: &Path, +) -> CheckResult<()> { + let context = PackageContext { + relative_package_dir: &relative_package_dir.to_path_buf(), + absolute_package_dir: &absolute_package_dir.to_path_buf(), + }; - // The empty argument here is the subpath under the package directory to check - // An empty one means the package directory itself - check_path(&context, Path::new("")).context(format!( - "While checking the references in package directory {}", - relative_package_dir.display() - )) - }); - flatten_check_results(check_results, |_| ()) + // The empty argument here is the subpath under the package directory to check + // An empty one means the package directory itself + check_path(&context, 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 diff --git a/pkgs/test/nixpkgs-check-by-name/src/structure.rs b/pkgs/test/nixpkgs-check-by-name/src/structure.rs index 24586c6b533c..09ec798e440e 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/structure.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/structure.rs @@ -1,6 +1,7 @@ use crate::check_result::{ flatten_check_results, pass, sequence_check_results, CheckError, CheckResult, }; +use crate::references; use crate::utils; use crate::utils::{BASE_SUBPATH, PACKAGE_NIX_FILENAME}; use lazy_static::lazy_static; @@ -154,11 +155,17 @@ pub fn check_structure(path: &Path) -> CheckResult { pass(()) }; + let reference_check_result = references::check_references( + &relative_package_dir, + &path.join(&relative_package_dir), + ); + flatten_check_results( [ name_check_result, shard_check_result, package_nix_check_result, + reference_check_result, ], |_| package_name.clone(), ) From e58bc75444d5d722a995cf350b0d6c298aeb3808 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 20 Oct 2023 02:22:42 +0200 Subject: [PATCH 22/28] tests.nixpkgs-check-by-name: Remove Nixpkgs struct Isn't necessary anymore with the refactoring --- pkgs/test/nixpkgs-check-by-name/src/eval.rs | 15 +++--- pkgs/test/nixpkgs-check-by-name/src/main.rs | 5 +- .../nixpkgs-check-by-name/src/structure.rs | 47 +++++++------------ 3 files changed, 28 insertions(+), 39 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index eb90a295a577..487ec7cab730 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs @@ -41,7 +41,8 @@ const EXPR: &str = include_str!("eval.nix"); /// See the `eval.nix` file for how this is achieved on the Nix side pub fn check_values( version: Version, - nixpkgs: &structure::Nixpkgs, + nixpkgs_path: &Path, + package_names: Vec, eval_accessible_paths: Vec<&Path>, ) -> CheckResult<()> { // Write the list of packages we need to check into a temporary JSON file. @@ -53,7 +54,7 @@ pub fn check_values( // 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() ))?; @@ -85,9 +86,9 @@ pub fn check_values( .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 { @@ -109,9 +110,9 @@ pub fn check_values( String::from_utf8_lossy(&result.stdout) ))?; - let check_results = nixpkgs.package_names.iter().map(|package_name| { - let relative_package_file = structure::Nixpkgs::relative_file_for_package(package_name); - let absolute_package_file = nixpkgs.path.join(&relative_package_file); + let check_results = 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 valid = match &attribute_info.variant { diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index ff223068697a..67f6055e7338 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs @@ -90,9 +90,10 @@ pub fn check_nixpkgs( } else { let check_result = check_structure(&nixpkgs_path); - if let Some(nixpkgs) = write_check_result(&mut error_writer, check_result)? { + if let Some(package_names) = write_check_result(&mut error_writer, check_result)? { // Only if we could successfully parse the structure, we do the evaluation checks - let check_result = eval::check_values(version, &nixpkgs, eval_accessible_paths); + let check_result = + eval::check_values(version, &nixpkgs_path, package_names, eval_accessible_paths); write_check_result(&mut error_writer, check_result)?; } } diff --git a/pkgs/test/nixpkgs-check-by-name/src/structure.rs b/pkgs/test/nixpkgs-check-by-name/src/structure.rs index 09ec798e440e..c8fbc17fcb39 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/structure.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/structure.rs @@ -13,38 +13,27 @@ lazy_static! { 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, +// Some utility functions for the basic structure + +pub fn shard_for_package(package_name: &str) -> String { + package_name.to_lowercase().chars().take(2).collect() } -impl Nixpkgs { - // Some utility functions for the basic structure +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 { + relative_dir_for_shard(&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_dir_for_package(package_name: &str) -> PathBuf { - Nixpkgs::relative_dir_for_shard(&Nixpkgs::shard_for_package(package_name)) - .join(package_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_file_for_package(package_name: &str) -> PathBuf { + relative_dir_for_package(package_name).join(PACKAGE_NIX_FILENAME) } /// Read the structure of a Nixpkgs directory, displaying errors on the writer. /// May return early with I/O errors. -pub fn check_structure(path: &Path) -> CheckResult { +pub fn check_structure(path: &Path) -> CheckResult> { let base_dir = path.join(BASE_SUBPATH); let check_results = utils::read_dir_sorted(&base_dir)? @@ -52,7 +41,7 @@ pub fn check_structure(path: &Path) -> CheckResult { .map(|shard_entry| { 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" { // README.md is allowed to be a file and not checked @@ -120,8 +109,7 @@ pub fn check_structure(path: &Path) -> CheckResult { pass(()) }; - let correct_relative_package_dir = - Nixpkgs::relative_dir_for_package(&package_name); + let correct_relative_package_dir = relative_dir_for_package(&package_name); let shard_check_result = if relative_package_dir != correct_relative_package_dir { // Only show this error if we have a valid shard and package name @@ -176,8 +164,7 @@ pub fn check_structure(path: &Path) -> CheckResult { } }); - flatten_check_results(check_results, |x| Nixpkgs { - path: path.to_owned(), - package_names: x.into_iter().flatten().collect::>(), + flatten_check_results(check_results, |x| { + x.into_iter().flatten().collect::>() }) } From 3d60440799721601b04ff39e83374aa684a5300b Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 20 Oct 2023 02:45:54 +0200 Subject: [PATCH 23/28] tests.nixpkgs-check-by-name: Remove error writer --- .../nixpkgs-check-by-name/src/check_result.rs | 16 --------- pkgs/test/nixpkgs-check-by-name/src/main.rs | 35 +++++++++++-------- pkgs/test/nixpkgs-check-by-name/src/utils.rs | 24 ------------- 3 files changed, 21 insertions(+), 54 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs index 818ba1a9afd7..e5f14152d5ac 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs @@ -1,5 +1,4 @@ use crate::utils::PACKAGE_NIX_FILENAME; -use crate::ErrorWriter; use itertools::concat; use itertools::{Either, Itertools}; use rnix::parser::ParseError; @@ -225,21 +224,6 @@ impl fmt::Display for CheckError { } } -pub fn write_check_result( - error_writer: &mut ErrorWriter, - check_result: CheckResult, -) -> anyhow::Result> { - match check_result? { - Either::Left(errors) => { - for error in errors { - error_writer.write(&error.to_string())? - } - Ok(None) - } - Either::Right(value) => Ok(Some(value)), - } -} - pub fn pass(value: A) -> CheckResult { Ok(Either::Right(value)) } diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index 67f6055e7338..537276a177d5 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs @@ -6,13 +6,14 @@ mod utils; use crate::structure::check_structure; use anyhow::Context; -use check_result::write_check_result; +use check_result::pass; use clap::{Parser, ValueEnum}; use colored::Colorize; +use itertools::Either; +use itertools::Either::{Left, Right}; use std::io; use std::path::{Path, PathBuf}; use std::process::ExitCode; -use utils::ErrorWriter; /// Program to check the validity of pkgs/by-name #[derive(Parser, Debug)] @@ -78,26 +79,32 @@ pub fn check_nixpkgs( 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(utils::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.", utils::BASE_SUBPATH ); + pass(()) } else { - let check_result = check_structure(&nixpkgs_path); - - if let Some(package_names) = write_check_result(&mut error_writer, check_result)? { + match check_structure(&nixpkgs_path)? { + Left(errors) => Ok(Left(errors)), + Right(package_names) => // Only if we could successfully parse the structure, we do the evaluation checks - let check_result = - eval::check_values(version, &nixpkgs_path, package_names, eval_accessible_paths); - write_check_result(&mut error_writer, check_result)?; + { + eval::check_values(version, &nixpkgs_path, package_names, eval_accessible_paths) + } } + }; + + match check_result? { + Either::Left(errors) => { + for error in errors { + writeln!(error_writer, "{}", error.to_string().red())? + } + Ok(false) + } + Either::Right(_) => Ok(true), } - Ok(error_writer.empty) } #[cfg(test)] diff --git a/pkgs/test/nixpkgs-check-by-name/src/utils.rs b/pkgs/test/nixpkgs-check-by-name/src/utils.rs index 728e50e72336..5cc4a0863ba8 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/utils.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/utils.rs @@ -1,5 +1,4 @@ use anyhow::Context; -use colored::Colorize; use std::fs; use std::io; use std::path::Path; @@ -50,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 { - pub writer: W, - pub empty: bool, -} - -impl ErrorWriter { - pub fn new(writer: W) -> ErrorWriter { - ErrorWriter { - writer, - empty: true, - } - } - - pub fn write(&mut self, string: &str) -> io::Result<()> { - self.empty = false; - writeln!(self.writer, "{}", string.red()) - } -} From eac0b69063c0706d3a0bc956a1efc04745c14594 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Mon, 23 Oct 2023 23:54:02 +0200 Subject: [PATCH 24/28] tests.nixpkgs-check-by-name: Redesign and document check_result functions --- .../nixpkgs-check-by-name/src/check_result.rs | 104 ++++++++++------- pkgs/test/nixpkgs-check-by-name/src/eval.rs | 16 +-- pkgs/test/nixpkgs-check-by-name/src/main.rs | 9 +- .../nixpkgs-check-by-name/src/references.rs | 37 +++--- .../nixpkgs-check-by-name/src/structure.rs | 106 +++++++++--------- 5 files changed, 147 insertions(+), 125 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs index e5f14152d5ac..db2377c7204b 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs @@ -1,13 +1,17 @@ use crate::utils::PACKAGE_NIX_FILENAME; use itertools::concat; -use itertools::{Either, Itertools}; +use itertools::{ + Either, + Either::{Left, Right}, + Itertools, +}; use rnix::parser::ParseError; use std::ffi::OsString; use std::fmt; use std::io; use std::path::PathBuf; -pub enum CheckError { +pub enum CheckProblem { ShardNonDir { relative_shard_path: PathBuf, }, @@ -90,97 +94,97 @@ pub enum CheckError { }, } -impl CheckError { +impl CheckProblem { pub fn into_result(self) -> CheckResult { - Ok(Either::Left(vec![self])) + Ok(Left(vec![self])) } } -impl fmt::Display for CheckError { +impl fmt::Display for CheckProblem { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { - CheckError::ShardNonDir { relative_shard_path } => + CheckProblem::ShardNonDir { relative_shard_path } => write!( f, "{}: This is a file, but it should be a directory.", relative_shard_path.display(), ), - CheckError::InvalidShardName { relative_shard_path, shard_name } => + CheckProblem::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() ), - CheckError::PackageNonDir { relative_package_dir } => + CheckProblem::PackageNonDir { relative_package_dir } => write!( f, "{}: This path is a file, but it should be a directory.", relative_package_dir.display(), ), - CheckError::CaseSensitiveDuplicate { relative_shard_path, first, second } => + CheckProblem::CaseSensitiveDuplicate { relative_shard_path, first, second } => write!( f, "{}: Duplicate case-sensitive package directories {first:?} and {second:?}.", relative_shard_path.display(), ), - CheckError::InvalidPackageName { relative_package_dir, package_name } => + CheckProblem::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(), ), - CheckError::IncorrectShard { relative_package_dir, correct_relative_package_dir } => + CheckProblem::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(), ), - CheckError::PackageNixNonExistent { relative_package_dir } => + CheckProblem::PackageNixNonExistent { relative_package_dir } => write!( f, "{}: Missing required \"{PACKAGE_NIX_FILENAME}\" file.", relative_package_dir.display(), ), - CheckError::PackageNixDir { relative_package_dir } => + CheckProblem::PackageNixDir { relative_package_dir } => write!( f, "{}: \"{PACKAGE_NIX_FILENAME}\" must be a file.", relative_package_dir.display(), ), - CheckError::UndefinedAttr { relative_package_file, package_name } => + CheckProblem::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() ), - CheckError::WrongCallPackage { relative_package_file, package_name } => + CheckProblem::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() ), - CheckError::NonDerivation { relative_package_file, package_name } => + CheckProblem::NonDerivation { relative_package_file, package_name } => write!( f, "pkgs.{package_name}: This attribute defined by {} is not a derivation", relative_package_file.display() ), - CheckError::OutsideSymlink { relative_package_dir, subpath } => + CheckProblem::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(), ), - CheckError::UnresolvableSymlink { relative_package_dir, subpath, io_error } => + CheckProblem::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(), ), - CheckError::CouldNotParseNix { relative_package_dir, subpath, error } => + CheckProblem::CouldNotParseNix { relative_package_dir, subpath, error } => write!( f, "{}: File {} could not be parsed by rnix: {}", @@ -188,7 +192,7 @@ impl fmt::Display for CheckError { subpath.display(), error, ), - CheckError::PathInterpolation { relative_package_dir, subpath, line, text } => + CheckProblem::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.", @@ -196,7 +200,7 @@ impl fmt::Display for CheckError { subpath.display(), text ), - CheckError::SearchPath { relative_package_dir, subpath, line, text } => + CheckProblem::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.", @@ -204,7 +208,7 @@ impl fmt::Display for CheckError { subpath.display(), text ), - CheckError::OutsidePathReference { relative_package_dir, subpath, line, text } => + CheckProblem::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.", @@ -212,7 +216,7 @@ impl fmt::Display for CheckError { subpath.display(), text, ), - CheckError::UnresolvablePathReference { relative_package_dir, subpath, line, text, io_error } => + CheckProblem::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}.", @@ -224,27 +228,44 @@ impl fmt::Display for CheckError { } } -pub fn pass(value: A) -> CheckResult { - Ok(Either::Right(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(Left(Vec)): A non-fatal 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(Right(A)): A successful (potentially intermediate) result with an arbitrary value. +/// No fatal errors have occurred and no problems have been found with Nixpkgs. +pub type CheckResult = anyhow::Result, A>>; + +/// Map a `CheckResult` to a `CheckResult` by applying a function to the +/// potentially contained value in case of success. +pub fn map(check_result: CheckResult, f: impl FnOnce(I) -> O) -> CheckResult { + Ok(check_result?.map_right(f)) } -pub type CheckResult = anyhow::Result, A>>; +/// Create a successfull `CheckResult` from a value `A`. +pub fn ok(value: A) -> CheckResult { + Ok(Right(value)) +} -pub fn sequence_check_results(first: CheckResult<()>, second: CheckResult) -> CheckResult { +/// Combine two check results, both of which need to be successful for the return value to be successful. +/// The `CheckProblem`s of both sides are returned concatenated. +pub fn and(first: CheckResult<()>, second: CheckResult) -> CheckResult { match (first?, second?) { - (Either::Right(_), Either::Right(right_value)) => pass(right_value), - (Either::Left(errors), Either::Right(_)) => Ok(Either::Left(errors)), - (Either::Right(_), Either::Left(errors)) => Ok(Either::Left(errors)), - (Either::Left(errors_l), Either::Left(errors_r)) => { - Ok(Either::Left(concat([errors_l, errors_r]))) - } + (Right(_), Right(right_value)) => Ok(Right(right_value)), + (Left(errors), Right(_)) => Ok(Left(errors)), + (Right(_), Left(errors)) => Ok(Left(errors)), + (Left(errors_l), Left(errors_r)) => Ok(Left(concat([errors_l, errors_r]))), } } -pub fn flatten_check_results( - check_results: impl IntoIterator>, - value_transform: impl Fn(Vec) -> O, -) -> CheckResult { +/// Combine many checks results into a single one. +/// All given check results need to be successful in order for the returned check result to be successful, +/// in which case the returned check result value contains each a `Vec` of each individual result value. +/// The `CheckProblem`s of all results are returned concatenated. +pub fn sequence(check_results: impl IntoIterator>) -> CheckResult> { let (errors, values): (Vec<_>, Vec<_>) = check_results .into_iter() .collect::>>()? @@ -256,8 +277,13 @@ pub fn flatten_check_results( let flattened_errors = errors.into_iter().flatten().collect::>(); if flattened_errors.is_empty() { - Ok(Either::Right(value_transform(values))) + Ok(Right(values)) } else { - Ok(Either::Left(flattened_errors)) + Ok(Left(flattened_errors)) } } + +/// Like `sequence`, but replace the resulting value with () +pub fn sequence_(check_results: impl IntoIterator>) -> CheckResult<()> { + map(sequence(check_results), |_| ()) +} diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index 487ec7cab730..a76abba05d96 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs @@ -1,4 +1,5 @@ -use crate::check_result::{flatten_check_results, pass, CheckError, CheckResult}; +use crate::check_result; +use crate::check_result::{CheckProblem, CheckResult}; use crate::structure; use crate::Version; use std::path::Path; @@ -110,7 +111,7 @@ pub fn check_values( String::from_utf8_lossy(&result.stdout) ))?; - let check_results = package_names.iter().map(|package_name| { + check_result::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); @@ -136,27 +137,26 @@ pub fn check_values( }; if !valid { - CheckError::WrongCallPackage { + CheckProblem::WrongCallPackage { relative_package_file: relative_package_file.clone(), package_name: package_name.clone(), } .into_result() } else if !attribute_info.is_derivation { - CheckError::NonDerivation { + CheckProblem::NonDerivation { relative_package_file: relative_package_file.clone(), package_name: package_name.clone(), } .into_result() } else { - pass(()) + check_result::ok(()) } } else { - CheckError::UndefinedAttr { + CheckProblem::UndefinedAttr { relative_package_file: relative_package_file.clone(), package_name: package_name.clone(), } .into_result() } - }); - flatten_check_results(check_results, |_| ()) + })) } diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index 537276a177d5..11fedde74130 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs @@ -6,11 +6,12 @@ mod utils; use crate::structure::check_structure; use anyhow::Context; -use check_result::pass; use clap::{Parser, ValueEnum}; use colored::Colorize; -use itertools::Either; -use itertools::Either::{Left, Right}; +use itertools::{ + Either, + Either::{Left, Right}, +}; use std::io; use std::path::{Path, PathBuf}; use std::process::ExitCode; @@ -84,7 +85,7 @@ pub fn check_nixpkgs( "Given Nixpkgs path does not contain a {} subdirectory, no check necessary.", utils::BASE_SUBPATH ); - pass(()) + check_result::ok(()) } else { match check_structure(&nixpkgs_path)? { Left(errors) => Ok(Left(errors)), diff --git a/pkgs/test/nixpkgs-check-by-name/src/references.rs b/pkgs/test/nixpkgs-check-by-name/src/references.rs index 91f898fe3bfd..a6bc53d717d7 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/references.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/references.rs @@ -1,4 +1,5 @@ -use crate::check_result::{flatten_check_results, pass, CheckError, CheckResult}; +use crate::check_result; +use crate::check_result::{CheckProblem, CheckResult}; use crate::utils; use crate::utils::LineIndex; @@ -46,16 +47,16 @@ fn check_path(context: &PackageContext, subpath: &Path) -> CheckResult<()> { // 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) { - CheckError::OutsideSymlink { + CheckProblem::OutsideSymlink { relative_package_dir: context.relative_package_dir.clone(), subpath: subpath.to_path_buf(), } .into_result() } else { - pass(()) + check_result::ok(()) } } - Err(io_error) => CheckError::UnresolvableSymlink { + Err(io_error) => CheckProblem::UnresolvableSymlink { relative_package_dir: context.relative_package_dir.clone(), subpath: subpath.to_path_buf(), io_error, @@ -64,12 +65,11 @@ fn check_path(context: &PackageContext, subpath: &Path) -> CheckResult<()> { } } else if path.is_dir() { // Recursively check each entry - let check_results = utils::read_dir_sorted(&path)?.into_iter().map(|entry| { + check_result::sequence_(utils::read_dir_sorted(&path)?.into_iter().map(|entry| { let entry_subpath = subpath.join(entry.file_name()); check_path(context, &entry_subpath) .context(format!("Error while recursing into {}", subpath.display())) - }); - flatten_check_results(check_results, |_| ()) + })) } else if path.is_file() { // Only check Nix files if let Some(ext) = path.extension() { @@ -79,10 +79,10 @@ fn check_path(context: &PackageContext, subpath: &Path) -> CheckResult<()> { subpath.display() )) } else { - pass(()) + check_result::ok(()) } } else { - pass(()) + check_result::ok(()) } } else { // This should never happen, git doesn't support other file types @@ -104,7 +104,7 @@ fn check_nix_file(context: &PackageContext, subpath: &Path) -> CheckResult<()> { let root = Root::parse(&contents); if let Some(error) = root.errors().first() { - return CheckError::CouldNotParseNix { + return CheckProblem::CouldNotParseNix { relative_package_dir: context.relative_package_dir.clone(), subpath: subpath.to_path_buf(), error: error.clone(), @@ -114,17 +114,17 @@ fn check_nix_file(context: &PackageContext, subpath: &Path) -> CheckResult<()> { let line_index = LineIndex::new(&contents); - let check_results = root.syntax().descendants().map(|node| { + check_result::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 - pass(()) + check_result::ok(()) } else if node.children().count() != 0 { // Filters out ./foo/${bar}/baz // TODO: We can just check ./foo - CheckError::PathInterpolation { + CheckProblem::PathInterpolation { relative_package_dir: context.relative_package_dir.clone(), subpath: subpath.to_path_buf(), line, @@ -133,7 +133,7 @@ fn check_nix_file(context: &PackageContext, subpath: &Path) -> CheckResult<()> { .into_result() } else if text.starts_with('<') { // Filters out search paths like - CheckError::SearchPath { + CheckProblem::SearchPath { relative_package_dir: context.relative_package_dir.clone(), subpath: subpath.to_path_buf(), line, @@ -149,7 +149,7 @@ fn check_nix_file(context: &PackageContext, subpath: &Path) -> CheckResult<()> { // 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) { - CheckError::OutsidePathReference { + CheckProblem::OutsidePathReference { relative_package_dir: context.relative_package_dir.clone(), subpath: subpath.to_path_buf(), line, @@ -157,10 +157,10 @@ fn check_nix_file(context: &PackageContext, subpath: &Path) -> CheckResult<()> { } .into_result() } else { - pass(()) + check_result::ok(()) } } - Err(e) => CheckError::UnresolvablePathReference { + Err(e) => CheckProblem::UnresolvablePathReference { relative_package_dir: context.relative_package_dir.clone(), subpath: subpath.to_path_buf(), line, @@ -170,6 +170,5 @@ fn check_nix_file(context: &PackageContext, subpath: &Path) -> CheckResult<()> { .into_result(), } } - }); - flatten_check_results(check_results, |_| ()) + })) } diff --git a/pkgs/test/nixpkgs-check-by-name/src/structure.rs b/pkgs/test/nixpkgs-check-by-name/src/structure.rs index c8fbc17fcb39..90a552a38863 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/structure.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/structure.rs @@ -1,6 +1,5 @@ -use crate::check_result::{ - flatten_check_results, pass, sequence_check_results, CheckError, CheckResult, -}; +use crate::check_result; +use crate::check_result::{CheckProblem, CheckResult}; use crate::references; use crate::utils; use crate::utils::{BASE_SUBPATH, PACKAGE_NIX_FILENAME}; @@ -36,7 +35,7 @@ pub fn relative_file_for_package(package_name: &str) -> PathBuf { pub fn check_structure(path: &Path) -> CheckResult> { let base_dir = path.join(BASE_SUBPATH); - let check_results = utils::read_dir_sorted(&base_dir)? + let shard_results = utils::read_dir_sorted(&base_dir)? .into_iter() .map(|shard_entry| { let shard_path = shard_entry.path(); @@ -45,35 +44,35 @@ pub fn check_structure(path: &Path) -> CheckResult> { if shard_name == "README.md" { // README.md is allowed to be a file and not checked - pass(vec![]) + check_result::ok(vec![]) } else if !shard_path.is_dir() { - CheckError::ShardNonDir { + CheckProblem::ShardNonDir { relative_shard_path: relative_shard_path.clone(), } .into_result() // 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 check_result = if !shard_name_valid { - CheckError::InvalidShardName { + let result = if !shard_name_valid { + CheckProblem::InvalidShardName { relative_shard_path: relative_shard_path.clone(), shard_name: shard_name.clone(), } .into_result() } else { - pass(()) + check_result::ok(()) }; let entries = utils::read_dir_sorted(&shard_path)?; - let duplicate_check_results = entries + 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)| { - CheckError::CaseSensitiveDuplicate { + CheckProblem::CaseSensitiveDuplicate { relative_shard_path: relative_shard_path.clone(), first: l.file_name(), second: r.file_name(), @@ -81,90 +80,87 @@ pub fn check_structure(path: &Path) -> CheckResult> { .into_result::<()>() }); - let check_result = sequence_check_results( - check_result, - flatten_check_results(duplicate_check_results, |_| ()), - ); + let result = check_result::and(result, check_result::sequence_(duplicate_results)); - let check_results = entries.into_iter().map(|package_entry| { + let package_results = entries.into_iter().map(|package_entry| { 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() { - CheckError::PackageNonDir { + CheckProblem::PackageNonDir { relative_package_dir: relative_package_dir.clone(), } .into_result() } else { let package_name_valid = PACKAGE_NAME_REGEX.is_match(&package_name); - let name_check_result = if !package_name_valid { - CheckError::InvalidPackageName { + let result = if !package_name_valid { + CheckProblem::InvalidPackageName { relative_package_dir: relative_package_dir.clone(), package_name: package_name.clone(), } .into_result() } else { - pass(()) + check_result::ok(()) }; let correct_relative_package_dir = relative_dir_for_package(&package_name); - let shard_check_result = + let result = check_result::and( + result, 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 { - CheckError::IncorrectShard { + CheckProblem::IncorrectShard { relative_package_dir: relative_package_dir.clone(), correct_relative_package_dir: correct_relative_package_dir .clone(), } .into_result() } else { - pass(()) + check_result::ok(()) } } else { - pass(()) - }; - - let package_nix_path = package_path.join(PACKAGE_NIX_FILENAME); - let package_nix_check_result = if !package_nix_path.exists() { - CheckError::PackageNixNonExistent { - relative_package_dir: relative_package_dir.clone(), - } - .into_result() - } else if package_nix_path.is_dir() { - CheckError::PackageNixDir { - relative_package_dir: relative_package_dir.clone(), - } - .into_result() - } else { - pass(()) - }; - - let reference_check_result = references::check_references( - &relative_package_dir, - &path.join(&relative_package_dir), + check_result::ok(()) + }, ); - flatten_check_results( - [ - name_check_result, - shard_check_result, - package_nix_check_result, - reference_check_result, - ], - |_| package_name.clone(), - ) + let package_nix_path = package_path.join(PACKAGE_NIX_FILENAME); + let result = check_result::and( + result, + if !package_nix_path.exists() { + CheckProblem::PackageNixNonExistent { + relative_package_dir: relative_package_dir.clone(), + } + .into_result() + } else if package_nix_path.is_dir() { + CheckProblem::PackageNixDir { + relative_package_dir: relative_package_dir.clone(), + } + .into_result() + } else { + check_result::ok(()) + }, + ); + + let result = check_result::and( + result, + references::check_references( + &relative_package_dir, + &path.join(&relative_package_dir), + ), + ); + + check_result::map(result, |_| package_name.clone()) } }); - sequence_check_results(check_result, flatten_check_results(check_results, |x| x)) + check_result::and(result, check_result::sequence(package_results)) } }); - flatten_check_results(check_results, |x| { + check_result::map(check_result::sequence(shard_results), |x| { x.into_iter().flatten().collect::>() }) } From 8be41ace99f26479ba1d5a7a0c413e0885113158 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Mon, 23 Oct 2023 23:58:51 +0200 Subject: [PATCH 25/28] tests.nixpkgs-check-by-name: Separate file for all problems And introduce a function for some smaller indentation --- .../nixpkgs-check-by-name/src/check_result.rs | 239 +----------------- pkgs/test/nixpkgs-check-by-name/src/eval.rs | 9 +- pkgs/test/nixpkgs-check-by-name/src/main.rs | 1 + .../src/nixpkgs_problem.rs | 218 ++++++++++++++++ .../nixpkgs-check-by-name/src/references.rs | 17 +- .../nixpkgs-check-by-name/src/structure.rs | 166 ++++++------ 6 files changed, 331 insertions(+), 319 deletions(-) create mode 100644 pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs diff --git a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs index db2377c7204b..a6538d778b61 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs @@ -1,243 +1,21 @@ -use crate::utils::PACKAGE_NIX_FILENAME; +use crate::nixpkgs_problem::NixpkgsProblem; use itertools::concat; use itertools::{ Either, Either::{Left, Right}, Itertools, }; -use rnix::parser::ParseError; -use std::ffi::OsString; -use std::fmt; -use std::io; -use std::path::PathBuf; - -pub enum CheckProblem { - 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 CheckProblem { - pub fn into_result(self) -> CheckResult { - Ok(Left(vec![self])) - } -} - -impl fmt::Display for CheckProblem { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - match self { - CheckProblem::ShardNonDir { relative_shard_path } => - write!( - f, - "{}: This is a file, but it should be a directory.", - relative_shard_path.display(), - ), - CheckProblem::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() - ), - CheckProblem::PackageNonDir { relative_package_dir } => - write!( - f, - "{}: This path is a file, but it should be a directory.", - relative_package_dir.display(), - ), - CheckProblem::CaseSensitiveDuplicate { relative_shard_path, first, second } => - write!( - f, - "{}: Duplicate case-sensitive package directories {first:?} and {second:?}.", - relative_shard_path.display(), - ), - CheckProblem::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(), - ), - CheckProblem::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(), - ), - CheckProblem::PackageNixNonExistent { relative_package_dir } => - write!( - f, - "{}: Missing required \"{PACKAGE_NIX_FILENAME}\" file.", - relative_package_dir.display(), - ), - CheckProblem::PackageNixDir { relative_package_dir } => - write!( - f, - "{}: \"{PACKAGE_NIX_FILENAME}\" must be a file.", - relative_package_dir.display(), - ), - CheckProblem::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() - ), - CheckProblem::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() - ), - CheckProblem::NonDerivation { relative_package_file, package_name } => - write!( - f, - "pkgs.{package_name}: This attribute defined by {} is not a derivation", - relative_package_file.display() - ), - CheckProblem::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(), - ), - CheckProblem::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(), - ), - CheckProblem::CouldNotParseNix { relative_package_dir, subpath, error } => - write!( - f, - "{}: File {} could not be parsed by rnix: {}", - relative_package_dir.display(), - subpath.display(), - error, - ), - CheckProblem::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 - ), - CheckProblem::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 - ), - CheckProblem::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, - ), - CheckProblem::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, - ), - } - } -} /// 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(Left(Vec)): A non-fatal problem with the Nixpkgs files. +/// - Ok(Left(Vec)): A non-fatal 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(Right(A)): A successful (potentially intermediate) result with an arbitrary value. /// No fatal errors have occurred and no problems have been found with Nixpkgs. -pub type CheckResult = anyhow::Result, A>>; +pub type CheckResult = anyhow::Result, A>>; /// Map a `CheckResult` to a `CheckResult` by applying a function to the /// potentially contained value in case of success. @@ -250,8 +28,15 @@ pub fn ok(value: A) -> CheckResult { Ok(Right(value)) } +impl NixpkgsProblem { + /// Create a `CheckResult` from a single check problem + pub fn into_result(self) -> CheckResult { + Ok(Left(vec![self])) + } +} + /// Combine two check results, both of which need to be successful for the return value to be successful. -/// The `CheckProblem`s of both sides are returned concatenated. +/// The `NixpkgsProblem`s of both sides are returned concatenated. pub fn and(first: CheckResult<()>, second: CheckResult) -> CheckResult { match (first?, second?) { (Right(_), Right(right_value)) => Ok(Right(right_value)), @@ -264,7 +49,7 @@ pub fn and(first: CheckResult<()>, second: CheckResult) -> CheckResult /// Combine many checks results into a single one. /// All given check results need to be successful in order for the returned check result to be successful, /// in which case the returned check result value contains each a `Vec` of each individual result value. -/// The `CheckProblem`s of all results are returned concatenated. +/// The `NixpkgsProblem`s of all results are returned concatenated. pub fn sequence(check_results: impl IntoIterator>) -> CheckResult> { let (errors, values): (Vec<_>, Vec<_>) = check_results .into_iter() diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index a76abba05d96..37fc783f3d34 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs @@ -1,5 +1,6 @@ use crate::check_result; -use crate::check_result::{CheckProblem, CheckResult}; +use crate::check_result::CheckResult; +use crate::nixpkgs_problem::NixpkgsProblem; use crate::structure; use crate::Version; use std::path::Path; @@ -137,13 +138,13 @@ pub fn check_values( }; if !valid { - CheckProblem::WrongCallPackage { + NixpkgsProblem::WrongCallPackage { relative_package_file: relative_package_file.clone(), package_name: package_name.clone(), } .into_result() } else if !attribute_info.is_derivation { - CheckProblem::NonDerivation { + NixpkgsProblem::NonDerivation { relative_package_file: relative_package_file.clone(), package_name: package_name.clone(), } @@ -152,7 +153,7 @@ pub fn check_values( check_result::ok(()) } } else { - CheckProblem::UndefinedAttr { + NixpkgsProblem::UndefinedAttr { relative_package_file: relative_package_file.clone(), package_name: package_name.clone(), } diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index 11fedde74130..1651624f3f40 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs @@ -1,5 +1,6 @@ mod check_result; mod eval; +mod nixpkgs_problem; mod references; mod structure; mod utils; 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 000000000000..2344a8cc1325 --- /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 a6bc53d717d7..37837a54ddc2 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/references.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/references.rs @@ -1,5 +1,6 @@ use crate::check_result; -use crate::check_result::{CheckProblem, CheckResult}; +use crate::check_result::CheckResult; +use crate::nixpkgs_problem::NixpkgsProblem; use crate::utils; use crate::utils::LineIndex; @@ -47,7 +48,7 @@ fn check_path(context: &PackageContext, subpath: &Path) -> CheckResult<()> { // 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) { - CheckProblem::OutsideSymlink { + NixpkgsProblem::OutsideSymlink { relative_package_dir: context.relative_package_dir.clone(), subpath: subpath.to_path_buf(), } @@ -56,7 +57,7 @@ fn check_path(context: &PackageContext, subpath: &Path) -> CheckResult<()> { check_result::ok(()) } } - Err(io_error) => CheckProblem::UnresolvableSymlink { + Err(io_error) => NixpkgsProblem::UnresolvableSymlink { relative_package_dir: context.relative_package_dir.clone(), subpath: subpath.to_path_buf(), io_error, @@ -104,7 +105,7 @@ fn check_nix_file(context: &PackageContext, subpath: &Path) -> CheckResult<()> { let root = Root::parse(&contents); if let Some(error) = root.errors().first() { - return CheckProblem::CouldNotParseNix { + return NixpkgsProblem::CouldNotParseNix { relative_package_dir: context.relative_package_dir.clone(), subpath: subpath.to_path_buf(), error: error.clone(), @@ -124,7 +125,7 @@ fn check_nix_file(context: &PackageContext, subpath: &Path) -> CheckResult<()> { } else if node.children().count() != 0 { // Filters out ./foo/${bar}/baz // TODO: We can just check ./foo - CheckProblem::PathInterpolation { + NixpkgsProblem::PathInterpolation { relative_package_dir: context.relative_package_dir.clone(), subpath: subpath.to_path_buf(), line, @@ -133,7 +134,7 @@ fn check_nix_file(context: &PackageContext, subpath: &Path) -> CheckResult<()> { .into_result() } else if text.starts_with('<') { // Filters out search paths like - CheckProblem::SearchPath { + NixpkgsProblem::SearchPath { relative_package_dir: context.relative_package_dir.clone(), subpath: subpath.to_path_buf(), line, @@ -149,7 +150,7 @@ fn check_nix_file(context: &PackageContext, subpath: &Path) -> CheckResult<()> { // 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) { - CheckProblem::OutsidePathReference { + NixpkgsProblem::OutsidePathReference { relative_package_dir: context.relative_package_dir.clone(), subpath: subpath.to_path_buf(), line, @@ -160,7 +161,7 @@ fn check_nix_file(context: &PackageContext, subpath: &Path) -> CheckResult<()> { check_result::ok(()) } } - Err(e) => CheckProblem::UnresolvablePathReference { + Err(e) => NixpkgsProblem::UnresolvablePathReference { relative_package_dir: context.relative_package_dir.clone(), subpath: subpath.to_path_buf(), line, diff --git a/pkgs/test/nixpkgs-check-by-name/src/structure.rs b/pkgs/test/nixpkgs-check-by-name/src/structure.rs index 90a552a38863..b69f6211c0a0 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/structure.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/structure.rs @@ -1,10 +1,13 @@ use crate::check_result; -use crate::check_result::{CheckProblem, CheckResult}; +use crate::check_result::CheckResult; +use crate::nixpkgs_problem::NixpkgsProblem; use crate::references; use crate::utils; use crate::utils::{BASE_SUBPATH, PACKAGE_NIX_FILENAME}; +use itertools::concat; use lazy_static::lazy_static; use regex::Regex; +use std::fs::DirEntry; use std::path::{Path, PathBuf}; lazy_static! { @@ -30,8 +33,8 @@ pub fn relative_file_for_package(package_name: &str) -> PathBuf { relative_dir_for_package(package_name).join(PACKAGE_NIX_FILENAME) } -/// Read the structure of a Nixpkgs directory, displaying errors on the writer. -/// May return early with I/O errors. +/// Check the structure of Nixpkgs, returning the attribute names that are defined in +/// `pkgs/by-name` pub fn check_structure(path: &Path) -> CheckResult> { let base_dir = path.join(BASE_SUBPATH); @@ -46,7 +49,7 @@ pub fn check_structure(path: &Path) -> CheckResult> { // README.md is allowed to be a file and not checked check_result::ok(vec![]) } else if !shard_path.is_dir() { - CheckProblem::ShardNonDir { + NixpkgsProblem::ShardNonDir { relative_shard_path: relative_shard_path.clone(), } .into_result() @@ -54,7 +57,7 @@ pub fn check_structure(path: &Path) -> CheckResult> { } else { let shard_name_valid = SHARD_NAME_REGEX.is_match(&shard_name); let result = if !shard_name_valid { - CheckProblem::InvalidShardName { + NixpkgsProblem::InvalidShardName { relative_shard_path: relative_shard_path.clone(), shard_name: shard_name.clone(), } @@ -72,7 +75,7 @@ pub fn check_structure(path: &Path) -> CheckResult> { l.file_name().to_ascii_lowercase() == r.file_name().to_ascii_lowercase() }) .map(|(l, r)| { - CheckProblem::CaseSensitiveDuplicate { + NixpkgsProblem::CaseSensitiveDuplicate { relative_shard_path: relative_shard_path.clone(), first: l.file_name(), second: r.file_name(), @@ -83,84 +86,87 @@ pub fn check_structure(path: &Path) -> CheckResult> { let result = check_result::and(result, check_result::sequence_(duplicate_results)); let package_results = entries.into_iter().map(|package_entry| { - 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() { - CheckProblem::PackageNonDir { - relative_package_dir: relative_package_dir.clone(), - } - .into_result() - } else { - let package_name_valid = PACKAGE_NAME_REGEX.is_match(&package_name); - let result = if !package_name_valid { - CheckProblem::InvalidPackageName { - relative_package_dir: relative_package_dir.clone(), - package_name: package_name.clone(), - } - .into_result() - } else { - check_result::ok(()) - }; - - let correct_relative_package_dir = relative_dir_for_package(&package_name); - let result = check_result::and( - result, - 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 { - CheckProblem::IncorrectShard { - relative_package_dir: relative_package_dir.clone(), - correct_relative_package_dir: correct_relative_package_dir - .clone(), - } - .into_result() - } else { - check_result::ok(()) - } - } else { - check_result::ok(()) - }, - ); - - let package_nix_path = package_path.join(PACKAGE_NIX_FILENAME); - let result = check_result::and( - result, - if !package_nix_path.exists() { - CheckProblem::PackageNixNonExistent { - relative_package_dir: relative_package_dir.clone(), - } - .into_result() - } else if package_nix_path.is_dir() { - CheckProblem::PackageNixDir { - relative_package_dir: relative_package_dir.clone(), - } - .into_result() - } else { - check_result::ok(()) - }, - ); - - let result = check_result::and( - result, - references::check_references( - &relative_package_dir, - &path.join(&relative_package_dir), - ), - ); - - check_result::map(result, |_| package_name.clone()) - } + check_package(path, &shard_name, shard_name_valid, package_entry) }); check_result::and(result, check_result::sequence(package_results)) } }); - check_result::map(check_result::sequence(shard_results), |x| { - x.into_iter().flatten().collect::>() - }) + // Combine the package names conatained within each shard into a longer list + check_result::map(check_result::sequence(shard_results), concat) +} + +fn check_package( + path: &Path, + shard_name: &str, + shard_name_valid: bool, + package_entry: DirEntry, +) -> CheckResult { + 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() { + NixpkgsProblem::PackageNonDir { + relative_package_dir: relative_package_dir.clone(), + } + .into_result() + } 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_result() + } else { + check_result::ok(()) + }; + + let correct_relative_package_dir = relative_dir_for_package(&package_name); + let result = check_result::and( + result, + 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_result() + } else { + check_result::ok(()) + } + } else { + check_result::ok(()) + }, + ); + + let package_nix_path = package_path.join(PACKAGE_NIX_FILENAME); + let result = check_result::and( + result, + if !package_nix_path.exists() { + NixpkgsProblem::PackageNixNonExistent { + relative_package_dir: relative_package_dir.clone(), + } + .into_result() + } else if package_nix_path.is_dir() { + NixpkgsProblem::PackageNixDir { + relative_package_dir: relative_package_dir.clone(), + } + .into_result() + } else { + check_result::ok(()) + }, + ); + + let result = check_result::and( + result, + references::check_references(&relative_package_dir, &path.join(&relative_package_dir)), + ); + + check_result::map(result, |_| package_name.clone()) + } } From 03c58ad1d687e8928b2b59f8dc44fb2c4c43300f Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Tue, 24 Oct 2023 00:20:32 +0200 Subject: [PATCH 26/28] tests.nixpkgs-check-by-name: Minor doc updates --- pkgs/test/nixpkgs-check-by-name/README.md | 4 ++-- pkgs/test/nixpkgs-check-by-name/src/main.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/README.md b/pkgs/test/nixpkgs-check-by-name/README.md index 2d2db9a58bc5..146cea0a64ba 100644 --- a/pkgs/test/nixpkgs-check-by-name/README.md +++ b/pkgs/test/nixpkgs-check-by-name/README.md @@ -1,6 +1,6 @@ # Nixpkgs pkgs/by-name checker -This directory implements a program to check the [validity](#validity-checks) of the `pkgs/by-name` Nixpkgs directory once introduced. +This directory implements a program to check the [validity](#validity-checks) of the `pkgs/by-name` Nixpkgs directory. It is being used by [this GitHub Actions workflow](../../../.github/workflows/check-by-name.yml). This is part of the implementation of [RFC 140](https://github.com/NixOS/rfcs/pull/140). @@ -24,7 +24,7 @@ This API may be changed over time if the CI workflow making use of it is adjuste - `2`: If an unexpected I/O error occurs - Standard error: - Informative messages - - Error messages if validation is not successful + - Detected problems if validation is not successful ## Validity checks diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index 1651624f3f40..1f52d81c36cf 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs @@ -68,8 +68,8 @@ 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( nixpkgs_path: &Path, version: Version, From 82e708c19230f77fbb5ea01157fc1226e72119de Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Tue, 24 Oct 2023 19:27:48 +0200 Subject: [PATCH 27/28] tests.nixpkgs-check-by-name: Custom Validation type and improvements Co-authored-by: Wanja Hentze --- .../nixpkgs-check-by-name/src/check_result.rs | 74 --------- pkgs/test/nixpkgs-check-by-name/src/eval.rs | 93 +++++------ pkgs/test/nixpkgs-check-by-name/src/main.rs | 22 ++- .../nixpkgs-check-by-name/src/references.rs | 148 +++++++++--------- .../nixpkgs-check-by-name/src/structure.rs | 120 +++++++------- .../nixpkgs-check-by-name/src/validation.rs | 102 ++++++++++++ 6 files changed, 296 insertions(+), 263 deletions(-) delete mode 100644 pkgs/test/nixpkgs-check-by-name/src/check_result.rs create mode 100644 pkgs/test/nixpkgs-check-by-name/src/validation.rs diff --git a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs b/pkgs/test/nixpkgs-check-by-name/src/check_result.rs deleted file mode 100644 index a6538d778b61..000000000000 --- a/pkgs/test/nixpkgs-check-by-name/src/check_result.rs +++ /dev/null @@ -1,74 +0,0 @@ -use crate::nixpkgs_problem::NixpkgsProblem; -use itertools::concat; -use itertools::{ - Either, - Either::{Left, Right}, - Itertools, -}; - -/// 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(Left(Vec)): A non-fatal 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(Right(A)): A successful (potentially intermediate) result with an arbitrary value. -/// No fatal errors have occurred and no problems have been found with Nixpkgs. -pub type CheckResult = anyhow::Result, A>>; - -/// Map a `CheckResult` to a `CheckResult` by applying a function to the -/// potentially contained value in case of success. -pub fn map(check_result: CheckResult, f: impl FnOnce(I) -> O) -> CheckResult { - Ok(check_result?.map_right(f)) -} - -/// Create a successfull `CheckResult` from a value `A`. -pub fn ok(value: A) -> CheckResult { - Ok(Right(value)) -} - -impl NixpkgsProblem { - /// Create a `CheckResult` from a single check problem - pub fn into_result(self) -> CheckResult { - Ok(Left(vec![self])) - } -} - -/// Combine two check results, 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(first: CheckResult<()>, second: CheckResult) -> CheckResult { - match (first?, second?) { - (Right(_), Right(right_value)) => Ok(Right(right_value)), - (Left(errors), Right(_)) => Ok(Left(errors)), - (Right(_), Left(errors)) => Ok(Left(errors)), - (Left(errors_l), Left(errors_r)) => Ok(Left(concat([errors_l, errors_r]))), - } -} - -/// Combine many checks results into a single one. -/// All given check results need to be successful in order for the returned check result to be successful, -/// in which case the returned check result value contains each a `Vec` of each individual result value. -/// The `NixpkgsProblem`s of all results are returned concatenated. -pub fn sequence(check_results: impl IntoIterator>) -> CheckResult> { - let (errors, values): (Vec<_>, Vec<_>) = check_results - .into_iter() - .collect::>>()? - .into_iter() - .partition_map(|r| r); - - // 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().flatten().collect::>(); - - if flattened_errors.is_empty() { - Ok(Right(values)) - } else { - Ok(Left(flattened_errors)) - } -} - -/// Like `sequence`, but replace the resulting value with () -pub fn sequence_(check_results: impl IntoIterator>) -> CheckResult<()> { - map(sequence(check_results), |_| ()) -} diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index 37fc783f3d34..e4f986748b4f 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs @@ -1,7 +1,6 @@ -use crate::check_result; -use crate::check_result::CheckResult; use crate::nixpkgs_problem::NixpkgsProblem; use crate::structure; +use crate::validation::{self, Validation::Success}; use crate::Version; use std::path::Path; @@ -46,7 +45,7 @@ pub fn check_values( nixpkgs_path: &Path, package_names: Vec, eval_accessible_paths: Vec<&Path>, -) -> CheckResult<()> { +) -> 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")?; @@ -112,52 +111,54 @@ pub fn check_values( String::from_utf8_lossy(&result.stdout) ))?; - check_result::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); + 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 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 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 !valid { - NixpkgsProblem::WrongCallPackage { - relative_package_file: relative_package_file.clone(), - package_name: package_name.clone(), + 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(()) } - .into_result() - } else if !attribute_info.is_derivation { - NixpkgsProblem::NonDerivation { - relative_package_file: relative_package_file.clone(), - package_name: package_name.clone(), - } - .into_result() } else { - check_result::ok(()) + NixpkgsProblem::UndefinedAttr { + relative_package_file: relative_package_file.clone(), + package_name: package_name.clone(), + } + .into() } - } else { - NixpkgsProblem::UndefinedAttr { - relative_package_file: relative_package_file.clone(), - package_name: package_name.clone(), - } - .into_result() - } - })) + }, + ))) } diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index 1f52d81c36cf..4cabf8f446f5 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs @@ -1,18 +1,16 @@ -mod check_result; 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, ValueEnum}; use colored::Colorize; -use itertools::{ - Either, - Either::{Left, Right}, -}; use std::io; use std::path::{Path, PathBuf}; use std::process::ExitCode; @@ -86,26 +84,26 @@ pub fn check_nixpkgs( "Given Nixpkgs path does not contain a {} subdirectory, no check necessary.", utils::BASE_SUBPATH ); - check_result::ok(()) + Success(()) } else { match check_structure(&nixpkgs_path)? { - Left(errors) => Ok(Left(errors)), - Right(package_names) => + 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) + eval::check_values(version, &nixpkgs_path, package_names, eval_accessible_paths)? } } }; - match check_result? { - Either::Left(errors) => { + match check_result { + Failure(errors) => { for error in errors { writeln!(error_writer, "{}", error.to_string().red())? } Ok(false) } - Either::Right(_) => Ok(true), + Success(_) => Ok(true), } } diff --git a/pkgs/test/nixpkgs-check-by-name/src/references.rs b/pkgs/test/nixpkgs-check-by-name/src/references.rs index 37837a54ddc2..c437ebd222f8 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/references.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/references.rs @@ -1,8 +1,7 @@ -use crate::check_result; -use crate::check_result::CheckResult; use crate::nixpkgs_problem::NixpkgsProblem; use crate::utils; use crate::utils::LineIndex; +use crate::validation::{self, ResultIteratorExt, Validation::Success}; use anyhow::Context; use rnix::{Root, SyntaxKind::NODE_PATH}; @@ -23,7 +22,7 @@ struct PackageContext<'a> { pub fn check_references( relative_package_dir: &Path, absolute_package_dir: &Path, -) -> CheckResult<()> { +) -> validation::Result<()> { let context = PackageContext { relative_package_dir: &relative_package_dir.to_path_buf(), absolute_package_dir: &absolute_package_dir.to_path_buf(), @@ -38,10 +37,10 @@ pub fn check_references( } /// Checks for a specific path to not have references outside -fn check_path(context: &PackageContext, subpath: &Path) -> CheckResult<()> { +fn check_path(context: &PackageContext, subpath: &Path) -> validation::Result<()> { let path = context.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) => { @@ -52,9 +51,9 @@ fn check_path(context: &PackageContext, subpath: &Path) -> CheckResult<()> { relative_package_dir: context.relative_package_dir.clone(), subpath: subpath.to_path_buf(), } - .into_result() + .into() } else { - check_result::ok(()) + Success(()) } } Err(io_error) => NixpkgsProblem::UnresolvableSymlink { @@ -62,15 +61,20 @@ fn check_path(context: &PackageContext, subpath: &Path) -> CheckResult<()> { subpath: subpath.to_path_buf(), io_error, } - .into_result(), + .into(), } } else if path.is_dir() { // Recursively check each entry - check_result::sequence_(utils::read_dir_sorted(&path)?.into_iter().map(|entry| { - 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(context, &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() { @@ -78,22 +82,22 @@ fn check_path(context: &PackageContext, subpath: &Path) -> CheckResult<()> { check_nix_file(context, subpath).context(format!( "Error while checking Nix file {}", subpath.display() - )) + ))? } else { - check_result::ok(()) + Success(()) } } else { - check_result::ok(()) + Success(()) } } else { // This should never happen, git doesn't support other file types anyhow::bail!("Unsupported file type for path {}", subpath.display()); - } + }) } /// Check whether a nix file contains path expression references pointing outside the package /// directory -fn check_nix_file(context: &PackageContext, subpath: &Path) -> CheckResult<()> { +fn check_nix_file(context: &PackageContext, subpath: &Path) -> validation::Result<()> { let path = context.absolute_package_dir.join(subpath); let parent_dir = path.parent().context(format!( "Could not get parent of path {}", @@ -105,71 +109,75 @@ fn check_nix_file(context: &PackageContext, subpath: &Path) -> CheckResult<()> { let root = Root::parse(&contents); if let Some(error) = root.errors().first() { - return NixpkgsProblem::CouldNotParseNix { + return Ok(NixpkgsProblem::CouldNotParseNix { relative_package_dir: context.relative_package_dir.clone(), subpath: subpath.to_path_buf(), error: error.clone(), } - .into_result(); + .into()); } let line_index = LineIndex::new(&contents); - check_result::sequence_(root.syntax().descendants().map(|node| { - let text = node.text().to_string(); - let line = line_index.line(node.text_range().start().into()); + 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 - check_result::ok(()) - } else if node.children().count() != 0 { - // Filters out ./foo/${bar}/baz - // TODO: We can just check ./foo - NixpkgsProblem::PathInterpolation { - relative_package_dir: context.relative_package_dir.clone(), - subpath: subpath.to_path_buf(), - line, - text, - } - .into_result() - } else if text.starts_with('<') { - // Filters out search paths like - NixpkgsProblem::SearchPath { - relative_package_dir: context.relative_package_dir.clone(), - subpath: subpath.to_path_buf(), - line, - text, - } - .into_result() - } 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(context.absolute_package_dir) { - NixpkgsProblem::OutsidePathReference { - relative_package_dir: context.relative_package_dir.clone(), - subpath: subpath.to_path_buf(), - line, - text, - } - .into_result() - } else { - check_result::ok(()) - } - } - Err(e) => NixpkgsProblem::UnresolvablePathReference { + 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: context.relative_package_dir.clone(), subpath: subpath.to_path_buf(), line, text, - io_error: e, } - .into_result(), + .into() + } else if text.starts_with('<') { + // Filters out search paths like + NixpkgsProblem::SearchPath { + relative_package_dir: context.relative_package_dir.clone(), + 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(context.absolute_package_dir) + { + NixpkgsProblem::OutsidePathReference { + relative_package_dir: context.relative_package_dir.clone(), + subpath: subpath.to_path_buf(), + line, + text, + } + .into() + } else { + Success(()) + } + } + Err(e) => NixpkgsProblem::UnresolvablePathReference { + relative_package_dir: context.relative_package_dir.clone(), + subpath: subpath.to_path_buf(), + line, + text, + io_error: e, + } + .into(), + } } - } - })) + }, + ))) } diff --git a/pkgs/test/nixpkgs-check-by-name/src/structure.rs b/pkgs/test/nixpkgs-check-by-name/src/structure.rs index b69f6211c0a0..4051ca037c9a 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/structure.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/structure.rs @@ -1,9 +1,8 @@ -use crate::check_result; -use crate::check_result::CheckResult; use crate::nixpkgs_problem::NixpkgsProblem; use crate::references; use crate::utils; 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; @@ -35,24 +34,25 @@ pub fn relative_file_for_package(package_name: &str) -> PathBuf { /// Check the structure of Nixpkgs, returning the attribute names that are defined in /// `pkgs/by-name` -pub fn check_structure(path: &Path) -> CheckResult> { +pub fn check_structure(path: &Path) -> validation::Result> { let base_dir = path.join(BASE_SUBPATH); let shard_results = utils::read_dir_sorted(&base_dir)? .into_iter() - .map(|shard_entry| { + .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 = 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 - check_result::ok(vec![]) + + Success(vec![]) } else if !shard_path.is_dir() { NixpkgsProblem::ShardNonDir { relative_shard_path: relative_shard_path.clone(), } - .into_result() + .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); @@ -61,9 +61,9 @@ pub fn check_structure(path: &Path) -> CheckResult> { relative_shard_path: relative_shard_path.clone(), shard_name: shard_name.clone(), } - .into_result() + .into() } else { - check_result::ok(()) + Success(()) }; let entries = utils::read_dir_sorted(&shard_path)?; @@ -80,21 +80,25 @@ pub fn check_structure(path: &Path) -> CheckResult> { first: l.file_name(), second: r.file_name(), } - .into_result::<()>() + .into() }); - let result = check_result::and(result, check_result::sequence_(duplicate_results)); + 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) - }); + let package_results = entries + .into_iter() + .map(|package_entry| { + check_package(path, &shard_name, shard_name_valid, package_entry) + }) + .collect_vec()?; - check_result::and(result, check_result::sequence(package_results)) - } - }); + result.and(validation::sequence(package_results)) + }) + }) + .collect_vec()?; // Combine the package names conatained within each shard into a longer list - check_result::map(check_result::sequence(shard_results), concat) + Ok(validation::sequence(shard_results).map(concat)) } fn check_package( @@ -102,16 +106,16 @@ fn check_package( shard_name: &str, shard_name_valid: bool, package_entry: DirEntry, -) -> CheckResult { +) -> validation::Result { 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() { + Ok(if !package_path.is_dir() { NixpkgsProblem::PackageNonDir { relative_package_dir: relative_package_dir.clone(), } - .into_result() + .into() } else { let package_name_valid = PACKAGE_NAME_REGEX.is_match(&package_name); let result = if !package_name_valid { @@ -119,54 +123,48 @@ fn check_package( relative_package_dir: relative_package_dir.clone(), package_name: package_name.clone(), } - .into_result() + .into() } else { - check_result::ok(()) + Success(()) }; let correct_relative_package_dir = relative_dir_for_package(&package_name); - let result = check_result::and( - result, - 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_result() - } else { - check_result::ok(()) + 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 { - check_result::ok(()) - }, - ); + Success(()) + } + } else { + Success(()) + }); let package_nix_path = package_path.join(PACKAGE_NIX_FILENAME); - let result = check_result::and( - result, - if !package_nix_path.exists() { - NixpkgsProblem::PackageNixNonExistent { - relative_package_dir: relative_package_dir.clone(), - } - .into_result() - } else if package_nix_path.is_dir() { - NixpkgsProblem::PackageNixDir { - relative_package_dir: relative_package_dir.clone(), - } - .into_result() - } else { - check_result::ok(()) - }, - ); + 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 = check_result::and( - result, - references::check_references(&relative_package_dir, &path.join(&relative_package_dir)), - ); + let result = result.and(references::check_references( + &relative_package_dir, + &path.join(&relative_package_dir), + )?); - check_result::map(result, |_| package_name.clone()) - } + result.map(|_| package_name.clone()) + }) } 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 000000000000..e72793851521 --- /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 { + Failure(Vec), + Success(A), +} + +impl From for Validation { + /// Create a `Validation` 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)): 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 = anyhow::Result>; + +pub trait ResultIteratorExt: Sized + Iterator> { + fn collect_vec(self) -> std::result::Result, E>; +} + +impl ResultIteratorExt for I +where + I: Sized + Iterator>, +{ + /// A convenience version of `collect` specialised to a vector + fn collect_vec(self) -> std::result::Result, E> { + self.collect() + } +} + +impl Validation { + /// Map a `Validation` to a `Validation` by applying a function to the + /// potentially contained value in case of success. + pub fn map(self, f: impl FnOnce(A) -> B) -> Validation { + 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(self, other: Validation) -> Validation { + 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(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), + }); + + // 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>) -> Validation<()> { + sequence(validations).map(|_| ()) +} From 7753969628f2d229d2e9fc40a904d5f08dece09b Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Tue, 24 Oct 2023 19:37:52 +0200 Subject: [PATCH 28/28] tests.nixpkgs-check-by-name: Remove PackageContext helper Was not really necessary anymore --- .../nixpkgs-check-by-name/src/references.rs | 62 ++++++++----------- 1 file changed, 27 insertions(+), 35 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/references.rs b/pkgs/test/nixpkgs-check-by-name/src/references.rs index c437ebd222f8..0561a9b22e85 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/references.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/references.rs @@ -7,15 +7,7 @@ use anyhow::Context; use rnix::{Root, SyntaxKind::NODE_PATH}; use std::ffi::OsStr; use std::fs::read_to_string; -use std::path::{Path, PathBuf}; - -/// Small helper so we don't need to pass in the same arguments to all functions -struct PackageContext<'a> { - /// 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. @@ -23,22 +15,21 @@ pub fn check_references( relative_package_dir: &Path, absolute_package_dir: &Path, ) -> validation::Result<()> { - let context = PackageContext { - relative_package_dir: &relative_package_dir.to_path_buf(), - absolute_package_dir: &absolute_package_dir.to_path_buf(), - }; - // The empty argument here is the subpath under the package directory to check // An empty one means the package directory itself - check_path(&context, Path::new("")).context(format!( + 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(context: &PackageContext, subpath: &Path) -> validation::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); Ok(if path.is_symlink() { // Check whether the symlink resolves to outside the package directory @@ -46,9 +37,9 @@ fn check_path(context: &PackageContext, subpath: &Path) -> validation::Result<() 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) { + if let Err(_prefix_error) = target.strip_prefix(absolute_package_dir) { NixpkgsProblem::OutsideSymlink { - relative_package_dir: context.relative_package_dir.clone(), + relative_package_dir: relative_package_dir.to_path_buf(), subpath: subpath.to_path_buf(), } .into() @@ -57,7 +48,7 @@ fn check_path(context: &PackageContext, subpath: &Path) -> validation::Result<() } } Err(io_error) => NixpkgsProblem::UnresolvableSymlink { - relative_package_dir: context.relative_package_dir.clone(), + relative_package_dir: relative_package_dir.to_path_buf(), subpath: subpath.to_path_buf(), io_error, } @@ -70,7 +61,7 @@ fn check_path(context: &PackageContext, subpath: &Path) -> validation::Result<() .into_iter() .map(|entry| { let entry_subpath = subpath.join(entry.file_name()); - check_path(context, &entry_subpath) + check_path(relative_package_dir, absolute_package_dir, &entry_subpath) .context(format!("Error while recursing into {}", subpath.display())) }) .collect_vec()?, @@ -79,10 +70,9 @@ fn check_path(context: &PackageContext, subpath: &Path) -> validation::Result<() // 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(()) } @@ -97,8 +87,12 @@ fn check_path(context: &PackageContext, subpath: &Path) -> validation::Result<() /// Check whether a nix file contains path expression references pointing outside the package /// directory -fn check_nix_file(context: &PackageContext, subpath: &Path) -> validation::Result<()> { - let path = context.absolute_package_dir.join(subpath); +fn check_nix_file( + relative_package_dir: &Path, + absolute_package_dir: &Path, + subpath: &Path, +) -> 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,7 +104,7 @@ fn check_nix_file(context: &PackageContext, subpath: &Path) -> validation::Resul let root = Root::parse(&contents); if let Some(error) = root.errors().first() { return Ok(NixpkgsProblem::CouldNotParseNix { - relative_package_dir: context.relative_package_dir.clone(), + relative_package_dir: relative_package_dir.to_path_buf(), subpath: subpath.to_path_buf(), error: error.clone(), } @@ -131,7 +125,7 @@ fn check_nix_file(context: &PackageContext, subpath: &Path) -> validation::Resul // Filters out ./foo/${bar}/baz // TODO: We can just check ./foo NixpkgsProblem::PathInterpolation { - relative_package_dir: context.relative_package_dir.clone(), + relative_package_dir: relative_package_dir.to_path_buf(), subpath: subpath.to_path_buf(), line, text, @@ -140,7 +134,7 @@ fn check_nix_file(context: &PackageContext, subpath: &Path) -> validation::Resul } else if text.starts_with('<') { // Filters out search paths like NixpkgsProblem::SearchPath { - relative_package_dir: context.relative_package_dir.clone(), + relative_package_dir: relative_package_dir.to_path_buf(), subpath: subpath.to_path_buf(), line, text, @@ -154,11 +148,9 @@ fn check_nix_file(context: &PackageContext, subpath: &Path) -> validation::Resul // 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) - { + if let Err(_prefix_error) = target.strip_prefix(absolute_package_dir) { NixpkgsProblem::OutsidePathReference { - relative_package_dir: context.relative_package_dir.clone(), + relative_package_dir: relative_package_dir.to_path_buf(), subpath: subpath.to_path_buf(), line, text, @@ -169,7 +161,7 @@ fn check_nix_file(context: &PackageContext, subpath: &Path) -> validation::Resul } } Err(e) => NixpkgsProblem::UnresolvablePathReference { - relative_package_dir: context.relative_package_dir.clone(), + relative_package_dir: relative_package_dir.to_path_buf(), subpath: subpath.to_path_buf(), line, text,