diff options
author | Silvan Mosberger <silvan.mosberger@tweag.io> | 2024-01-22 22:47:23 +0100 |
---|---|---|
committer | Silvan Mosberger <silvan.mosberger@tweag.io> | 2024-01-22 22:56:21 +0100 |
commit | a48038d3091ede61d2d58c17450fba47b3499b40 (patch) | |
tree | cfa5f8e7eb1b20997a8418d2875e514619c18d4f | |
parent | 33b06c3696b6df2c45ba43434d8c44f69cfdda2d (diff) |
tests.nixpkgs-check-by-name: Minor code improvements
- Renames EmptyNonAutoCalled to ManualDefinition and add some docs to better explain what it's for - Don't conflate the Ratchet type with the Context type, keep them apart, making the code a bit cleaner, but also allows adding additional context for a Tight ratchet in the future - Change the signature of to_nixpkgs_problem to align with the other ratchet function signatures
-rw-r--r-- | pkgs/test/nixpkgs-check-by-name/src/eval.rs | 14 | ||||
-rw-r--r-- | pkgs/test/nixpkgs-check-by-name/src/ratchet.rs | 95 |
2 files changed, 68 insertions, 41 deletions
diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index e4584f09d8cd4..c3be2c5917d62 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs @@ -191,13 +191,13 @@ pub fn check_values( CallPackage(CallPackageInfo { is_derivation: true, call_package_variant: Manual { path, empty_arg }, - }) => Success(Loose(ratchet::UsesByName { + }) => Success(Loose(ratchet::CouldUseByName { call_package_path: path, empty_arg, })), }; uses_by_name.map(|x| ratchet::Package { - empty_non_auto_called: Tight, + manual_definition: Tight, uses_by_name: x, }) } @@ -213,7 +213,7 @@ pub fn check_values( // We choose the latter, since we want to move towards pkgs/by-name, not away // from it Success(ratchet::Package { - empty_non_auto_called: Tight, + manual_definition: Tight, uses_by_name: Tight, }) } @@ -248,7 +248,7 @@ pub fn check_values( check_result.and(match &call_package_variant { Auto => Success(ratchet::Package { - empty_non_auto_called: Tight, + manual_definition: Tight, uses_by_name: Tight, }), Manual { path, empty_arg } => { @@ -261,11 +261,7 @@ pub fn check_values( if correct_file { Success(ratchet::Package { // Empty arguments for non-auto-called packages are not allowed anymore. - empty_non_auto_called: if *empty_arg { - Loose(ratchet::EmptyNonAutoCalled) - } else { - Tight - }, + manual_definition: if *empty_arg { Loose(()) } else { Tight }, uses_by_name: 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 f8c129626cc00..dabc8f67316a7 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs @@ -33,7 +33,7 @@ impl Nixpkgs { /// The ratchet value for a top-level package pub struct Package { /// The ratchet value for the check for non-auto-called empty arguments - pub empty_non_auto_called: RatchetState<EmptyNonAutoCalled>, + pub manual_definition: RatchetState<ManualDefinition>, /// The ratchet value for the check for new packages using pkgs/by-name pub uses_by_name: RatchetState<UsesByName>, @@ -43,10 +43,10 @@ impl Package { /// Validates the ratchet checks for a top-level package pub fn compare(name: &str, optional_from: Option<&Self>, to: &Self) -> Validation<()> { validation::sequence_([ - RatchetState::<EmptyNonAutoCalled>::compare( + RatchetState::<ManualDefinition>::compare( name, - optional_from.map(|x| &x.empty_non_auto_called), - &to.empty_non_auto_called, + optional_from.map(|x| &x.manual_definition), + &to.manual_definition, ), RatchetState::<UsesByName>::compare( name, @@ -58,13 +58,13 @@ impl Package { } /// The ratchet state of a generic ratchet check. -pub enum RatchetState<Context> { +pub enum RatchetState<Ratchet: ToNixpkgsProblem> { /// 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 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), + Loose(Ratchet::ToContext), /// 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. @@ -73,40 +73,63 @@ pub enum RatchetState<Context> { /// A trait that can convert an attribute-specific error context into a NixpkgsProblem pub trait ToNixpkgsProblem { + /// Context relating to the Nixpkgs that is being transitioned _to_ + type ToContext; + /// How to convert an attribute-specific error context into a NixpkgsProblem - fn to_nixpkgs_problem(name: &str, context: &Self, existed_before: bool) -> NixpkgsProblem; + fn to_nixpkgs_problem( + name: &str, + optional_from: Option<()>, + to: &Self::ToContext, + ) -> NixpkgsProblem; } impl<Context: ToNixpkgsProblem> RatchetState<Context> { /// 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<&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) { - // Always okay to keep it tight or tighten the ratchet - (_, RatchetState::Tight) => Success(()), - - // Grandfathering policy for a loose ratchet - (RatchetState::Loose { .. }, RatchetState::Loose { .. }) => Success(()), - + match (optional_from, to) { // Loosening a ratchet is now allowed - (RatchetState::Tight, RatchetState::Loose(context)) => { - Context::to_nixpkgs_problem(name, context, optional_from.is_some()).into() + (Some(RatchetState::Tight), RatchetState::Loose(loose_context)) => { + Context::to_nixpkgs_problem(name, Some(()), loose_context).into() } + + // Introducing a loose ratchet is also not allowed + (None, RatchetState::Loose(loose_context)) => { + Context::to_nixpkgs_problem(name, None, loose_context).into() + } + + // Everything else is allowed, including: + // - Loose -> Loose (grandfathering policy for a loose ratchet) + // - -> Tight (always okay to keep or make the ratchet tight) + _ => Success(()), } } } -/// The ratchet value of an attribute -/// for the non-auto-called empty argument check of a single. +/// The ratchet to check whether a top-level attribute has/needs +/// a manual definition, e.g. in all-packages.nix. +/// +/// This ratchet is only tight for attributes that: +/// - Are not defined in `pkgs/by-name`, and rely on a manual definition +/// - Are defined in `pkgs/by-name` without any manual definition, +/// (no custom argument overrides) +/// - Are defined with `pkgs/by-name` with a manual definition that can't be removed +/// because it provides custom argument overrides /// -/// This checks that packages defined in `pkgs/by-name` cannot be overridden -/// with an empty second argument like `callPackage ... { }`. -pub struct EmptyNonAutoCalled; +/// In comparison, this ratchet is loose for attributes that: +/// - Are defined in `pkgs/by-name` with a manual definition +/// that doesn't have any custom argument overrides +pub enum ManualDefinition {} + +impl ToNixpkgsProblem for ManualDefinition { + type ToContext = (); -impl ToNixpkgsProblem for EmptyNonAutoCalled { - fn to_nixpkgs_problem(name: &str, _context: &Self, _existed_before: bool) -> NixpkgsProblem { + fn to_nixpkgs_problem( + name: &str, + _optional_from: Option<()>, + _to: &Self::ToContext, + ) -> NixpkgsProblem { NixpkgsProblem::WrongCallPackage { relative_package_file: structure::relative_file_for_package(name), package_name: name.to_owned(), @@ -119,8 +142,10 @@ impl ToNixpkgsProblem for EmptyNonAutoCalled { /// /// This checks that all new package defined using callPackage must be defined via pkgs/by-name /// It also checks that once a package uses pkgs/by-name, it can't switch back to all-packages.nix +pub enum UsesByName {} + #[derive(Clone)] -pub struct UsesByName { +pub struct CouldUseByName { /// The first callPackage argument, used for better errors pub call_package_path: Option<PathBuf>, /// Whether the second callPackage argument is empty, used for better errors @@ -128,18 +153,24 @@ pub struct UsesByName { } impl ToNixpkgsProblem for UsesByName { - fn to_nixpkgs_problem(name: &str, a: &Self, existed_before: bool) -> NixpkgsProblem { - if existed_before { + type ToContext = CouldUseByName; + + fn to_nixpkgs_problem( + name: &str, + optional_from: Option<()>, + to: &Self::ToContext, + ) -> NixpkgsProblem { + if let Some(()) = optional_from { NixpkgsProblem::MovedOutOfByName { package_name: name.to_owned(), - call_package_path: a.call_package_path.clone(), - empty_arg: a.empty_arg, + call_package_path: to.call_package_path.clone(), + empty_arg: to.empty_arg, } } else { NixpkgsProblem::NewPackageNotUsingByName { package_name: name.to_owned(), - call_package_path: a.call_package_path.clone(), - empty_arg: a.empty_arg, + call_package_path: to.call_package_path.clone(), + empty_arg: to.empty_arg, } } } |