From 08e45272523708f13436a203220d81539382ac63 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Wed, 6 Dec 2023 02:56:50 +0100 Subject: [PATCH] tests.nixpkgs-check-by-name: Implement gradual empty arg check migration --- pkgs/test/nixpkgs-check-by-name/src/eval.rs | 138 ++++++++++++++---- pkgs/test/nixpkgs-check-by-name/src/main.rs | 78 +++++----- .../src/nixpkgs_problem.rs | 10 ++ 3 files changed, 162 insertions(+), 64 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index 161d013374e7f39..604942cf3315194 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs @@ -1,7 +1,7 @@ +use crate::validation::Validation; use crate::nixpkgs_problem::NixpkgsProblem; use crate::structure; use crate::validation::{self, Validation::Success}; -use crate::Version; use std::path::Path; use anyhow::Context; @@ -35,15 +35,78 @@ enum AttributeVariant { Other, } +pub enum AttributeResult { + AutoCalled, + CallPackage(EmptyArgVersion), +} + +#[derive(Clone, Copy, PartialEq, PartialOrd)] +pub enum EmptyArgVersion { + /// Initial version + V0, + /// Empty argument check + V1, +} + +impl EmptyArgVersion { + fn latest() -> Self { + Self::V1 + } + + fn compare(from: Self, to: Self, name: &str) -> Validation<()> { + if to >= from { + Success(()) + } else { + NixpkgsProblem::EmptyArgVersionDecrease { + relative_package_file: structure::relative_file_for_package(name), + package_name: name.to_owned(), + }.into() + } + } +} + +impl AttributeResult { + fn version(&self) -> EmptyArgVersion { + match self { + AttributeResult::AutoCalled => EmptyArgVersion::latest(), + AttributeResult::CallPackage(version) => *version, + } + } +} + +pub struct Attributes { + pub attributes: HashMap, +} + +impl Attributes { + pub fn new() -> Self { + Self { attributes: HashMap::new() } + } + + pub fn compare(from: Self, to: Self) -> Validation<()> { + // We only loop over the current attributes, + // we don't care about ones that were removed + validation::sequence_( + to.attributes.into_iter().map(|(name, value)| { + let before = from.attributes + .get(&name) + .map(|x| x.version()) + // New attributes must use the latest version + .unwrap_or(EmptyArgVersion::latest()); + EmptyArgVersion::compare(before, value.version(), &name) + }) + ) + } +} + /// 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( - version: Version, nixpkgs_path: &Path, package_names: Vec, - eval_accessible_paths: Vec<&Path>, -) -> validation::Result<()> { + eval_accessible_paths: &Vec<&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. let attrs_file = NamedTempFile::new().context("Failed to create a temporary file")?; @@ -110,47 +173,60 @@ pub fn check_values( String::from_utf8_lossy(&result.stdout) ))?; - Ok(validation::sequence_(package_names.iter().map( + 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, + let result = match &attribute_info.variant { + AttributeVariant::AutoCalled => Success(AttributeResult::AutoCalled), 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 + + if correct_file { + Success(AttributeResult::CallPackage( + if *empty_arg { + EmptyArgVersion::V0 + } else { + EmptyArgVersion::V1 + } + )) } else { - true - }; - correct_file && non_empty - } - AttributeVariant::Other => false, + NixpkgsProblem::WrongCallPackage { + relative_package_file: relative_package_file.clone(), + package_name: package_name.clone(), + } + .into() + } + }, + AttributeVariant::Other => + NixpkgsProblem::WrongCallPackage { + relative_package_file: relative_package_file.clone(), + package_name: package_name.clone(), + } + .into(), }; - 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(()) + match result { + Validation::Success(value) => { + if !attribute_info.is_derivation { + NixpkgsProblem::NonDerivation { + relative_package_file: relative_package_file.clone(), + package_name: package_name.clone(), + } + .into() + } else { + Success((package_name.clone(), value)) + } + }, + Validation::Failure(errors) => Validation::Failure(errors), } + } else { NixpkgsProblem::UndefinedAttr { relative_package_file: relative_package_file.clone(), @@ -159,5 +235,5 @@ pub fn check_values( .into() } }, - ))) + )).map(|elems| Attributes{ attributes: elems.into_iter().collect() })) } diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index 4cabf8f446f56af..95de125f61e29b6 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs @@ -9,7 +9,7 @@ use crate::structure::check_structure; use crate::validation::Validation::Failure; use crate::validation::Validation::Success; use anyhow::Context; -use clap::{Parser, ValueEnum}; +use clap::Parser; use colored::Colorize; use std::io; use std::path::{Path, PathBuf}; @@ -21,25 +21,15 @@ use std::process::ExitCode; pub struct Args { /// Path to nixpkgs nixpkgs: PathBuf, - /// The version of the checks - /// Increasing this may cause failures for a Nixpkgs that succeeded before - /// TODO: Remove default once Nixpkgs CI passes this argument - #[arg(long, value_enum, default_value_t = Version::V0)] - version: Version, -} -/// The version of the checks -#[derive(Debug, Clone, PartialEq, PartialOrd, ValueEnum)] -pub enum Version { - /// Initial version - V0, - /// Empty argument check - V1, + /// Path to the base Nixpkgs to compare against + #[arg(long)] + base: Option, } fn main() -> ExitCode { let args = Args::parse(); - match check_nixpkgs(&args.nixpkgs, args.version, vec![], &mut io::stderr()) { + match check_nixpkgs(args.base.as_deref(), &args.nixpkgs, vec![], &mut io::stderr()) { Ok(true) => { eprintln!("{}", "Validated successfully".green()); ExitCode::SUCCESS @@ -69,49 +59,71 @@ fn main() -> ExitCode { /// - `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, + base_nixpkgs: Option<&Path>, + pr_nixpkgs: &Path, eval_accessible_paths: Vec<&Path>, error_writer: &mut W, ) -> anyhow::Result { + + let pr_result = get_result(pr_nixpkgs, &eval_accessible_paths)?; + let check_result = match pr_result { + Success(attributes) => { + if let Some(base) = base_nixpkgs { + match get_result(base, &eval_accessible_paths)? { + Success(base_attributes) => + eval::Attributes::compare(base_attributes, attributes), + Failure(errors) => Failure(errors), + } + } else { + eval::Attributes::compare(eval::Attributes::new(), attributes) + } + }, + Failure(errors) => Failure(errors), + }; + + match check_result { + Failure(errors) => { + for error in errors { + writeln!(error_writer, "{}", error.to_string().red())? + } + Ok(false) + } + Success(_) => Ok(true), + } +} + +pub fn get_result( + nixpkgs_path: &Path, + eval_accessible_paths: &Vec<&Path>, +) -> validation::Result { let nixpkgs_path = nixpkgs_path.canonicalize().context(format!( "Nixpkgs path {} could not be resolved", nixpkgs_path.display() ))?; - let check_result = if !nixpkgs_path.join(utils::BASE_SUBPATH).exists() { + Ok( + if !nixpkgs_path.join(utils::BASE_SUBPATH).exists() { eprintln!( "Given Nixpkgs path does not contain a {} subdirectory, no check necessary.", utils::BASE_SUBPATH ); - Success(()) + Success(eval::Attributes::new()) } else { match check_structure(&nixpkgs_path)? { Failure(errors) => Failure(errors), Success(package_names) => // Only if we could successfully parse the structure, we do the evaluation checks { - eval::check_values(version, &nixpkgs_path, package_names, eval_accessible_paths)? - } - } - }; - - match check_result { - Failure(errors) => { - for error in errors { - writeln!(error_writer, "{}", error.to_string().red())? + eval::check_values(&nixpkgs_path, package_names, eval_accessible_paths)? } - Ok(false) } - Success(_) => Ok(true), - } + }) } #[cfg(test)] mod tests { use crate::check_nixpkgs; use crate::utils; - use crate::Version; use anyhow::Context; use std::fs; use std::path::Path; @@ -200,7 +212,7 @@ mod tests { // We don't want coloring to mess up the tests let writer = temp_env::with_var("NO_COLOR", Some("1"), || -> anyhow::Result<_> { let mut writer = vec![]; - check_nixpkgs(&path, Version::V1, vec![&extra_nix_path], &mut writer) + check_nixpkgs(None, &path, vec![&extra_nix_path], &mut writer) .context(format!("Failed test case {name}"))?; Ok(writer) })?; diff --git a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs index 2344a8cc1325362..f3442fc7cf248a7 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs @@ -44,6 +44,10 @@ pub enum NixpkgsProblem { relative_package_file: PathBuf, package_name: String, }, + EmptyArgVersionDecrease { + relative_package_file: PathBuf, + package_name: String, + }, NonDerivation { relative_package_file: PathBuf, package_name: String, @@ -153,6 +157,12 @@ impl fmt::Display for NixpkgsProblem { "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::EmptyArgVersionDecrease { 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,