From 413dd9c03e37562c61cd89799e5eb8a88c7bb42a Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 15 Dec 2023 01:02:02 +0100 Subject: [PATCH] tests.nixpkgs-check-by-name: Minor improvements from review --- pkgs/test/nixpkgs-check-by-name/src/eval.rs | 8 ++--- pkgs/test/nixpkgs-check-by-name/src/main.rs | 32 ++++++++++--------- .../test/nixpkgs-check-by-name/src/version.rs | 2 ++ .../tests/no-by-name/expected | 1 + 4 files changed, 24 insertions(+), 19 deletions(-) create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/no-by-name/expected diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index 927e446b452f..20652d9ede26 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs @@ -41,7 +41,7 @@ enum AttributeVariant { pub fn check_values( nixpkgs_path: &Path, package_names: Vec, - eval_accessible_paths: &Vec<&Path>, + eval_accessible_paths: &[&Path], ) -> 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. @@ -110,11 +110,11 @@ pub fn check_values( ))?; Ok( - validation::sequence(package_names.iter().map(|package_name| { - let relative_package_file = structure::relative_file_for_package(package_name); + validation::sequence(package_names.into_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) { + if let Some(attribute_info) = actual_files.get(&package_name) { let check_result = if !attribute_info.is_derivation { NixpkgsProblem::NonDerivation { relative_package_file: relative_package_file.clone(), diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index 91e1992a52c9..ee73ffbd0f8d 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs @@ -31,12 +31,7 @@ pub struct Args { fn main() -> ExitCode { let args = Args::parse(); - match process( - args.base.as_deref(), - &args.nixpkgs, - &vec![], - &mut io::stderr(), - ) { + match process(args.base.as_deref(), &args.nixpkgs, &[], &mut io::stderr()) { Ok(true) => { eprintln!("{}", "Validated successfully".green()); ExitCode::SUCCESS @@ -69,10 +64,10 @@ fn main() -> ExitCode { pub fn process( base_nixpkgs: Option<&Path>, main_nixpkgs: &Path, - eval_accessible_paths: &Vec<&Path>, + eval_accessible_paths: &[&Path], error_writer: &mut W, ) -> anyhow::Result { - let main_result = check_nixpkgs(main_nixpkgs, eval_accessible_paths)?; + let main_result = check_nixpkgs(main_nixpkgs, eval_accessible_paths, error_writer)?; let check_result = main_result.result_map(|nixpkgs_version| { if let Some(base) = base_nixpkgs { check_nixpkgs(base, eval_accessible_paths, error_writer)?.result_map( @@ -99,11 +94,17 @@ pub fn process( } } -/// Checks whether the pkgs/by-name structure in Nixpkgs is valid, -/// and returns to which degree it's valid for checks with increased strictness. -pub fn check_nixpkgs( +/// Checks whether the pkgs/by-name structure in Nixpkgs is valid. +/// +/// This does not include checks that depend on the base version of Nixpkgs to compare against, +/// which is used for checks that were only introduced later and increased strictness. +/// +/// Instead a `version::Nixpkgs` is returned, whose `compare` method allows comparing the +/// result of this function for the base Nixpkgs against the one for the main Nixpkgs. +pub fn check_nixpkgs( nixpkgs_path: &Path, - eval_accessible_paths: &Vec<&Path>, + eval_accessible_paths: &[&Path], + error_writer: &mut W, ) -> validation::Result { Ok({ let nixpkgs_path = nixpkgs_path.canonicalize().context(format!( @@ -112,10 +113,11 @@ pub fn check_nixpkgs( ))?; if !nixpkgs_path.join(utils::BASE_SUBPATH).exists() { - eprintln!( + writeln!( + error_writer, "Given Nixpkgs path does not contain a {} subdirectory, no check necessary.", utils::BASE_SUBPATH - ); + )?; Success(version::Nixpkgs::default()) } else { check_structure(&nixpkgs_path)?.result_map(|package_names| @@ -224,7 +226,7 @@ mod tests { // We don't want coloring to mess up the tests let writer = temp_env::with_var("NO_COLOR", Some("1"), || -> anyhow::Result<_> { let mut writer = vec![]; - process(base_nixpkgs, &path, &vec![&extra_nix_path], &mut writer) + process(base_nixpkgs, &path, &[&extra_nix_path], &mut writer) .context(format!("Failed test case {name}"))?; Ok(writer) })?; diff --git a/pkgs/test/nixpkgs-check-by-name/src/version.rs b/pkgs/test/nixpkgs-check-by-name/src/version.rs index c5cee95e0d53..c82f537c504b 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/version.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/version.rs @@ -16,6 +16,8 @@ impl Nixpkgs { /// Compares two Nixpkgs versions against each other, returning validation errors only if the /// `from` version satisfied the stricter checks, while the `to` version doesn't satisfy them /// anymore. + /// This enables a gradual transition from weaker to stricter checks, by only allowing PRs to + /// increase strictness. pub fn compare(optional_from: Option, to: Self) -> Validation<()> { validation::sequence_( // We only loop over the current attributes, diff --git a/pkgs/test/nixpkgs-check-by-name/tests/no-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/no-by-name/expected new file mode 100644 index 000000000000..ddcb2df46e5f --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/no-by-name/expected @@ -0,0 +1 @@ +Given Nixpkgs path does not contain a pkgs/by-name subdirectory, no check necessary.