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

pkgs/test/nixpkgs-check-by-name/README.md

@@ -8,16 +8,10 @@ This is part of the implementation of [RFC 140](https://github.com/NixOS/rfcs/pu
 
 This API may be changed over time if the CI workflow making use of it is adjusted to deal with the change appropriately.
 
-- Command line: `nixpkgs-check-by-name <NIXPKGS>`
+- Command line: `nixpkgs-check-by-name [--base <BASE_NIXPKGS>] <NIXPKGS>`
 - Arguments:
+  - `<BASE_NIXPKGS>`: The path to the Nixpkgs to check against
   - `<NIXPKGS>`: The path to the Nixpkgs to check
-  - `--version <VERSION>`: The version of the checks to perform.
-
-    Possible values:
-    - `v0` (default)
-    - `v1`
-
-    See [validation](#validity-checks) for the differences.
 - Exit code:
   - `0`: If the [validation](#validity-checks) is successful
   - `1`: If the [validation](#validity-checks) is not successful
@@ -42,7 +36,8 @@ These checks are performed by this tool:
 
 ### Nix evaluation checks
 - `pkgs.${name}` is defined as `callPackage pkgs/by-name/${shard}/${name}/package.nix args` for some `args`.
-  - **Only after --version v1**: If `pkgs.${name}` is not auto-called from `pkgs/by-name`, `args` must not be empty
+  - If `pkgs.${name}` is not auto-called from `pkgs/by-name`, `args` must not be empty,
+    with the exception that if `BASE_NIXPKGS` also has a definition for the same package with empty `args`, it's allowed
 - `pkgs.lib.isDerivation pkgs.${name}` is `true`.
 
 ## Development
@@ -86,6 +81,10 @@ Tests are declared in [`./tests`](./tests) as subdirectories imitating Nixpkgs w
   allowing the simulation of package overrides to the real [`pkgs/top-level/all-packages.nix`](../../top-level/all-packages.nix`).
   The default is an empty overlay.
 
+- `base` (optional):
+  Contains another subdirectory imitating Nixpkgs with potentially any of the above structures.
+  This will be used as the `--base` argument, allowing tests of gradual transitions.
+
 - `expected` (optional):
   A file containing the expected standard output.
   The default is expecting an empty standard output.

pkgs/test/nixpkgs-check-by-name/src/eval.rs

@@ -1,7 +1,7 @@
 use crate::nixpkgs_problem::NixpkgsProblem;
 use crate::structure;
 use crate::validation::{self, Validation::Success};
-use crate::Version;
+use crate::version;
 use std::path::Path;
 
 use anyhow::Context;
@@ -39,11 +39,10 @@ enum AttributeVariant {
 /// 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<version::Nixpkgs> {
     // 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,54 +109,67 @@ pub fn check_values(
             String::from_utf8_lossy(&result.stdout)
         ))?;
 
-    Ok(validation::sequence_(package_names.iter().map(
-        |package_name| {
+    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 check_result = if !attribute_info.is_derivation {
+                    NixpkgsProblem::NonDerivation {
+                        relative_package_file: relative_package_file.clone(),
+                        package_name: package_name.clone(),
+                    }
+                    .into()
+                } else {
+                    Success(())
+                };
+
+                let check_result = check_result.and(match &attribute_info.variant {
+                    AttributeVariant::AutoCalled => Success(version::Attribute {
+                        empty_non_auto_called: version::EmptyNonAutoCalled::Valid,
+                    }),
                     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 correct_file {
+                            Success(version::Attribute {
+                                empty_non_auto_called: if *empty_arg {
+                                    version::EmptyNonAutoCalled::Invalid
+                                } else {
+                                    version::EmptyNonAutoCalled::Valid
+                                },
+                            })
+                        } else {
+                            NixpkgsProblem::WrongCallPackage {
+                                relative_package_file: relative_package_file.clone(),
+                                package_name: package_name.clone(),
+                            }
+                            .into()
+                        }
                     }
-                    .into()
-                } else if !attribute_info.is_derivation {
-                    NixpkgsProblem::NonDerivation {
+                    AttributeVariant::Other => NixpkgsProblem::WrongCallPackage {
                         relative_package_file: relative_package_file.clone(),
                         package_name: package_name.clone(),
                     }
-                    .into()
-                } else {
-                    Success(())
-                }
+                    .into(),
+                });
+
+                check_result.map(|value| (package_name.clone(), value))
             } else {
                 NixpkgsProblem::UndefinedAttr {
                     relative_package_file: relative_package_file.clone(),
                     package_name: package_name.clone(),
                 }
                 .into()
             }
-        },
-    )))
+        }))
+        .map(|elems| version::Nixpkgs {
+            attributes: elems.into_iter().collect(),
+        }),
+    )
 }

pkgs/test/nixpkgs-check-by-name/src/main.rs

@@ -4,12 +4,14 @@ mod references;
 mod structure;
 mod utils;
 mod validation;
+mod version;
 
 use crate::structure::check_structure;
 use crate::validation::Validation::Failure;
 use crate::validation::Validation::Success;
+use crate::version::Nixpkgs;
 use anyhow::Context;
-use clap::{Parser, ValueEnum};
+use clap::Parser;
 use colored::Colorize;
 use std::io;
 use std::path::{Path, PathBuf};
@@ -21,25 +23,20 @@ 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 process(
+        args.base.as_deref(),
+        &args.nixpkgs,
+        &vec![],
+        &mut io::stderr(),
+    ) {
         Ok(true) => {
             eprintln!("{}", "Validated successfully".green());
             ExitCode::SUCCESS
@@ -55,10 +52,11 @@ fn main() -> ExitCode {
     }
 }
 
-/// Checks whether the pkgs/by-name structure in Nixpkgs is valid.
+/// Does the actual work. This is the abstraction used both by `main` and the tests.
 ///
 /// # Arguments
-/// - `nixpkgs_path`: The path to the Nixpkgs to check
+/// - `base_nixpkgs`: The path to the base Nixpkgs to compare against
+/// - `main_nixpkgs`: The path to the main Nixpkgs to check
 /// - `eval_accessible_paths`:
 ///   Extra paths that need to be accessible to evaluate Nixpkgs using `restrict-eval`.
 ///   This is used to allow the tests to access the mock-nixpkgs.nix file
@@ -68,33 +66,25 @@ fn main() -> ExitCode {
 /// - `Err(e)` if an I/O-related error `e` occurred.
 /// - `Ok(false)` if there are problems, all of which will be written to `error_writer`.
 /// - `Ok(true)` if there are no problems
-pub fn check_nixpkgs<W: io::Write>(
-    nixpkgs_path: &Path,
-    version: Version,
-    eval_accessible_paths: Vec<&Path>,
+pub fn process<W: io::Write>(
+    base_nixpkgs: Option<&Path>,
+    main_nixpkgs: &Path,
+    eval_accessible_paths: &Vec<&Path>,
     error_writer: &mut W,
 ) -> anyhow::Result<bool> {
-    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() {
-        eprintln!(
-            "Given Nixpkgs path does not contain a {} subdirectory, no check necessary.",
-            utils::BASE_SUBPATH
-        );
-        Success(())
-    } 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)?
-            }
+    let main_result = check_nixpkgs(main_nixpkgs, eval_accessible_paths)?;
+    let check_result = main_result.result_map(|nixpkgs_version| {
+        if let Some(base) = base_nixpkgs {
+            check_nixpkgs(base, eval_accessible_paths)?.result_map(|base_nixpkgs_version| {
+                Ok(Nixpkgs::compare(base_nixpkgs_version, nixpkgs_version))
+            })
+        } else {
+            Ok(Nixpkgs::compare(
+                version::Nixpkgs::default(),
+                nixpkgs_version,
+            ))
         }
-    };
+    })?;
 
     match check_result {
         Failure(errors) => {
@@ -103,15 +93,40 @@ pub fn check_nixpkgs<W: io::Write>(
             }
             Ok(false)
         }
-        Success(_) => Ok(true),
+        Success(()) => Ok(true),
     }
 }
 
+/// 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(
+    nixpkgs_path: &Path,
+    eval_accessible_paths: &Vec<&Path>,
+) -> validation::Result<version::Nixpkgs> {
+    Ok({
+        let nixpkgs_path = nixpkgs_path.canonicalize().context(format!(
+            "Nixpkgs path {} could not be resolved",
+            nixpkgs_path.display()
+        ))?;
+
+        if !nixpkgs_path.join(utils::BASE_SUBPATH).exists() {
+            eprintln!(
+                "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|
+                // Only if we could successfully parse the structure, we do the evaluation checks
+                eval::check_values(&nixpkgs_path, package_names, eval_accessible_paths))?
+        }
+    })
+}
+
 #[cfg(test)]
 mod tests {
-    use crate::check_nixpkgs;
+    use crate::process;
     use crate::utils;
-    use crate::Version;
     use anyhow::Context;
     use std::fs;
     use std::path::Path;
@@ -197,10 +212,17 @@ mod tests {
     fn test_nixpkgs(name: &str, path: &Path, expected_errors: &str) -> anyhow::Result<()> {
         let extra_nix_path = Path::new("tests/mock-nixpkgs.nix");
 
+        let base_path = path.join("base");
+        let base_nixpkgs = if base_path.exists() {
+            Some(base_path.as_path())
+        } else {
+            None
+        };
+
         // 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)
+            process(base_nixpkgs, &path, &vec![&extra_nix_path], &mut writer)
                 .context(format!("Failed test case {name}"))?;
             Ok(writer)
         })?;

pkgs/test/nixpkgs-check-by-name/src/validation.rs

@@ -58,6 +58,15 @@ impl<A> Validation<A> {
             Success(value) => Success(f(value)),
         }
     }
+
+    /// Map a `Validation<A>` to a `Result<B>` by applying a function `A -> Result<B>`
+    /// only if there is a `Success` value
+    pub fn result_map<B>(self, f: impl FnOnce(A) -> Result<B>) -> Result<B> {
+        match self {
+            Failure(err) => Ok(Failure(err)),
+            Success(value) => f(value),
+        }
+    }
 }
 
 impl Validation<()> {