diff --git a/pkgs/test/nixpkgs-check-by-name/README.md b/pkgs/test/nixpkgs-check-by-name/README.md index 8ed23204decad..7e8d39104e48b 100644 --- a/pkgs/test/nixpkgs-check-by-name/README.md +++ b/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 ] ` - Arguments: - - ``: The path to the Nixpkgs to check. - - ``: The path to the Nixpkgs to use as the base to compare `` 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. + - ``: + The path to the Nixpkgs to check. + For PRs, this should be set to a checkout of the PR branch. + - ``: + 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 ``, which is then checked against `` 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. diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index 20652d9ede263..cd8c70472cf25 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/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, eval_accessible_paths: &[&Path], -) -> validation::Result { +) -> validation::Result { // 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(), }), ) } diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index ee73ffbd0f8d0..01f7d4b71982a 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/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, } @@ -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( eval_accessible_paths: &[&Path], error_writer: &mut W, ) -> anyhow::Result { + // 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( /// 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( nixpkgs_path: &Path, eval_accessible_paths: &[&Path], error_writer: &mut W, -) -> validation::Result { +) -> validation::Result { Ok({ let nixpkgs_path = nixpkgs_path.canonicalize().context(format!( "Nixpkgs path {} could not be resolved", @@ -118,7 +122,7 @@ pub fn check_nixpkgs( "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 diff --git a/pkgs/test/nixpkgs-check-by-name/src/version.rs b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs similarity index 61% rename from pkgs/test/nixpkgs-check-by-name/src/version.rs rename to pkgs/test/nixpkgs-check-by-name/src/ratchet.rs index c82f537c504b8..c12f1ead25402 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/version.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs @@ -1,30 +1,28 @@ +//! 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, + /// The ratchet values for each package in `pkgs/by-name` + pub packages: HashMap, } 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, 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 @@ -32,22 +30,24 @@ impl Nixpkgs { // 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 {