diff options
author | Silvan Mosberger <silvan.mosberger@tweag.io> | 2023-12-14 03:11:14 +0100 |
---|---|---|
committer | Silvan Mosberger <silvan.mosberger@tweag.io> | 2023-12-14 03:51:10 +0100 |
commit | d487a975ccb27302d1095ab56cd3c104712452c7 (patch) | |
tree | da37a6ba93ee2a019da4d321fd981a1852e9bf91 /pkgs/test | |
parent | a6ba4cae311698ea907ee239785f75d76bd01e4b (diff) |
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 <BASE_NIXPKGS>` 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.
Diffstat (limited to 'pkgs/test')
-rw-r--r-- | pkgs/test/nixpkgs-check-by-name/README.md | 13 | ||||
-rw-r--r-- | pkgs/test/nixpkgs-check-by-name/src/eval.rs | 2 | ||||
-rw-r--r-- | pkgs/test/nixpkgs-check-by-name/src/main.rs | 64 | ||||
-rw-r--r-- | pkgs/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 146cea0a64ba1..b098658fce4c2 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 <NIXPKGS>` +- Command line: `nixpkgs-check-by-name [--base <BASE_NIXPKGS>] <NIXPKGS>` - Arguments: + - `<BASE_NIXPKGS>`: The path to the Nixpkgs to check against - `<NIXPKGS>`: The path to the Nixpkgs to check - - `--version <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 face1117f6435..927e446b452f3 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<String>, - eval_accessible_paths: Vec<&Path>, + eval_accessible_paths: &Vec<&Path>, ) -> validation::Result<version::Nixpkgs> { // Write the list of packages we need to check into a temporary JSON file. // This can then get read by the Nix evaluation. diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index 981d1134c85a7..53c24845cb200 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<PathBuf>, } 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<W: io::Write>( - 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<bool> { - 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<W: io::Write>( } Ok(false) } - Success(_) => Ok(true), + Success(()) => Ok(true), } } @@ -102,7 +101,7 @@ pub fn process<W: io::Write>( /// 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<version::Nixpkgs> { 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 ab146270241ef..7f83bdf3ff675 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 { |