From 5b7ae79ef0dc40df8c6da2a31a90b8aad5cb88df Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Wed, 3 Jan 2024 17:56:45 +0100 Subject: [PATCH 1/8] tests.nixpkgs-check-by-name: Refactor eval code and improve comments Does a bunch of cleanups to the eval.{rs,nix} code to make future changes easier, no functionality is changed. --- pkgs/test/nixpkgs-check-by-name/src/eval.nix | 108 ++++++------ pkgs/test/nixpkgs-check-by-name/src/eval.rs | 163 +++++++++++-------- 2 files changed, 152 insertions(+), 119 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.nix b/pkgs/test/nixpkgs-check-by-name/src/eval.nix index bf9f19d8e460b..847fab1085d17 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.nix +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.nix @@ -1,11 +1,7 @@ # Takes a path to nixpkgs and a path to the json-encoded list of attributes to check. -# Returns an attribute set containing information on each requested attribute. -# If the attribute is missing from Nixpkgs it's also missing from the result. -# -# The returned information is an attribute set with: -# - call_package_path: The from ` = callPackage { ... }`, -# or null if it's not defined as with callPackage, or if the is not a path -# - is_derivation: The result of `lib.isDerivation ` +# Returns an value containing information on each requested attribute, +# which is decoded on the Rust side. +# See ./eval.rs for the meaning of the returned values { attrsPath, nixpkgsPath, @@ -13,70 +9,78 @@ let attrs = builtins.fromJSON (builtins.readFile attrsPath); - # This overlay mocks callPackage to persist the path of the first argument - callPackageOverlay = self: super: { + # We need access to the `callPackage` arguments of each attribute. + # The only way to do so is to override `callPackage` with our own version that adds this information to the result, + # and then try to access this information. + overlay = final: prev: { + + # Information for attributes defined using `callPackage` callPackage = fn: args: - let - result = super.callPackage fn args; - variantInfo._attributeVariant = { - # These names are used by the deserializer on the Rust side - CallPackage.path = + addVariantInfo (prev.callPackage fn args) { + Manual = { + path = if builtins.isPath fn then toString fn else null; - CallPackage.empty_arg = + empty_arg = args == { }; }; - in - if builtins.isAttrs result then - # If this was the last overlay to be applied, we could just only return the `_callPackagePath`, - # but that's not the case because stdenv has another overlays on top of user-provided ones. - # So to not break the stdenv build we need to return the mostly proper result here - result // variantInfo - else - # It's very rare that callPackage doesn't return an attribute set, but it can occur. - variantInfo; + }; + # Information for attributes that are auto-called from pkgs/by-name. + # This internal attribute is only used by pkgs/by-name _internalCallByNamePackageFile = file: - let - result = super._internalCallByNamePackageFile file; - variantInfo._attributeVariant = { - # This name is used by the deserializer on the Rust side - AutoCalled = null; - }; - in - if builtins.isAttrs result then - # If this was the last overlay to be applied, we could just only return the `_callPackagePath`, - # but that's not the case because stdenv has another overlays on top of user-provided ones. - # So to not break the stdenv build we need to return the mostly proper result here - result // variantInfo - else - # It's very rare that callPackage doesn't return an attribute set, but it can occur. - variantInfo; + addVariantInfo (prev._internalCallByNamePackageFile file) { + Auto = null; + }; + }; + # We can't just replace attribute values with their info in the overlay, + # because attributes can depend on other attributes, so this would break evaluation. + addVariantInfo = value: variant: + if builtins.isAttrs value then + value // { + _callPackageVariant = variant; + } + else + # It's very rare that callPackage doesn't return an attribute set, but it can occur. + # In such a case we can't really return anything sensible that would include the info, + # so just don't return the info and let the consumer handle it. + value; + pkgs = import nixpkgsPath { # Don't let the users home directory influence this result config = { }; - overlays = [ callPackageOverlay ]; + overlays = [ overlay ]; }; - attrInfo = attr: - let - value = pkgs.${attr}; - in - { - # These names are used by the deserializer on the Rust side - variant = value._attributeVariant or { Other = null; }; - is_derivation = pkgs.lib.isDerivation value; - }; + attrInfo = name: value: + if ! builtins.isAttrs value then + { + NonAttributeSet = null; + } + else if ! value ? _callPackageVariant then + { + NonCallPackage = null; + } + else + { + CallPackage = { + call_package_variant = value._callPackageVariant; + is_derivation = pkgs.lib.isDerivation value; + }; + }; attrInfos = builtins.listToAttrs (map (name: { inherit name; - value = attrInfo name; + value = + if ! pkgs ? ${name} then + { Missing = null; } + else + { Existing = attrInfo name pkgs.${name}; }; }) attrs); in -# Filter out attributes not in Nixpkgs -builtins.intersectAttrs pkgs attrInfos +attrInfos diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index cd8c70472cf25..4e8fba5493861 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs @@ -13,26 +13,42 @@ use tempfile::NamedTempFile; /// Attribute set of this structure is returned by eval.nix #[derive(Deserialize)] -struct AttributeInfo { - variant: AttributeVariant, +enum ByNameAttribute { + /// The attribute doesn't exist at all + Missing, + Existing(AttributeInfo), +} + +#[derive(Deserialize)] +enum AttributeInfo { + /// The attribute exists, but its value isn't an attribute set + NonAttributeSet, + /// The attribute exists, but its value isn't defined using callPackage + NonCallPackage, + /// The attribute exists and its value is an attribute set + CallPackage(CallPackageInfo), +} + +#[derive(Deserialize)] +struct CallPackageInfo { + call_package_variant: CallPackageVariant, + /// Whether the attribute is a derivation (`lib.isDerivation`) is_derivation: bool, } #[derive(Deserialize)] -enum AttributeVariant { +enum CallPackageVariant { /// The attribute is auto-called as pkgs.callPackage using pkgs/by-name, /// and it is not overridden by a definition in all-packages.nix - AutoCalled, + Auto, /// The attribute is defined as a pkgs.callPackage , /// and overridden by all-packages.nix - CallPackage { + Manual { /// The argument or None if it's not a path path: Option, /// true if is { } empty_arg: bool, }, - /// The attribute is not defined as pkgs.callPackage - Other, } /// Check that the Nixpkgs attribute values corresponding to the packages in pkgs/by-name are @@ -103,74 +119,87 @@ pub fn check_values( anyhow::bail!("Failed to run command {command:?}"); } // Parse the resulting JSON value - let actual_files: HashMap = serde_json::from_slice(&result.stdout) + let attributes: HashMap = serde_json::from_slice(&result.stdout) .context(format!( "Failed to deserialise {}", String::from_utf8_lossy(&result.stdout) ))?; - Ok( - validation::sequence(package_names.into_iter().map(|package_name| { - let relative_package_file = structure::relative_file_for_package(&package_name); + let check_result = validation::sequence(attributes.into_iter().map( + |(attribute_name, attribute_value)| { + let relative_package_file = structure::relative_file_for_package(&attribute_name); let absolute_package_file = nixpkgs_path.join(&relative_package_file); - if let Some(attribute_info) = actual_files.get(&package_name) { - let check_result = if !attribute_info.is_derivation { - NixpkgsProblem::NonDerivation { - relative_package_file: relative_package_file.clone(), - package_name: package_name.clone(), - } - .into() - } else { - Success(()) - }; - - let check_result = check_result.and(match &attribute_info.variant { - AttributeVariant::AutoCalled => Success(ratchet::Package { - empty_non_auto_called: ratchet::EmptyNonAutoCalled::Valid, - }), - AttributeVariant::CallPackage { path, empty_arg } => { - let correct_file = if let Some(call_package_path) = path { - absolute_package_file == *call_package_path - } else { - false - }; - - if correct_file { - Success(ratchet::Package { - // Empty arguments for non-auto-called packages are not allowed anymore. - empty_non_auto_called: if *empty_arg { - ratchet::EmptyNonAutoCalled::Invalid - } else { - ratchet::EmptyNonAutoCalled::Valid - }, - }) - } else { - NixpkgsProblem::WrongCallPackage { - relative_package_file: relative_package_file.clone(), - package_name: package_name.clone(), + use AttributeInfo::*; + use ByNameAttribute::*; + use CallPackageVariant::*; + + let check_result = match attribute_value { + Missing => NixpkgsProblem::UndefinedAttr { + relative_package_file: relative_package_file.clone(), + package_name: attribute_name.clone(), + } + .into(), + Existing(NonAttributeSet) => NixpkgsProblem::NonDerivation { + relative_package_file: relative_package_file.clone(), + package_name: attribute_name.clone(), + } + .into(), + Existing(NonCallPackage) => NixpkgsProblem::WrongCallPackage { + relative_package_file: relative_package_file.clone(), + package_name: attribute_name.clone(), + } + .into(), + Existing(CallPackage(CallPackageInfo { + is_derivation, + call_package_variant, + })) => { + let check_result = if !is_derivation { + NixpkgsProblem::NonDerivation { + relative_package_file: relative_package_file.clone(), + package_name: attribute_name.clone(), + } + .into() + } else { + Success(()) + }; + + check_result.and(match &call_package_variant { + Auto => Success(ratchet::Package { + empty_non_auto_called: ratchet::EmptyNonAutoCalled::Valid, + }), + Manual { path, empty_arg } => { + let correct_file = if let Some(call_package_path) = path { + absolute_package_file == *call_package_path + } else { + false + }; + + if correct_file { + Success(ratchet::Package { + // Empty arguments for non-auto-called packages are not allowed anymore. + empty_non_auto_called: if *empty_arg { + ratchet::EmptyNonAutoCalled::Invalid + } else { + ratchet::EmptyNonAutoCalled::Valid + }, + }) + } else { + NixpkgsProblem::WrongCallPackage { + relative_package_file: relative_package_file.clone(), + package_name: attribute_name.clone(), + } + .into() } - .into() } - } - AttributeVariant::Other => NixpkgsProblem::WrongCallPackage { - relative_package_file: relative_package_file.clone(), - package_name: package_name.clone(), - } - .into(), - }); - - check_result.map(|value| (package_name.clone(), value)) - } else { - NixpkgsProblem::UndefinedAttr { - relative_package_file: relative_package_file.clone(), - package_name: package_name.clone(), + }) } - .into() - } - })) - .map(|elems| ratchet::Nixpkgs { - packages: elems.into_iter().collect(), - }), - ) + }; + check_result.map(|value| (attribute_name.clone(), value)) + }, + )); + + Ok(check_result.map(|elems| ratchet::Nixpkgs { + packages: elems.into_iter().collect(), + })) } From 66f29590c080536efcc41308646d85bec8411765 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Wed, 3 Jan 2024 19:29:29 +0100 Subject: [PATCH 2/8] tests.nixpkgs-check-by-name: Set evaluation system to x86_64-linux This was previously a checking impurity that could produce different results when run on different systems. --- pkgs/test/nixpkgs-check-by-name/README.md | 2 ++ pkgs/test/nixpkgs-check-by-name/src/eval.nix | 3 +++ pkgs/test/nixpkgs-check-by-name/tests/mock-nixpkgs.nix | 2 ++ 3 files changed, 7 insertions(+) diff --git a/pkgs/test/nixpkgs-check-by-name/README.md b/pkgs/test/nixpkgs-check-by-name/README.md index 19865ca0952b5..871847bd74cca 100644 --- a/pkgs/test/nixpkgs-check-by-name/README.md +++ b/pkgs/test/nixpkgs-check-by-name/README.md @@ -28,6 +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 + +Evaluate Nixpkgs with `system` set to `x86_64-linux` and check that: - 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`. diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.nix b/pkgs/test/nixpkgs-check-by-name/src/eval.nix index 847fab1085d17..e75a28ddbdf71 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.nix +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.nix @@ -54,6 +54,9 @@ let # Don't let the users home directory influence this result config = { }; overlays = [ overlay ]; + # We check evaluation and callPackage only for x86_64-linux. + # Not ideal, but hard to fix + system = "x86_64-linux"; }; attrInfo = name: value: diff --git a/pkgs/test/nixpkgs-check-by-name/tests/mock-nixpkgs.nix b/pkgs/test/nixpkgs-check-by-name/tests/mock-nixpkgs.nix index 01bb27a480388..cb8066062cc64 100644 --- a/pkgs/test/nixpkgs-check-by-name/tests/mock-nixpkgs.nix +++ b/pkgs/test/nixpkgs-check-by-name/tests/mock-nixpkgs.nix @@ -19,6 +19,8 @@ It returns a Nixpkgs-like function that can be auto-called and evaluates to an a overlays ? [], # Passed by the checker to make sure a real Nixpkgs isn't influenced by impurities config ? {}, + # Passed by the checker to make sure a real Nixpkgs isn't influenced by impurities + system ? null, }: let From ba6faf428f575f88b2caa4ce67413a7bcde26e34 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Wed, 3 Jan 2024 21:54:21 +0100 Subject: [PATCH 3/8] tests.nixpkgs-check-by-name: Make --base required CI now passes the flag, so it doesn't have to be optional anymore --- pkgs/test/nixpkgs-check-by-name/src/main.rs | 32 ++++++++----------- .../test/nixpkgs-check-by-name/src/ratchet.rs | 21 +++--------- .../tests/empty-base/default.nix | 1 + .../tests/empty-base/pkgs/by-name/README.md | 0 4 files changed, 18 insertions(+), 36 deletions(-) create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/empty-base/default.nix create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/empty-base/pkgs/by-name/README.md diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index 18c950d0a6eb0..6e78f6f5ae2aa 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs @@ -38,15 +38,13 @@ pub struct Args { /// 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, + base: PathBuf, } fn main() -> ExitCode { let args = Args::parse(); - match process(args.base.as_deref(), &args.nixpkgs, &[], &mut io::stderr()) { + match process(&args.base, &args.nixpkgs, &[], &mut io::stderr()) { Ok(true) => { eprintln!("{}", "Validated successfully".green()); ExitCode::SUCCESS @@ -77,7 +75,7 @@ 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( - base_nixpkgs: Option<&Path>, + base_nixpkgs: &Path, main_nixpkgs: &Path, eval_accessible_paths: &[&Path], error_writer: &mut W, @@ -87,18 +85,14 @@ pub fn process( 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(ratchet::Nixpkgs::compare( - Some(base_nixpkgs_version), - nixpkgs_version, - )) - }, - ) - } else { - Ok(ratchet::Nixpkgs::compare(None, nixpkgs_version)) - } + check_nixpkgs(base_nixpkgs, eval_accessible_paths, error_writer)?.result_map( + |base_nixpkgs_version| { + Ok(ratchet::Nixpkgs::compare( + base_nixpkgs_version, + nixpkgs_version, + )) + }, + ) })?; match check_result { @@ -234,9 +228,9 @@ mod tests { let base_path = path.join("base"); let base_nixpkgs = if base_path.exists() { - Some(base_path.as_path()) + base_path.as_path() } else { - None + Path::new("tests/empty-base") }; // We don't want coloring to mess up the tests diff --git a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs index c12f1ead25402..3123f7e0e5329 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs @@ -16,26 +16,13 @@ pub struct Nixpkgs { impl Nixpkgs { /// Validates the ratchet checks for Nixpkgs - pub fn compare(optional_from: Option, 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.packages.into_iter().map(|(name, attr_to)| { - let attr_from = if let Some(from) = &optional_from { - 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(&Package { - empty_non_auto_called: EmptyNonAutoCalled::Invalid, - }) - }; - Package::compare(&name, attr_from, &attr_to) - }), + to.packages + .into_iter() + .map(|(name, attr_to)| Package::compare(&name, from.packages.get(&name), &attr_to)), ) } } diff --git a/pkgs/test/nixpkgs-check-by-name/tests/empty-base/default.nix b/pkgs/test/nixpkgs-check-by-name/tests/empty-base/default.nix new file mode 100644 index 0000000000000..af25d1450122b --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/empty-base/default.nix @@ -0,0 +1 @@ +import ../mock-nixpkgs.nix { root = ./.; } diff --git a/pkgs/test/nixpkgs-check-by-name/tests/empty-base/pkgs/by-name/README.md b/pkgs/test/nixpkgs-check-by-name/tests/empty-base/pkgs/by-name/README.md new file mode 100644 index 0000000000000..e69de29bb2d1d From 2a8f4693486c4b4ba84586ed5b2312e82b77e3ef Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Thu, 4 Jan 2024 02:03:10 +0100 Subject: [PATCH 4/8] tests.nixpkgs-check-by-name: Re-usable ratchet logic This makes the attribute ratchet check logic more re-usable, which will be used in a future commit. It also renames the ratchet states to something more intuitive --- pkgs/test/nixpkgs-check-by-name/src/eval.rs | 7 +- .../test/nixpkgs-check-by-name/src/ratchet.rs | 70 ++++++++++++++----- 2 files changed, 55 insertions(+), 22 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index 4e8fba5493861..00bdc0bc9f394 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs @@ -130,6 +130,7 @@ pub fn check_values( let relative_package_file = structure::relative_file_for_package(&attribute_name); let absolute_package_file = nixpkgs_path.join(&relative_package_file); + use ratchet::RatchetState::*; use AttributeInfo::*; use ByNameAttribute::*; use CallPackageVariant::*; @@ -166,7 +167,7 @@ pub fn check_values( check_result.and(match &call_package_variant { Auto => Success(ratchet::Package { - empty_non_auto_called: ratchet::EmptyNonAutoCalled::Valid, + empty_non_auto_called: Tight, }), Manual { path, empty_arg } => { let correct_file = if let Some(call_package_path) = path { @@ -179,9 +180,9 @@ pub fn check_values( Success(ratchet::Package { // Empty arguments for non-auto-called packages are not allowed anymore. empty_non_auto_called: if *empty_arg { - ratchet::EmptyNonAutoCalled::Invalid + Loose(ratchet::EmptyNonAutoCalled) } else { - ratchet::EmptyNonAutoCalled::Valid + Tight }, }) } else { diff --git a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs index 3123f7e0e5329..c363f85466e92 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs @@ -30,7 +30,7 @@ impl Nixpkgs { /// 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, + pub empty_non_auto_called: RatchetState, } impl Package { @@ -44,29 +44,61 @@ impl Package { } } -/// The ratchet value of a single package in `pkgs/by-name` +/// The ratchet state of a generic ratchet check. +pub enum RatchetState { + /// The ratchet is loose, it can be tightened more. + /// In other words, this is the legacy state we're trying to move away from. + /// Introducing new instances is now allowed but previous instances will continue to be allowed. + /// The `Context` is context for error messages in case a new instance of this state is + /// introduced + Loose(Context), + /// The ratchet is tight, it can't be tightened any further. + /// This is either because we already use the latest state, or because the ratchet isn't + /// relevant. + Tight, +} + +/// A trait for a specific ratchet check for an attribute. +trait AttributeRatchet: Sized { + /// What error to produce in case the ratchet went in the wrong direction + fn to_error(name: &str, context: &Self, existed_before: bool) -> NixpkgsProblem; + + /// Compare the previous ratchet state of an attribute to the new state. + /// The previous state may be `None` in case the attribute is new. + fn compare( + name: &str, + optional_from: Option<&RatchetState>, + to: &RatchetState, + ) -> Validation<()> { + // If we don't have a previous state, enforce a tight ratchet + let from = optional_from.unwrap_or(&RatchetState::Tight); + match (from, to) { + // Always okay to keep it tight or tighten the ratchet + (_, RatchetState::Tight) => Success(()), + + // Grandfathering policy for a loose ratchet + (RatchetState::Loose { .. }, RatchetState::Loose { .. }) => Success(()), + + // Loosening a ratchet is now allowed + (RatchetState::Tight, RatchetState::Loose(context)) => { + Self::to_error(name, context, optional_from.is_some()).into() + } + } + } +} + +/// The ratchet value of an attribute /// 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 { - Invalid, - Valid, -} +pub struct EmptyNonAutoCalled; -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 { - Success(()) - } else { - NixpkgsProblem::WrongCallPackage { - relative_package_file: structure::relative_file_for_package(name), - package_name: name.to_owned(), - } - .into() +impl AttributeRatchet for EmptyNonAutoCalled { + fn to_error(name: &str, _context: &Self, _existed_before: bool) -> NixpkgsProblem { + NixpkgsProblem::WrongCallPackage { + relative_package_file: structure::relative_file_for_package(name), + package_name: name.to_owned(), } } } From 27c873af995394445eef05eaa1b95997f57e868a Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 5 Jan 2024 00:50:41 +0100 Subject: [PATCH 5/8] tests.nixpkgs-check-by-name: Deterministic ordering Makes errors for attributes deterministic so it's easier to test (also, reproducibility is always nice) --- pkgs/test/nixpkgs-check-by-name/src/eval.nix | 11 ++++++----- pkgs/test/nixpkgs-check-by-name/src/eval.rs | 6 +++--- pkgs/test/nixpkgs-check-by-name/src/ratchet.rs | 12 +++++++----- 3 files changed, 16 insertions(+), 13 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.nix b/pkgs/test/nixpkgs-check-by-name/src/eval.nix index e75a28ddbdf71..b605a35a322a7 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.nix +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.nix @@ -76,14 +76,15 @@ let }; }; - attrInfos = builtins.listToAttrs (map (name: { - inherit name; - value = + attrInfos = map (name: [ + name + ( if ! pkgs ? ${name} then { Missing = null; } else - { Existing = attrInfo name pkgs.${name}; }; - }) attrs); + { Existing = attrInfo name pkgs.${name}; } + ) + ]) attrs; in attrInfos diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index 00bdc0bc9f394..4004841c94304 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs @@ -6,7 +6,6 @@ use std::path::Path; use anyhow::Context; use serde::Deserialize; -use std::collections::HashMap; use std::path::PathBuf; use std::process; use tempfile::NamedTempFile; @@ -119,7 +118,7 @@ pub fn check_values( anyhow::bail!("Failed to run command {command:?}"); } // Parse the resulting JSON value - let attributes: HashMap = serde_json::from_slice(&result.stdout) + let attributes: Vec<(String, ByNameAttribute)> = serde_json::from_slice(&result.stdout) .context(format!( "Failed to deserialise {}", String::from_utf8_lossy(&result.stdout) @@ -201,6 +200,7 @@ pub fn check_values( )); Ok(check_result.map(|elems| ratchet::Nixpkgs { - packages: elems.into_iter().collect(), + package_names, + package_map: elems.into_iter().collect(), })) } diff --git a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs index c363f85466e92..ae0c29cb6f35a 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs @@ -10,8 +10,10 @@ use std::collections::HashMap; /// The ratchet value for the entirety of Nixpkgs. #[derive(Default)] pub struct Nixpkgs { - /// The ratchet values for each package in `pkgs/by-name` - pub packages: HashMap, + /// Sorted list of attributes in package_map + pub package_names: Vec, + /// The ratchet values for all packages + pub package_map: HashMap, } impl Nixpkgs { @@ -20,9 +22,9 @@ impl Nixpkgs { validation::sequence_( // We only loop over the current attributes, // we don't need to check ones that were removed - to.packages - .into_iter() - .map(|(name, attr_to)| Package::compare(&name, from.packages.get(&name), &attr_to)), + to.package_names.into_iter().map(|name| { + Package::compare(&name, from.package_map.get(&name), &to.package_map[&name]) + }), ) } } From 54b05324f41d3d45eccddc3feb4bbb9e0c060a39 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 5 Jan 2024 00:48:02 +0100 Subject: [PATCH 6/8] tests.nixpkgs-check-by-name: Internal strip nixpkgs prefix Strips the Nixpkgs prefix from the callPackage paths, makes future error messages using this path be deterministic. --- pkgs/test/nixpkgs-check-by-name/src/eval.nix | 5 ++++- pkgs/test/nixpkgs-check-by-name/src/eval.rs | 3 +-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.nix b/pkgs/test/nixpkgs-check-by-name/src/eval.nix index b605a35a322a7..7707dc732b703 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.nix +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.nix @@ -9,6 +9,9 @@ let attrs = builtins.fromJSON (builtins.readFile attrsPath); + nixpkgsPathLength = builtins.stringLength (toString nixpkgsPath) + 1; + removeNixpkgsPrefix = builtins.substring nixpkgsPathLength (-1); + # We need access to the `callPackage` arguments of each attribute. # The only way to do so is to override `callPackage` with our own version that adds this information to the result, # and then try to access this information. @@ -20,7 +23,7 @@ let Manual = { path = if builtins.isPath fn then - toString fn + removeNixpkgsPrefix (toString fn) else null; empty_arg = diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index 4004841c94304..7680af185bdbd 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs @@ -127,7 +127,6 @@ pub fn check_values( let check_result = validation::sequence(attributes.into_iter().map( |(attribute_name, attribute_value)| { let relative_package_file = structure::relative_file_for_package(&attribute_name); - let absolute_package_file = nixpkgs_path.join(&relative_package_file); use ratchet::RatchetState::*; use AttributeInfo::*; @@ -170,7 +169,7 @@ pub fn check_values( }), Manual { path, empty_arg } => { let correct_file = if let Some(call_package_path) = path { - absolute_package_file == *call_package_path + relative_package_file == *call_package_path } else { false }; From 4cd2e64db339bfb1d07214b7fb83d5abbc29a4f6 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Tue, 9 Jan 2024 19:35:11 +0100 Subject: [PATCH 7/8] tests.nixpkgs-check-by-name: Minor improvements from feedback - Typo - Rename AttributeRatchet to ToNixpkgsProblem - Make the compare trait method into a RatchetState method Co-Authored-By: Philip Taron --- .../test/nixpkgs-check-by-name/src/ratchet.rs | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs index ae0c29cb6f35a..85feb0eee62f3 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs @@ -38,7 +38,7 @@ pub struct Package { 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( + RatchetState::::compare( name, optional_from.map(|x| &x.empty_non_auto_called), &to.empty_non_auto_called, @@ -50,7 +50,7 @@ impl Package { pub enum RatchetState { /// The ratchet is loose, it can be tightened more. /// In other words, this is the legacy state we're trying to move away from. - /// Introducing new instances is now allowed but previous instances will continue to be allowed. + /// Introducing new instances is not allowed but previous instances will continue to be allowed. /// The `Context` is context for error messages in case a new instance of this state is /// introduced Loose(Context), @@ -60,18 +60,16 @@ pub enum RatchetState { Tight, } -/// A trait for a specific ratchet check for an attribute. -trait AttributeRatchet: Sized { - /// What error to produce in case the ratchet went in the wrong direction - fn to_error(name: &str, context: &Self, existed_before: bool) -> NixpkgsProblem; +/// A trait that can convert an attribute-specific error context into a NixpkgsProblem +pub trait ToNixpkgsProblem { + /// How to convert an attribute-specific error context into a NixpkgsProblem + fn to_nixpkgs_problem(name: &str, context: &Self, existed_before: bool) -> NixpkgsProblem; +} +impl RatchetState { /// Compare the previous ratchet state of an attribute to the new state. /// The previous state may be `None` in case the attribute is new. - fn compare( - name: &str, - optional_from: Option<&RatchetState>, - to: &RatchetState, - ) -> Validation<()> { + fn compare(name: &str, optional_from: Option<&Self>, to: &Self) -> Validation<()> { // If we don't have a previous state, enforce a tight ratchet let from = optional_from.unwrap_or(&RatchetState::Tight); match (from, to) { @@ -83,7 +81,7 @@ trait AttributeRatchet: Sized { // Loosening a ratchet is now allowed (RatchetState::Tight, RatchetState::Loose(context)) => { - Self::to_error(name, context, optional_from.is_some()).into() + Context::to_nixpkgs_problem(name, context, optional_from.is_some()).into() } } } @@ -96,8 +94,8 @@ trait AttributeRatchet: Sized { /// with an empty second argument like `callPackage ... { }`. pub struct EmptyNonAutoCalled; -impl AttributeRatchet for EmptyNonAutoCalled { - fn to_error(name: &str, _context: &Self, _existed_before: bool) -> NixpkgsProblem { +impl ToNixpkgsProblem for EmptyNonAutoCalled { + fn to_nixpkgs_problem(name: &str, _context: &Self, _existed_before: bool) -> NixpkgsProblem { NixpkgsProblem::WrongCallPackage { relative_package_file: structure::relative_file_for_package(name), package_name: name.to_owned(), From a1db0cdf9bb5e90727eb1c455da448fb21bab91e Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Tue, 9 Jan 2024 19:39:50 +0100 Subject: [PATCH 8/8] tests.nixpkgs-check-by-name: .context -> .with_context Avoids allocation in the non-error case --- pkgs/test/nixpkgs-check-by-name/src/eval.rs | 26 ++++++++++------- pkgs/test/nixpkgs-check-by-name/src/main.rs | 12 ++++---- .../nixpkgs-check-by-name/src/references.rs | 29 ++++++++++--------- pkgs/test/nixpkgs-check-by-name/src/utils.rs | 4 +-- 4 files changed, 40 insertions(+), 31 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index 7680af185bdbd..65f71ccafc6f4 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs @@ -60,20 +60,22 @@ pub fn check_values( ) -> 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")?; + let attrs_file = NamedTempFile::new().with_context(|| "Failed to create a temporary file")?; // We need to canonicalise this path because if it's a symlink (which can be the case on // Darwin), Nix would need to read both the symlink and the target path, therefore need 2 // NIX_PATH entries for restrict-eval. But if we resolve the symlinks then only one predictable // entry is needed. let attrs_file_path = attrs_file.path().canonicalize()?; - serde_json::to_writer(&attrs_file, &package_names).context(format!( - "Failed to serialise the package names to the temporary path {}", - attrs_file_path.display() - ))?; + serde_json::to_writer(&attrs_file, &package_names).with_context(|| { + format!( + "Failed to serialise the package names to the temporary path {}", + attrs_file_path.display() + ) + })?; let expr_path = std::env::var("NIX_CHECK_BY_NAME_EXPR_PATH") - .context("Could not get environment variable NIX_CHECK_BY_NAME_EXPR_PATH")?; + .with_context(|| "Could not get environment variable NIX_CHECK_BY_NAME_EXPR_PATH")?; // With restrict-eval, only paths in NIX_PATH can be accessed, so we explicitly specify the // ones needed needed let mut command = process::Command::new("nix-instantiate"); @@ -112,17 +114,19 @@ pub fn check_values( let result = command .output() - .context(format!("Failed to run command {command:?}"))?; + .with_context(|| format!("Failed to run command {command:?}"))?; if !result.status.success() { anyhow::bail!("Failed to run command {command:?}"); } // Parse the resulting JSON value let attributes: Vec<(String, ByNameAttribute)> = serde_json::from_slice(&result.stdout) - .context(format!( - "Failed to deserialise {}", - String::from_utf8_lossy(&result.stdout) - ))?; + .with_context(|| { + format!( + "Failed to deserialise {}", + String::from_utf8_lossy(&result.stdout) + ) + })?; let check_result = validation::sequence(attributes.into_iter().map( |(attribute_name, attribute_value)| { diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index 6e78f6f5ae2aa..d7627acb5fee6 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs @@ -117,10 +117,12 @@ pub fn check_nixpkgs( error_writer: &mut W, ) -> validation::Result { Ok({ - let nixpkgs_path = nixpkgs_path.canonicalize().context(format!( - "Nixpkgs path {} could not be resolved", - nixpkgs_path.display() - ))?; + let nixpkgs_path = nixpkgs_path.canonicalize().with_context(|| { + format!( + "Nixpkgs path {} could not be resolved", + nixpkgs_path.display() + ) + })?; if !nixpkgs_path.join(utils::BASE_SUBPATH).exists() { writeln!( @@ -237,7 +239,7 @@ mod tests { let writer = temp_env::with_var("NO_COLOR", Some("1"), || -> anyhow::Result<_> { let mut writer = vec![]; process(base_nixpkgs, &path, &[&extra_nix_path], &mut writer) - .context(format!("Failed test case {name}"))?; + .with_context(|| format!("Failed test case {name}"))?; Ok(writer) })?; diff --git a/pkgs/test/nixpkgs-check-by-name/src/references.rs b/pkgs/test/nixpkgs-check-by-name/src/references.rs index 0561a9b22e858..3b3b05419780a 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/references.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/references.rs @@ -17,10 +17,12 @@ pub fn check_references( ) -> validation::Result<()> { // The empty argument here is the subpath under the package directory to check // An empty one means the package directory itself - check_path(relative_package_dir, absolute_package_dir, Path::new("")).context(format!( - "While checking the references in package directory {}", - relative_package_dir.display() - )) + check_path(relative_package_dir, absolute_package_dir, Path::new("")).with_context(|| { + format!( + "While checking the references in package directory {}", + relative_package_dir.display() + ) + }) } /// Checks for a specific path to not have references outside @@ -62,7 +64,9 @@ fn check_path( .map(|entry| { let entry_subpath = subpath.join(entry.file_name()); check_path(relative_package_dir, absolute_package_dir, &entry_subpath) - .context(format!("Error while recursing into {}", subpath.display())) + .with_context(|| { + format!("Error while recursing into {}", subpath.display()) + }) }) .collect_vec()?, ) @@ -70,8 +74,8 @@ fn check_path( // Only check Nix files if let Some(ext) = path.extension() { if ext == OsStr::new("nix") { - check_nix_file(relative_package_dir, absolute_package_dir, subpath).context( - format!("Error while checking Nix file {}", subpath.display()), + check_nix_file(relative_package_dir, absolute_package_dir, subpath).with_context( + || format!("Error while checking Nix file {}", subpath.display()), )? } else { Success(()) @@ -93,13 +97,12 @@ fn check_nix_file( subpath: &Path, ) -> validation::Result<()> { let path = absolute_package_dir.join(subpath); - let parent_dir = path.parent().context(format!( - "Could not get parent of path {}", - subpath.display() - ))?; + let parent_dir = path + .parent() + .with_context(|| format!("Could not get parent of path {}", subpath.display()))?; - let contents = - read_to_string(&path).context(format!("Could not read file {}", subpath.display()))?; + let contents = read_to_string(&path) + .with_context(|| format!("Could not read file {}", subpath.display()))?; let root = Root::parse(&contents); if let Some(error) = root.errors().first() { diff --git a/pkgs/test/nixpkgs-check-by-name/src/utils.rs b/pkgs/test/nixpkgs-check-by-name/src/utils.rs index 5cc4a0863ba82..7e0198dede424 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/utils.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/utils.rs @@ -10,10 +10,10 @@ pub const PACKAGE_NIX_FILENAME: &str = "package.nix"; pub fn read_dir_sorted(base_dir: &Path) -> anyhow::Result> { let listing = base_dir .read_dir() - .context(format!("Could not list directory {}", base_dir.display()))?; + .with_context(|| format!("Could not list directory {}", base_dir.display()))?; let mut shard_entries = listing .collect::>>() - .context(format!("Could not list directory {}", base_dir.display()))?; + .with_context(|| format!("Could not list directory {}", base_dir.display()))?; shard_entries.sort_by_key(|entry| entry.file_name()); Ok(shard_entries) }