From b8e4d555b4936adf93510c9e333798eecf32b5e7 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Thu, 14 Dec 2023 02:07:50 +0100 Subject: [PATCH 01/10] tests.nixpkgs-check-by-name: Minor refactor, allow more simultaneous problems This makes it such that these two errors can both be thrown for a single package: - The attribute value not being a derivation - The attribute not being a proper callPackage The tests had to be adjusted to only throw the error they were testing for --- pkgs/test/nixpkgs-check-by-name/src/eval.rs | 42 +++++++++++-------- .../override-no-call-package/all-packages.nix | 2 +- .../tests/override-no-file/all-packages.nix | 2 +- 3 files changed, 26 insertions(+), 20 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index 161d013374e7..08dc243359d5 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs @@ -116,8 +116,18 @@ pub fn check_values( 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, + 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(()) + }; + + check_result.and(match &attribute_info.variant { + AttributeVariant::AutoCalled => Success(()), AttributeVariant::CallPackage { path, empty_arg } => { let correct_file = if let Some(call_package_path) = path { absolute_package_file == *call_package_path @@ -131,26 +141,22 @@ pub fn check_values( } else { true }; - correct_file && non_empty + if correct_file && non_empty { + Success(()) + } else { + NixpkgsProblem::WrongCallPackage { + relative_package_file: relative_package_file.clone(), + package_name: package_name.clone(), + } + .into() + } } - AttributeVariant::Other => false, - }; - - if !valid { - NixpkgsProblem::WrongCallPackage { + AttributeVariant::Other => NixpkgsProblem::WrongCallPackage { relative_package_file: relative_package_file.clone(), package_name: package_name.clone(), } - .into() - } else if !attribute_info.is_derivation { - NixpkgsProblem::NonDerivation { - relative_package_file: relative_package_file.clone(), - package_name: package_name.clone(), - } - .into() - } else { - Success(()) - } + .into(), + }) } else { NixpkgsProblem::UndefinedAttr { relative_package_file: relative_package_file.clone(), diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/all-packages.nix b/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/all-packages.nix index 4fad280ae1c7..853c3a87db56 100644 --- a/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/all-packages.nix +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/all-packages.nix @@ -1,3 +1,3 @@ self: super: { - nonDerivation = null; + nonDerivation = self.someDrv; } diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-no-file/all-packages.nix b/pkgs/test/nixpkgs-check-by-name/tests/override-no-file/all-packages.nix index 4c521d2d4468..dc07f69b40ee 100644 --- a/pkgs/test/nixpkgs-check-by-name/tests/override-no-file/all-packages.nix +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-no-file/all-packages.nix @@ -1,3 +1,3 @@ self: super: { - nonDerivation = self.callPackage ({ }: { }) { }; + nonDerivation = self.callPackage ({ someDrv }: someDrv) { }; } From e98d22851b67a6125683f80735e4bc1042252aef Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Thu, 14 Dec 2023 03:03:04 +0100 Subject: [PATCH 02/10] tests.nixpkgs-check-by-name: Introduce result_map Convenience function to run another validation over a successful validation result. This will be usable in more locations in future commits, making the code nicer. --- pkgs/test/nixpkgs-check-by-name/src/main.rs | 9 ++------- pkgs/test/nixpkgs-check-by-name/src/validation.rs | 9 +++++++++ 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index 4cabf8f446f5..567da00333e6 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs @@ -86,14 +86,9 @@ pub fn check_nixpkgs( ); Success(()) } else { - match check_structure(&nixpkgs_path)? { - Failure(errors) => Failure(errors), - Success(package_names) => + check_structure(&nixpkgs_path)?.result_map(|package_names| // Only if we could successfully parse the structure, we do the evaluation checks - { - eval::check_values(version, &nixpkgs_path, package_names, eval_accessible_paths)? - } - } + eval::check_values(version, &nixpkgs_path, package_names, eval_accessible_paths))? }; match check_result { diff --git a/pkgs/test/nixpkgs-check-by-name/src/validation.rs b/pkgs/test/nixpkgs-check-by-name/src/validation.rs index e72793851521..b14bfb92eb2e 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/validation.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/validation.rs @@ -58,6 +58,15 @@ impl Validation { Success(value) => Success(f(value)), } } + + /// Map a `Validation` to a `Result` by applying a function `A -> Result` + /// only if there is a `Success` value + pub fn result_map(self, f: impl FnOnce(A) -> Result) -> Result { + match self { + Failure(err) => Ok(Failure(err)), + Success(value) => f(value), + } + } } impl Validation<()> { From a6ba4cae311698ea907ee239785f75d76bd01e4b Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Thu, 14 Dec 2023 02:47:44 +0100 Subject: [PATCH 03/10] tests.nixpkgs-check-by-name: Intermediate refactor This prepares the code base for the removal of the `--version` flag, to be replaced with a flag that can specify a base version to compare the main Nixpkgs against, in order to have gradual transitions to stricter checks. This refactoring does: - Introduce the `version` module that can house the logic to increase strictness, with a `version::Nixpkgs` struct that contains the strictness conformity of a single Nixpkgs version - Make the check return `version::Nixpkgs` - Handle the behavior of the still-existing `--version` flag with `version::Nixpkgs` - Introduce an intermediate `process` function to handle the top-level logic, especially useful in the next commit --- pkgs/test/nixpkgs-check-by-name/src/eval.rs | 44 +++++++----- pkgs/test/nixpkgs-check-by-name/src/main.rs | 64 +++++++++++------ .../test/nixpkgs-check-by-name/src/version.rs | 71 +++++++++++++++++++ 3 files changed, 139 insertions(+), 40 deletions(-) create mode 100644 pkgs/test/nixpkgs-check-by-name/src/version.rs diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index 08dc243359d5..face1117f643 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::structure; use crate::validation::{self, Validation::Success}; -use crate::Version; +use crate::version; use std::path::Path; use anyhow::Context; @@ -39,11 +39,10 @@ enum AttributeVariant { /// of the form `callPackage { ... }`. /// 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, eval_accessible_paths: Vec<&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")?; @@ -110,8 +109,8 @@ pub fn check_values( String::from_utf8_lossy(&result.stdout) ))?; - Ok(validation::sequence_(package_names.iter().map( - |package_name| { + Ok( + validation::sequence(package_names.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); @@ -126,23 +125,25 @@ pub fn check_values( Success(()) }; - check_result.and(match &attribute_info.variant { - AttributeVariant::AutoCalled => Success(()), + let check_result = check_result.and(match &attribute_info.variant { + AttributeVariant::AutoCalled => Success(version::Attribute { + empty_non_auto_called: version::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 - }; - if correct_file && non_empty { - Success(()) + + if correct_file { + Success(version::Attribute { + empty_non_auto_called: if *empty_arg { + version::EmptyNonAutoCalled::Invalid + } else { + version::EmptyNonAutoCalled::Valid + }, + }) } else { NixpkgsProblem::WrongCallPackage { relative_package_file: relative_package_file.clone(), @@ -156,7 +157,9 @@ pub fn check_values( package_name: package_name.clone(), } .into(), - }) + }); + + check_result.map(|value| (package_name.clone(), value)) } else { NixpkgsProblem::UndefinedAttr { relative_package_file: relative_package_file.clone(), @@ -164,6 +167,9 @@ pub fn check_values( } .into() } - }, - ))) + })) + .map(|elems| version::Nixpkgs { + attributes: 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 567da00333e6..981d1134c85a 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs @@ -4,6 +4,7 @@ mod references; mod structure; mod utils; mod validation; +mod version; use crate::structure::check_structure; use crate::validation::Validation::Failure; @@ -39,7 +40,7 @@ pub enum Version { fn main() -> ExitCode { let args = Args::parse(); - match check_nixpkgs(&args.nixpkgs, args.version, vec![], &mut io::stderr()) { + match process(&args.nixpkgs, args.version, vec![], &mut io::stderr()) { Ok(true) => { eprintln!("{}", "Validated successfully".green()); ExitCode::SUCCESS @@ -55,7 +56,7 @@ fn main() -> ExitCode { } } -/// Checks whether the pkgs/by-name structure in Nixpkgs is valid. +/// 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 @@ -68,28 +69,23 @@ fn main() -> ExitCode { /// - `Err(e)` if an I/O-related error `e` occurred. /// - `Ok(false)` if there are problems, all of which will be written to `error_writer`. /// - `Ok(true)` if there are no problems -pub fn check_nixpkgs( +pub fn process( nixpkgs_path: &Path, version: Version, eval_accessible_paths: Vec<&Path>, error_writer: &mut W, ) -> anyhow::Result { - let nixpkgs_path = nixpkgs_path.canonicalize().context(format!( - "Nixpkgs path {} could not be resolved", - nixpkgs_path.display() - ))?; - - let check_result = if !nixpkgs_path.join(utils::BASE_SUBPATH).exists() { - eprintln!( - "Given Nixpkgs path does not contain a {} subdirectory, no check necessary.", - utils::BASE_SUBPATH - ); - Success(()) - } else { - check_structure(&nixpkgs_path)?.result_map(|package_names| - // Only if we could successfully parse the structure, we do the evaluation checks - eval::check_values(version, &nixpkgs_path, package_names, eval_accessible_paths))? - }; + 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, + nixpkgs_version, + )) + })?; match check_result { Failure(errors) => { @@ -102,9 +98,35 @@ pub fn check_nixpkgs( } } +/// 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( + nixpkgs_path: &Path, + eval_accessible_paths: Vec<&Path>, +) -> validation::Result { + Ok({ + let nixpkgs_path = nixpkgs_path.canonicalize().context(format!( + "Nixpkgs path {} could not be resolved", + nixpkgs_path.display() + ))?; + + if !nixpkgs_path.join(utils::BASE_SUBPATH).exists() { + eprintln!( + "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| + // Only if we could successfully parse the structure, we do the evaluation checks + eval::check_values(&nixpkgs_path, package_names, eval_accessible_paths))? + } + }) +} + #[cfg(test)] mod tests { - use crate::check_nixpkgs; + use crate::process; use crate::utils; use crate::Version; use anyhow::Context; @@ -195,7 +217,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![]; - check_nixpkgs(&path, Version::V1, vec![&extra_nix_path], &mut writer) + process(&path, Version::V1, vec![&extra_nix_path], &mut writer) .context(format!("Failed test case {name}"))?; Ok(writer) })?; diff --git a/pkgs/test/nixpkgs-check-by-name/src/version.rs b/pkgs/test/nixpkgs-check-by-name/src/version.rs new file mode 100644 index 000000000000..ab146270241e --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/src/version.rs @@ -0,0 +1,71 @@ +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. +#[derive(Default)] +pub struct Nixpkgs { + /// The package attributes tracked in `pkgs/by-name` + pub attributes: 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. + pub fn compare(empty_non_auto_called_from: &EmptyNonAutoCalled, 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) + }), + ) + } +} + +/// The check version conformity of an attribute defined by `pkgs/by-name` +pub struct Attribute { + pub empty_non_auto_called: EmptyNonAutoCalled, +} + +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) + } +} + +/// Whether an attribute conforms to the new strictness check that +/// `callPackage ... {}` is not allowed anymore in `all-package.nix` +#[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 { + fn compare( + name: &str, + empty_non_auto_called_from: &EmptyNonAutoCalled, + to: &Self, + ) -> Validation<()> { + if to >= empty_non_auto_called_from { + Success(()) + } else { + NixpkgsProblem::WrongCallPackage { + relative_package_file: structure::relative_file_for_package(name), + package_name: name.to_owned(), + } + .into() + } + } +} From d487a975ccb27302d1095ab56cd3c104712452c7 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Thu, 14 Dec 2023 03:11:14 +0100 Subject: [PATCH 04/10] 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 ` 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 | 13 ++-- pkgs/test/nixpkgs-check-by-name/src/eval.rs | 2 +- pkgs/test/nixpkgs-check-by-name/src/main.rs | 64 +++++++++---------- .../test/nixpkgs-check-by-name/src/version.rs | 25 ++++---- 4 files changed, 47 insertions(+), 57 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/README.md b/pkgs/test/nixpkgs-check-by-name/README.md index 146cea0a64ba..b098658fce4c 100644 --- a/pkgs/test/nixpkgs-check-by-name/README.md +++ b/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 ` +- Command line: `nixpkgs-check-by-name [--base ] ` - Arguments: + - ``: The path to the Nixpkgs to check against - ``: The path to the Nixpkgs to check - - `--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 diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index face1117f643..927e446b452f 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/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, - eval_accessible_paths: Vec<&Path>, + eval_accessible_paths: &Vec<&Path>, ) -> 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. diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index 981d1134c85a..53c24845cb20 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/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, } 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( - 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 { - 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, - nixpkgs_version, - )) + 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( } Ok(false) } - Success(_) => Ok(true), + Success(()) => Ok(true), } } @@ -102,7 +101,7 @@ pub fn process( /// 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 { 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) })?; diff --git a/pkgs/test/nixpkgs-check-by-name/src/version.rs b/pkgs/test/nixpkgs-check-by-name/src/version.rs index ab146270241e..7f83bdf3ff67 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/version.rs +++ b/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 { From bb08bfc2d37f9c1dbaf6f585fded65bf77a67e61 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Thu, 14 Dec 2023 04:01:54 +0100 Subject: [PATCH 05/10] tests.nixpkgs-check-by-name: Test for gradual transition This implements the ability to test gradual transitions in check strictness, and adds one such test for the empty non-auto-called arguments check. --- pkgs/test/nixpkgs-check-by-name/README.md | 4 ++++ pkgs/test/nixpkgs-check-by-name/src/main.rs | 9 ++++++++- .../tests/override-empty-arg-gradual/all-packages.nix | 3 +++ .../override-empty-arg-gradual/base/all-packages.nix | 3 +++ .../tests/override-empty-arg-gradual/base/default.nix | 1 + .../base/pkgs/by-name/no/nonDerivation/package.nix | 1 + .../tests/override-empty-arg-gradual/default.nix | 1 + .../tests/override-empty-arg-gradual/expected | 0 .../pkgs/by-name/no/nonDerivation/package.nix | 1 + 9 files changed, 22 insertions(+), 1 deletion(-) create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/all-packages.nix create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/base/all-packages.nix create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/base/default.nix create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/base/pkgs/by-name/no/nonDerivation/package.nix create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/default.nix create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/expected create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/pkgs/by-name/no/nonDerivation/package.nix diff --git a/pkgs/test/nixpkgs-check-by-name/README.md b/pkgs/test/nixpkgs-check-by-name/README.md index b098658fce4c..c906eaffc974 100644 --- a/pkgs/test/nixpkgs-check-by-name/README.md +++ b/pkgs/test/nixpkgs-check-by-name/README.md @@ -81,6 +81,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 will be used as the `--base` argument, allowing tests of gradual transitions. + - `expected` (optional): A file containing the expected standard output. The default is expecting an empty standard output. diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index 53c24845cb20..bf3bfb193f18 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs @@ -212,10 +212,17 @@ mod tests { fn test_nixpkgs(name: &str, path: &Path, expected_errors: &str) -> anyhow::Result<()> { let extra_nix_path = Path::new("tests/mock-nixpkgs.nix"); + let base_path = path.join("base"); + let base_nixpkgs = if base_path.exists() { + Some(base_path.as_path()) + } else { + None + }; + // 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(None, &path, &vec![&extra_nix_path], &mut writer) + process(base_nixpkgs, &path, &vec![&extra_nix_path], &mut writer) .context(format!("Failed test case {name}"))?; Ok(writer) })?; diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/all-packages.nix b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/all-packages.nix new file mode 100644 index 000000000000..d369dd7228dc --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/all-packages.nix @@ -0,0 +1,3 @@ +self: super: { + nonDerivation = self.callPackage ./pkgs/by-name/no/nonDerivation/package.nix { }; +} diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/base/all-packages.nix b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/base/all-packages.nix new file mode 100644 index 000000000000..d369dd7228dc --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/base/all-packages.nix @@ -0,0 +1,3 @@ +self: super: { + nonDerivation = self.callPackage ./pkgs/by-name/no/nonDerivation/package.nix { }; +} diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/base/default.nix b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/base/default.nix new file mode 100644 index 000000000000..2875ea6327ef --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/base/default.nix @@ -0,0 +1 @@ +import ../../mock-nixpkgs.nix { root = ./.; } diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/base/pkgs/by-name/no/nonDerivation/package.nix b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/base/pkgs/by-name/no/nonDerivation/package.nix new file mode 100644 index 000000000000..a1b92efbbadb --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/base/pkgs/by-name/no/nonDerivation/package.nix @@ -0,0 +1 @@ +{ someDrv }: someDrv diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/default.nix b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/default.nix new file mode 100644 index 000000000000..af25d1450122 --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/default.nix @@ -0,0 +1 @@ +import ../mock-nixpkgs.nix { root = ./.; } diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/expected new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/pkgs/by-name/no/nonDerivation/package.nix b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/pkgs/by-name/no/nonDerivation/package.nix new file mode 100644 index 000000000000..a1b92efbbadb --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/pkgs/by-name/no/nonDerivation/package.nix @@ -0,0 +1 @@ +{ someDrv }: someDrv From 53b43ce0e322eb4fc8daaad8a5a597155d42379a Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 15 Dec 2023 00:47:34 +0100 Subject: [PATCH 06/10] tests.nixpkgs-check-by-name: Fix and document behavior without --base Previously, not passing `--base` would enforce the most strict checks. While there's currently no actual violation of these stricter checks, this does not match the previous behavior. This won't matter once CI passes `--base`, the code handling the optionality can be removed then. --- pkgs/test/nixpkgs-check-by-name/README.md | 9 +++++++-- pkgs/test/nixpkgs-check-by-name/src/main.rs | 16 +++++++++------- pkgs/test/nixpkgs-check-by-name/src/version.rs | 17 +++++++++++++++-- .../tests/override-empty-arg/base/default.nix | 1 + .../base/pkgs/by-name/README.md | 1 + 5 files changed, 33 insertions(+), 11 deletions(-) create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/base/default.nix create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/base/pkgs/by-name/README.md diff --git a/pkgs/test/nixpkgs-check-by-name/README.md b/pkgs/test/nixpkgs-check-by-name/README.md index c906eaffc974..8ed23204deca 100644 --- a/pkgs/test/nixpkgs-check-by-name/README.md +++ b/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 ] ` - Arguments: - - ``: The path to the Nixpkgs to check against - - ``: The path to the Nixpkgs to check + - ``: 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. - Exit code: - `0`: If the [validation](#validity-checks) is successful - `1`: If the [validation](#validity-checks) is not successful diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index bf3bfb193f18..91e1992a52c9 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs @@ -75,14 +75,16 @@ pub fn process( 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)) - }) + 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)) } })?; diff --git a/pkgs/test/nixpkgs-check-by-name/src/version.rs b/pkgs/test/nixpkgs-check-by-name/src/version.rs index 7f83bdf3ff67..c5cee95e0d53 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/version.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/version.rs @@ -16,12 +16,25 @@ 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<()> { + 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)| { - 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) }), ) } diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/base/default.nix b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/base/default.nix new file mode 100644 index 000000000000..2875ea6327ef --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/base/default.nix @@ -0,0 +1 @@ +import ../../mock-nixpkgs.nix { root = ./.; } diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/base/pkgs/by-name/README.md b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/base/pkgs/by-name/README.md new file mode 100644 index 000000000000..b0d2b34e338a --- /dev/null +++ b/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) From 413dd9c03e37562c61cd89799e5eb8a88c7bb42a Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 15 Dec 2023 01:02:02 +0100 Subject: [PATCH 07/10] tests.nixpkgs-check-by-name: Minor improvements from review --- pkgs/test/nixpkgs-check-by-name/src/eval.rs | 8 ++--- pkgs/test/nixpkgs-check-by-name/src/main.rs | 32 ++++++++++--------- .../test/nixpkgs-check-by-name/src/version.rs | 2 ++ .../tests/no-by-name/expected | 1 + 4 files changed, 24 insertions(+), 19 deletions(-) create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/no-by-name/expected diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index 927e446b452f..20652d9ede26 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/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, - eval_accessible_paths: &Vec<&Path>, + eval_accessible_paths: &[&Path], ) -> 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. @@ -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(), diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index 91e1992a52c9..ee73ffbd0f8d 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/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,10 +64,10 @@ fn main() -> ExitCode { pub fn process( base_nixpkgs: Option<&Path>, main_nixpkgs: &Path, - eval_accessible_paths: &Vec<&Path>, + eval_accessible_paths: &[&Path], error_writer: &mut W, ) -> anyhow::Result { - 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, error_writer)?.result_map( @@ -99,11 +94,17 @@ pub fn process( } } -/// 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( nixpkgs_path: &Path, - eval_accessible_paths: &Vec<&Path>, + eval_accessible_paths: &[&Path], + error_writer: &mut W, ) -> validation::Result { Ok({ let nixpkgs_path = nixpkgs_path.canonicalize().context(format!( @@ -112,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| @@ -224,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) })?; diff --git a/pkgs/test/nixpkgs-check-by-name/src/version.rs b/pkgs/test/nixpkgs-check-by-name/src/version.rs index c5cee95e0d53..c82f537c504b 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/version.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/version.rs @@ -16,6 +16,8 @@ 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. pub fn compare(optional_from: Option, to: Self) -> Validation<()> { validation::sequence_( // We only loop over the current attributes, diff --git a/pkgs/test/nixpkgs-check-by-name/tests/no-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/no-by-name/expected new file mode 100644 index 000000000000..ddcb2df46e5f --- /dev/null +++ b/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. From 79618ff8cbfb69b6eb70dbc591b42cee2fed974e Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 15 Dec 2023 02:14:48 +0100 Subject: [PATCH 08/10] tests.nixpkgs-check-by-name: Improve docs, introduce "ratchet" term --- pkgs/test/nixpkgs-check-by-name/README.md | 30 +++++++++---- pkgs/test/nixpkgs-check-by-name/src/eval.rs | 19 ++++---- pkgs/test/nixpkgs-check-by-name/src/main.rs | 34 +++++++------- .../src/{version.rs => ratchet.rs} | 44 ++++++++++--------- 4 files changed, 73 insertions(+), 54 deletions(-) rename pkgs/test/nixpkgs-check-by-name/src/{version.rs => ratchet.rs} (61%) diff --git a/pkgs/test/nixpkgs-check-by-name/README.md b/pkgs/test/nixpkgs-check-by-name/README.md index 8ed23204deca..7e8d39104e48 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. + - ``: + 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, all violations of stricter checks are allowed. - However, this flag will become required once CI passes it. + 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 20652d9ede26..cd8c70472cf2 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 ee73ffbd0f8d..01f7d4b71982 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 c82f537c504b..c12f1ead2540 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 { From 74e8b38dbe809022d096b11b87ba33a68ba0d374 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 15 Dec 2023 02:23:05 +0100 Subject: [PATCH 09/10] tests.nixpkgs-check-by-name: Move interface description into code This would be duplicated otherwise --- pkgs/test/nixpkgs-check-by-name/README.md | 26 +++++---------------- pkgs/test/nixpkgs-check-by-name/src/main.rs | 14 ++++++++++- 2 files changed, 19 insertions(+), 21 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/README.md b/pkgs/test/nixpkgs-check-by-name/README.md index 7e8d39104e48..640e744546a7 100644 --- a/pkgs/test/nixpkgs-check-by-name/README.md +++ b/pkgs/test/nixpkgs-check-by-name/README.md @@ -4,28 +4,14 @@ 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. +The interface of the tool is shown with `--help`: +``` +cargo run -- --help +``` -- Command line: `nixpkgs-check-by-name [--base ] ` -- Arguments: - - ``: - 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 - - `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 diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index 01f7d4b71982..18c950d0a6eb 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs @@ -17,8 +17,20 @@ use std::path::{Path, PathBuf}; use std::process::ExitCode; /// Program to check the validity of pkgs/by-name +/// +/// This CLI interface may be changed over time if the CI workflow making use of +/// it is adjusted to deal with the change appropriately. +/// +/// Exit code: +/// - `0`: If the validation is successful +/// - `1`: If the validation is not successful +/// - `2`: If an unexpected I/O error occurs +/// +/// Standard error: +/// - Informative messages +/// - Detected problems if validation is not successful #[derive(Parser, Debug)] -#[command(about)] +#[command(about, verbatim_doc_comment)] pub struct Args { /// Path to the main Nixpkgs to check. /// For PRs, this should be set to a checkout of the PR branch. From fc2d26939d2839d65553e8ca605f2440a45fc387 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 15 Dec 2023 17:27:26 +0100 Subject: [PATCH 10/10] tests.nixpkgs-check-by-name: Improve check clarity --- pkgs/test/nixpkgs-check-by-name/README.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/README.md b/pkgs/test/nixpkgs-check-by-name/README.md index 640e744546a7..19865ca0952b 100644 --- a/pkgs/test/nixpkgs-check-by-name/README.md +++ b/pkgs/test/nixpkgs-check-by-name/README.md @@ -19,7 +19,7 @@ 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. @@ -28,8 +28,8 @@ 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`. -- `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 @@ -41,7 +41,8 @@ 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, +- 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