tests.nixpkgs-check-by-name: Gradual migration from base Nixpkgs

This implements the option for a gradual migration to stricter checks.
For now this is only done for the check against empty non-auto-called
callPackage arguments, but in the future this can be used to ensure all
new packages make use of `pkgs/by-name`.

This is implemented by adding a `--base <BASE_NIXPKGS>` flag, which then
compares the base nixpkgs against the main nixpkgs version, making sure
that there are no regressions.

The `--version` flag is removed. While it was implemented, it was never
used in CI, so this is fine.

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

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<String>,
-    eval_accessible_paths: Vec<&Path>,
+    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.

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

@@ -9,8 +9,9 @@ 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};
@@ -22,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 process(&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
@@ -59,7 +55,8 @@ fn main() -> ExitCode {
 /// 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
@@ -70,21 +67,23 @@ 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 process<W: io::Write>(
-    nixpkgs_path: &Path,
-    version: Version,
-    eval_accessible_paths: Vec<&Path>,
+    base_nixpkgs: Option<&Path>,
+    main_nixpkgs: &Path,
+    eval_accessible_paths: &Vec<&Path>,
     error_writer: &mut W,
 ) -> anyhow::Result<bool> {
-    let nixpkgs_result = check_nixpkgs(nixpkgs_path, eval_accessible_paths)?;
-    let check_result = nixpkgs_result.result_map(|nixpkgs_version| {
-        let empty_non_auto_called_base = match version {
-            Version::V0 => version::EmptyNonAutoCalled::Invalid,
-            Version::V1 => version::EmptyNonAutoCalled::Valid,
-        };
-        Ok(version::Nixpkgs::compare(
-            &empty_non_auto_called_base,
+    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 {
@@ -94,7 +93,7 @@ pub fn process<W: io::Write>(
             }
             Ok(false)
         }
-        Success(_) => Ok(true),
+        Success(()) => Ok(true),
     }
 }
 
@@ -102,7 +101,7 @@ pub fn process<W: io::Write>(
 /// 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>,
+    eval_accessible_paths: &Vec<&Path>,
 ) -> validation::Result<version::Nixpkgs> {
     Ok({
         let nixpkgs_path = nixpkgs_path.canonicalize().context(format!(
@@ -128,7 +127,6 @@ pub fn check_nixpkgs(
 mod tests {
     use crate::process;
     use crate::utils;
-    use crate::Version;
     use anyhow::Context;
     use std::fs;
     use std::path::Path;
@@ -217,7 +215,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(&path, Version::V1, vec![&extra_nix_path], &mut writer)
+            process(None, &path, &vec![&extra_nix_path], &mut writer)
                 .context(format!("Failed test case {name}"))?;
             Ok(writer)
         })?;

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

@@ -16,12 +16,12 @@ 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.
-    pub fn compare(empty_non_auto_called_from: &EmptyNonAutoCalled, to: Self) -> Validation<()> {
+    pub fn compare(from: Self, to: Self) -> Validation<()> {
         validation::sequence_(
             // We only loop over the current attributes,
             // we don't need to check ones that were removed
             to.attributes.into_iter().map(|(name, attr_to)| {
-                Attribute::compare(&name, empty_non_auto_called_from, &attr_to)
+                Attribute::compare(&name, from.attributes.get(&name), &attr_to)
             }),
         )
     }
@@ -33,12 +33,12 @@ pub struct Attribute {
 }
 
 impl Attribute {
-    pub fn compare(
-        name: &str,
-        empty_non_auto_called_from: &EmptyNonAutoCalled,
-        to: &Self,
-    ) -> Validation<()> {
-        EmptyNonAutoCalled::compare(name, empty_non_auto_called_from, &to.empty_non_auto_called)
+    pub fn compare(name: &str, optional_from: Option<&Self>, to: &Self) -> Validation<()> {
+        EmptyNonAutoCalled::compare(
+            name,
+            optional_from.map(|x| &x.empty_non_auto_called),
+            &to.empty_non_auto_called,
+        )
     }
 }
 
@@ -53,12 +53,9 @@ pub enum EmptyNonAutoCalled {
 }
 
 impl EmptyNonAutoCalled {
-    fn compare(
-        name: &str,
-        empty_non_auto_called_from: &EmptyNonAutoCalled,
-        to: &Self,
-    ) -> Validation<()> {
-        if to >= empty_non_auto_called_from {
+    fn compare(name: &str, optional_from: Option<&Self>, to: &Self) -> Validation<()> {
+        let from = optional_from.unwrap_or(&Self::Valid);
+        if to >= from {
             Success(())
         } else {
             NixpkgsProblem::WrongCallPackage {