Merge pull request #272395 from tweag/by-name-migrate-empty-arg

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

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

@@ -4,35 +4,22 @@ This directory implements a program to check the [validity](#validity-checks) of
 It is being used by [this GitHub Actions workflow](../../../.github/workflows/check-by-name.yml).
 This is part of the implementation of [RFC 140](https://github.com/NixOS/rfcs/pull/140).
 
-## API
+## Interface
 
-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>`
-- Arguments:
-  - `<NIXPKGS>`: The path to the Nixpkgs to check
-  - `--version <VERSION>`: The version of the checks to perform.
-
-    Possible values:
-    - `v0` (default)
-    - `v1`
+The interface of the tool is shown with `--help`:
+```
+cargo run -- --help
+```
 
-    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
-  - `2`: If an unexpected I/O error occurs
-- Standard error:
-  - Informative messages
-  - Detected problems if validation is not successful
+The interface may be changed over time only if the CI workflow making use of it is adjusted to deal with the change appropriately.
 
 ## Validity checks
 
 These checks are performed by this tool:
 
 ### File structure checks
 - `pkgs/by-name` must only contain subdirectories of the form `${shard}/${name}`, called _package directories_.
-- The `name`'s of package directories must be unique when lowercased
+- The `name`'s of package directories must be unique when lowercased.
 - `name` is a string only consisting of the ASCII characters `a-z`, `A-Z`, `0-9`, `-` or `_`.
 - `shard` is the lowercased first two letters of `name`, expressed in Nix: `shard = toLower (substring 0 2 name)`.
 - Each package directory must contain a `package.nix` file and may contain arbitrary other files.
@@ -41,9 +28,21 @@ These checks are performed by this tool:
 - Each package directory must not refer to files outside itself using symlinks or Nix path expressions.
 
 ### 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
-- `pkgs.lib.isDerivation pkgs.${name}` is `true`.
+- For each package directory, the `pkgs.${name}` attribute must be defined as `callPackage pkgs/by-name/${shard}/${name}/package.nix args` for some `args`.
+- For each package directory, `pkgs.lib.isDerivation pkgs.${name}` must be `true`.
+
+### Ratchet checks
+
+Furthermore, this tool implements certain [ratchet](https://qntm.org/ratchet) checks.
+This allows gradually phasing out deprecated patterns without breaking the base branch or having to migrate it all at once.
+It works by not allowing new instances of the pattern to be introduced, but allowing already existing instances.
+The existing instances are coming from `<BASE_NIXPKGS>`, which is then checked against `<NIXPKGS>` for new instances.
+Ratchets should be removed eventually once the pattern is not used anymore.
+
+The current ratchets are:
+
+- New manual definitions of `pkgs.${name}` (e.g. in `pkgs/top-level/all-packages.nix`) with `args = { }`
+  (see [nix evaluation checks](#nix-evaluation-checks)) must not be introduced.
 
 ## Development
 
@@ -86,6 +85,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 is used for [ratchet checks](#ratchet-checks).
+
 - `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::ratchet;
 use crate::structure;
 use crate::validation::{self, Validation::Success};
-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: &[&Path],
+) -> validation::Result<ratchet::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,68 @@ pub fn check_values(
             String::from_utf8_lossy(&result.stdout)
         ))?;
 
-    Ok(validation::sequence_(package_names.iter().map(
-        |package_name| {
-            let relative_package_file = structure::relative_file_for_package(package_name);
+    Ok(
+        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) {
-                let valid = match &attribute_info.variant {
-                    AttributeVariant::AutoCalled => true,
+            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(),
+                        package_name: package_name.clone(),
+                    }
+                    .into()
+                } else {
+                    Success(())
+                };
+
+                let check_result = check_result.and(match &attribute_info.variant {
+                    AttributeVariant::AutoCalled => Success(ratchet::Package {
+                        empty_non_auto_called: ratchet::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(ratchet::Package {
+                                // Empty arguments for non-auto-called packages are not allowed anymore.
+                                empty_non_auto_called: if *empty_arg {
+                                    ratchet::EmptyNonAutoCalled::Invalid
+                                } else {
+                                    ratchet::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| ratchet::Nixpkgs {
+            packages: elems.into_iter().collect(),
+        }),
+    )
 }