tests.nixpkgs-check-by-name: Implement gradual empty arg check migration

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<String, AttributeResult>,
+}
+
+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 <package_file> { ... }`.
 /// 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<String>,
-    eval_accessible_paths: Vec<&Path>,
-) -> validation::Result<()> {
+    eval_accessible_paths: &Vec<&Path>,
+) -> validation::Result<Attributes> {
     // 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() }))
 }

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<PathBuf>,
 }
 
 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<W: io::Write>(
-    nixpkgs_path: &Path,
-    version: Version,
+    base_nixpkgs: Option<&Path>,
+    pr_nixpkgs: &Path,
     eval_accessible_paths: Vec<&Path>,
     error_writer: &mut W,
 ) -> anyhow::Result<bool> {
+
+    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<eval::Attributes> {
     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)
         })?;

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,