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

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

@@ -10,8 +10,13 @@ This API may be changed over time if the CI workflow making use of it is adjuste
 
 - 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
+  - `<NIXPKGS>`: The path to the Nixpkgs to check.
+  - `<BASE_NIXPKGS>`: The path to the Nixpkgs to use as the base to compare `<NIXPKGS>` against.
+    This allows the strictness of checks to increase over time by only preventing _new_ violations from being introduced,
+    while allowing violations that already existed.
+
+    If not specified, all violations of stricter checks are allowed.
+    However, this flag will become required once CI passes it.
 - Exit code:
   - `0`: If the [validation](#validity-checks) is successful
   - `1`: If the [validation](#validity-checks) is not successful

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: &[&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.
@@ -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(),

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,20 +64,22 @@ fn main() -> ExitCode {
 pub fn process<W: io::Write>(
     base_nixpkgs: Option<&Path>,
     main_nixpkgs: &Path,
-    eval_accessible_paths: &Vec<&Path>,
+    eval_accessible_paths: &[&Path],
     error_writer: &mut W,
 ) -> anyhow::Result<bool> {
-    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)?.result_map(|base_nixpkgs_version| {
-                Ok(Nixpkgs::compare(base_nixpkgs_version, nixpkgs_version))
-            })
+            check_nixpkgs(base, eval_accessible_paths, error_writer)?.result_map(
+                |base_nixpkgs_version| {
+                    Ok(Nixpkgs::compare(
+                        Some(base_nixpkgs_version),
+                        nixpkgs_version,
+                    ))
+                },
+            )
         } else {
-            Ok(Nixpkgs::compare(
-                version::Nixpkgs::default(),
-                nixpkgs_version,
-            ))
+            Ok(Nixpkgs::compare(None, nixpkgs_version))
         }
     })?;
 
@@ -97,11 +94,17 @@ pub fn process<W: io::Write>(
     }
 }
 
-/// 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<W: io::Write>(
     nixpkgs_path: &Path,
-    eval_accessible_paths: &Vec<&Path>,
+    eval_accessible_paths: &[&Path],
+    error_writer: &mut W,
 ) -> validation::Result<version::Nixpkgs> {
     Ok({
         let nixpkgs_path = nixpkgs_path.canonicalize().context(format!(
@@ -110,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|
@@ -222,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)
         })?;

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

@@ -16,12 +16,27 @@ 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(from: Self, to: Self) -> Validation<()> {
+    /// This enables a gradual transition from weaker to stricter checks, by only allowing PRs to
+    /// increase strictness.
+    pub fn compare(optional_from: Option<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, from.attributes.get(&name), &attr_to)
+                let attr_from = if let Some(from) = &optional_from {
+                    from.attributes.get(&name)
+                } else {
+                    // This pretends that if there's no base version to compare against, all
+                    // attributes existed without conforming to the new strictness check for
+                    // backwards compatibility.
+                    // TODO: Remove this case. This is only needed because the `--base`
+                    // argument is still optional, which doesn't need to be once CI is updated
+                    // to pass it.
+                    Some(&Attribute {
+                        empty_non_auto_called: EmptyNonAutoCalled::Invalid,
+                    })
+                };
+                Attribute::compare(&name, attr_from, &attr_to)
             }),
         )
     }

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.

pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/base/default.nix

@@ -0,0 +1 @@
+import ../../mock-nixpkgs.nix { root = ./.; }

pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/base/pkgs/by-name/README.md

@@ -0,0 +1 @@
+(this is just here so the directory can get tracked by git)