tests.nixpkgs-check-by-name: Improve docs, introduce "ratchet" term

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

@@ -10,13 +10,15 @@ 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:
-  - `<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.
+  - `<NIXPKGS>`:
+    The path to the Nixpkgs to check.
+    For PRs, this should be set to a checkout of the PR branch.
+  - `<BASE_NIXPKGS>`:
+    The path to the Nixpkgs to use as the [ratchet check](#ratchet-checks) base.
+    For PRs, this should be set to a checkout of the PRs base branch.
+
+    If not specified, no ratchet checks will be performed.
+    However, this flag will become required once CI uses it.
 - Exit code:
   - `0`: If the [validation](#validity-checks) is successful
   - `1`: If the [validation](#validity-checks) is not successful
@@ -41,10 +43,20 @@ 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`.
-  - 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`.
 
+### 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:
+
+- If `pkgs.${name}` is not auto-called from `pkgs/by-name`, the `args` in its `callPackage` must not be empty,
+
 ## Development
 
 Enter the development environment in this directory either automatically with `direnv` or with
@@ -88,7 +100,7 @@ Tests are declared in [`./tests`](./tests) as subdirectories imitating Nixpkgs w
 
 - `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.
+  This is used for [ratchet checks](#ratchet-checks).
 
 - `expected` (optional):
   A file containing the expected 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;
@@ -42,7 +42,7 @@ pub fn check_values(
     nixpkgs_path: &Path,
     package_names: Vec<String>,
     eval_accessible_paths: &[&Path],
-) -> validation::Result<version::Nixpkgs> {
+) -> 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")?;
@@ -126,8 +126,8 @@ pub fn check_values(
                 };
 
                 let check_result = check_result.and(match &attribute_info.variant {
-                    AttributeVariant::AutoCalled => Success(version::Attribute {
-                        empty_non_auto_called: version::EmptyNonAutoCalled::Valid,
+                    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 {
@@ -137,11 +137,12 @@ pub fn check_values(
                         };
 
                         if correct_file {
-                            Success(version::Attribute {
+                            Success(ratchet::Package {
+                                // Empty arguments for non-auto-called packages are not allowed anymore.
                                 empty_non_auto_called: if *empty_arg {
-                                    version::EmptyNonAutoCalled::Invalid
+                                    ratchet::EmptyNonAutoCalled::Invalid
                                 } else {
-                                    version::EmptyNonAutoCalled::Valid
+                                    ratchet::EmptyNonAutoCalled::Valid
                                 },
                             })
                         } else {
@@ -168,8 +169,8 @@ pub fn check_values(
                 .into()
             }
         }))
-        .map(|elems| version::Nixpkgs {
-            attributes: elems.into_iter().collect(),
+        .map(|elems| ratchet::Nixpkgs {
+            packages: elems.into_iter().collect(),
         }),
     )
 }

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

@@ -1,15 +1,14 @@
 mod eval;
 mod nixpkgs_problem;
+mod ratchet;
 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;
 use colored::Colorize;
@@ -21,10 +20,14 @@ use std::process::ExitCode;
 #[derive(Parser, Debug)]
 #[command(about)]
 pub struct Args {
-    /// Path to nixpkgs
+    /// Path to the main Nixpkgs to check.
+    /// For PRs, this should be set to a checkout of the PR branch.
     nixpkgs: PathBuf,
 
-    /// Path to the base Nixpkgs to compare against
+    /// Path to the base Nixpkgs to run ratchet checks against.
+    /// For PRs, this should be set to a checkout of the PRs base branch.
+    /// If not specified, no ratchet checks will be performed.
+    /// However, this flag will become required once CI uses it.
     #[arg(long)]
     base: Option<PathBuf>,
 }
@@ -50,8 +53,8 @@ fn main() -> ExitCode {
 /// Does the actual work. This is the abstraction used both by `main` and the tests.
 ///
 /// # Arguments
-/// - `base_nixpkgs`: The path to the base Nixpkgs to compare against
-/// - `main_nixpkgs`: The path to the main Nixpkgs to check
+/// - `base_nixpkgs`: Path to the base Nixpkgs to run ratchet checks against.
+/// - `main_nixpkgs`: 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
@@ -67,19 +70,22 @@ pub fn process<W: io::Write>(
     eval_accessible_paths: &[&Path],
     error_writer: &mut W,
 ) -> anyhow::Result<bool> {
+    // Check the main Nixpkgs first
     let main_result = check_nixpkgs(main_nixpkgs, eval_accessible_paths, error_writer)?;
     let check_result = main_result.result_map(|nixpkgs_version| {
+        // If the main Nixpkgs doesn't have any problems, run the ratchet checks against the base
+        // Nixpkgs
         if let Some(base) = base_nixpkgs {
             check_nixpkgs(base, eval_accessible_paths, error_writer)?.result_map(
                 |base_nixpkgs_version| {
-                    Ok(Nixpkgs::compare(
+                    Ok(ratchet::Nixpkgs::compare(
                         Some(base_nixpkgs_version),
                         nixpkgs_version,
                     ))
                 },
             )
         } else {
-            Ok(Nixpkgs::compare(None, nixpkgs_version))
+            Ok(ratchet::Nixpkgs::compare(None, nixpkgs_version))
         }
     })?;
 
@@ -96,16 +102,14 @@ pub fn process<W: io::Write>(
 
 /// 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.
+/// This does not include ratchet checks, see ../README.md#ratchet-checks
+/// Instead a `ratchet::Nixpkgs` value is returned, whose `compare` method allows performing the
+/// ratchet check against another result.
 pub fn check_nixpkgs<W: io::Write>(
     nixpkgs_path: &Path,
     eval_accessible_paths: &[&Path],
     error_writer: &mut W,
-) -> validation::Result<version::Nixpkgs> {
+) -> validation::Result<ratchet::Nixpkgs> {
     Ok({
         let nixpkgs_path = nixpkgs_path.canonicalize().context(format!(
             "Nixpkgs path {} could not be resolved",
@@ -118,7 +122,7 @@ pub fn check_nixpkgs<W: io::Write>(
                 "Given Nixpkgs path does not contain a {} subdirectory, no check necessary.",
                 utils::BASE_SUBPATH
             )?;
-            Success(version::Nixpkgs::default())
+            Success(ratchet::Nixpkgs::default())
         } else {
             check_structure(&nixpkgs_path)?.result_map(|package_names|
                 // Only if we could successfully parse the structure, we do the evaluation checks

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

@@ -1,53 +1,53 @@
+//! This module implements the ratchet checks, see ../README.md#ratchet-checks
+//!
+//! Each type has a `compare` method that validates the ratchet checks for that item.
+
 use crate::nixpkgs_problem::NixpkgsProblem;
 use crate::structure;
 use crate::validation::{self, Validation, Validation::Success};
 use std::collections::HashMap;
 
-/// The check version conformity of a Nixpkgs path:
-/// When the strictness of the check increases, this structure should be extended to distinguish
-/// between parts that are still valid, and ones that aren't valid anymore.
+/// The ratchet value for the entirety of Nixpkgs.
 #[derive(Default)]
 pub struct Nixpkgs {
-    /// The package attributes tracked in `pkgs/by-name`
-    pub attributes: HashMap<String, Attribute>,
+    /// The ratchet values for each package in `pkgs/by-name`
+    pub packages: HashMap<String, Package>,
 }
 
 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.
-    /// This enables a gradual transition from weaker to stricter checks, by only allowing PRs to
-    /// increase strictness.
+    /// Validates the ratchet checks for Nixpkgs
     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)| {
+            to.packages.into_iter().map(|(name, attr_to)| {
                 let attr_from = if let Some(from) = &optional_from {
-                    from.attributes.get(&name)
+                    from.packages.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 {
+                    Some(&Package {
                         empty_non_auto_called: EmptyNonAutoCalled::Invalid,
                     })
                 };
-                Attribute::compare(&name, attr_from, &attr_to)
+                Package::compare(&name, attr_from, &attr_to)
             }),
         )
     }
 }
 
-/// The check version conformity of an attribute defined by `pkgs/by-name`
-pub struct Attribute {
+/// The ratchet value for a single package in `pkgs/by-name`
+pub struct Package {
+    /// The ratchet value for the check for non-auto-called empty arguments
     pub empty_non_auto_called: EmptyNonAutoCalled,
 }
 
-impl Attribute {
+impl Package {
+    /// Validates the ratchet checks for a single package defined in `pkgs/by-name`
     pub fn compare(name: &str, optional_from: Option<&Self>, to: &Self) -> Validation<()> {
         EmptyNonAutoCalled::compare(
             name,
@@ -57,17 +57,19 @@ impl Attribute {
     }
 }
 
-/// Whether an attribute conforms to the new strictness check that
-/// `callPackage ... {}` is not allowed anymore in `all-package.nix`
+/// The ratchet value of a single package in `pkgs/by-name`
+/// for the non-auto-called empty argument check of a single.
+///
+/// This checks that packages defined in `pkgs/by-name` cannot be overridden
+/// with an empty second argument like `callPackage ... { }`.
 #[derive(PartialEq, PartialOrd)]
 pub enum EmptyNonAutoCalled {
-    /// The attribute is not valid anymore with the new check
     Invalid,
-    /// The attribute is still valid with the new check
     Valid,
 }
 
 impl EmptyNonAutoCalled {
+    /// Validates the non-auto-called empty argument ratchet check for a single package defined in `pkgs/by-name`
     fn compare(name: &str, optional_from: Option<&Self>, to: &Self) -> Validation<()> {
         let from = optional_from.unwrap_or(&Self::Valid);
         if to >= from {