diff options
Diffstat (limited to 'pkgs/test/nixpkgs-check-by-name/src')
| -rw-r--r-- | pkgs/test/nixpkgs-check-by-name/src/eval.rs | 68 | ||||
| -rw-r--r-- | pkgs/test/nixpkgs-check-by-name/src/main.rs | 42 | ||||
| -rw-r--r-- | pkgs/test/nixpkgs-check-by-name/src/ratchet.rs | 99 | ||||
| -rw-r--r-- | pkgs/test/nixpkgs-check-by-name/src/references.rs | 4 |
4 files changed, 112 insertions, 101 deletions
diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index c4dad8257cf98..dd30cb9045e53 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs @@ -2,8 +2,6 @@ use crate::nixpkgs_problem::NixpkgsProblem; use crate::ratchet; use crate::structure; use crate::validation::{self, Validation::Success}; -use std::collections::HashMap; -use std::ffi::OsString; use std::path::Path; use anyhow::Context; @@ -73,7 +71,7 @@ enum CallPackageVariant { pub fn check_values( nixpkgs_path: &Path, package_names: Vec<String>, - eval_nix_path: &HashMap<String, PathBuf>, + keep_nix_path: bool, ) -> validation::Result<ratchet::Nixpkgs> { // Write the list of packages we need to check into a temporary JSON file. // This can then get read by the Nix evaluation. @@ -99,8 +97,6 @@ pub fn check_values( command // Inherit stderr so that error messages always get shown .stderr(process::Stdio::inherit()) - // Clear NIX_PATH to be sure it doesn't influence the result - .env_remove("NIX_PATH") .args([ "--eval", "--json", @@ -121,15 +117,12 @@ pub fn check_values( .arg("-I") .arg(nixpkgs_path); - // Also add extra paths that need to be accessible - for (name, path) in eval_nix_path { - command.arg("-I"); - let mut name_value = OsString::new(); - name_value.push(name); - name_value.push("="); - name_value.push(path); - command.arg(name_value); + // Clear NIX_PATH to be sure it doesn't influence the result + // But not when requested to keep it, used so that the tests can pass extra Nix files + if !keep_nix_path { + command.env_remove("NIX_PATH"); } + command.args(["-I", &expr_path]); command.arg(expr_path); @@ -166,8 +159,8 @@ pub fn check_values( let uses_by_name = match attribute_info { // In these cases the package doesn't qualify for being in pkgs/by-name, // so the UsesByName ratchet is already as tight as it can be - NonAttributeSet => Success(Tight), - NonCallPackage => Success(Tight), + NonAttributeSet => Success(NonApplicable), + NonCallPackage => Success(NonApplicable), // This is the case when the `pkgs/by-name`-internal _internalCallByNamePackageFile // is used for a package outside `pkgs/by-name` CallPackage(CallPackageInfo { @@ -183,14 +176,14 @@ pub fn check_values( // In the future we could kind of abuse this behavior to have better // enforcement of conditional aliases, but for now we just need to not // give an error. - Success(Tight) + Success(NonApplicable) } // Only derivations can be in pkgs/by-name, // so this attribute doesn't qualify CallPackage(CallPackageInfo { is_derivation: false, .. - }) => Success(Tight), + }) => Success(NonApplicable), // The case of an attribute that qualifies: // - Uses callPackage @@ -198,30 +191,35 @@ 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, }) } NonByName(EvalFailure) => { - // This is a bit of an odd case: We don't even _know_ whether this attribute - // would qualify for using pkgs/by-name. We can either: - // - Assume it's not using pkgs/by-name, which has the problem that if a - // package evaluation gets broken temporarily, the fix can remove it from - // pkgs/by-name again - // - Assume it's using pkgs/by-name already, which has the problem that if a - // package evaluation gets broken temporarily, fixing it requires a move to - // pkgs/by-name - // We choose the latter, since we want to move towards pkgs/by-name, not away - // from it + // We don't know anything about this attribute really Success(ratchet::Package { - empty_non_auto_called: Tight, - uses_by_name: Tight, + // We'll assume that we can't remove any manual definitions, which has the + // minimal drawback that if there was a manual definition that could've + // been removed, fixing the package requires removing the definition, no + // big deal, that's a minor edit. + manual_definition: Tight, + + // Regarding whether this attribute could `pkgs/by-name`, we don't really + // know, so return NonApplicable, which has the effect that if a + // package evaluation gets broken temporarily, the fix can remove it from + // pkgs/by-name again. For now this isn't our problem, but in the future we + // might have another check to enforce that evaluation must not be broken. + // The alternative of assuming that it's using `pkgs/by-name` already + // has the problem that if a package evaluation gets broken temporarily, + // fixing it requires a move to pkgs/by-name, which could happen more + // often and isn't really justified. + uses_by_name: NonApplicable, }) } ByName(Missing) => NixpkgsProblem::UndefinedAttr { @@ -255,7 +253,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 } => { @@ -268,11 +266,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/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index 273ebca1643ed..8179ec8ded74b 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs @@ -12,7 +12,6 @@ use crate::validation::Validation::Success; use anyhow::Context; use clap::Parser; use colored::Colorize; -use std::collections::HashMap; use std::io; use std::path::{Path, PathBuf}; use std::process::ExitCode; @@ -45,12 +44,7 @@ pub struct Args { fn main() -> ExitCode { let args = Args::parse(); - match process( - &args.base, - &args.nixpkgs, - &HashMap::new(), - &mut io::stderr(), - ) { + match process(&args.base, &args.nixpkgs, false, &mut io::stderr()) { Ok(true) => { eprintln!("{}", "Validated successfully".green()); ExitCode::SUCCESS @@ -71,9 +65,9 @@ fn main() -> ExitCode { /// # Arguments /// - `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 +/// - `keep_nix_path`: Whether the value of the NIX_PATH environment variable should be kept for +/// the evaluation stage, allowing its contents to be accessed. +/// This is used to allow the tests to access e.g. the mock-nixpkgs.nix file /// - `error_writer`: An `io::Write` value to write validation errors to, if any. /// /// # Return value @@ -83,15 +77,15 @@ fn main() -> ExitCode { pub fn process<W: io::Write>( base_nixpkgs: &Path, main_nixpkgs: &Path, - eval_nix_path: &HashMap<String, PathBuf>, + keep_nix_path: bool, error_writer: &mut W, ) -> anyhow::Result<bool> { // Check the main Nixpkgs first - let main_result = check_nixpkgs(main_nixpkgs, eval_nix_path, error_writer)?; + let main_result = check_nixpkgs(main_nixpkgs, keep_nix_path, 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 - check_nixpkgs(base_nixpkgs, eval_nix_path, error_writer)?.result_map( + check_nixpkgs(base_nixpkgs, keep_nix_path, error_writer)?.result_map( |base_nixpkgs_version| { Ok(ratchet::Nixpkgs::compare( base_nixpkgs_version, @@ -119,7 +113,7 @@ pub fn process<W: io::Write>( /// ratchet check against another result. pub fn check_nixpkgs<W: io::Write>( nixpkgs_path: &Path, - eval_nix_path: &HashMap<String, PathBuf>, + keep_nix_path: bool, error_writer: &mut W, ) -> validation::Result<ratchet::Nixpkgs> { Ok({ @@ -140,7 +134,7 @@ pub fn check_nixpkgs<W: io::Write>( } 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_nix_path))? + eval::check_values(&nixpkgs_path, package_names, keep_nix_path))? } }) } @@ -150,10 +144,8 @@ mod tests { use crate::process; use crate::utils; use anyhow::Context; - use std::collections::HashMap; use std::fs; use std::path::Path; - use std::path::PathBuf; use tempfile::{tempdir_in, TempDir}; #[test] @@ -234,20 +226,6 @@ mod tests { } fn test_nixpkgs(name: &str, path: &Path, expected_errors: &str) -> anyhow::Result<()> { - let eval_nix_path = HashMap::from([ - ( - "test-nixpkgs".to_string(), - PathBuf::from("tests/mock-nixpkgs.nix"), - ), - ( - "test-nixpkgs/lib".to_string(), - PathBuf::from( - std::env::var("NIXPKGS_LIB_PATH") - .with_context(|| "Could not get environment variable NIXPKGS_LIB_PATH")?, - ), - ), - ]); - let base_path = path.join("base"); let base_nixpkgs = if base_path.exists() { base_path.as_path() @@ -258,7 +236,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, &eval_nix_path, &mut writer) + process(base_nixpkgs, &path, true, &mut writer) .with_context(|| format!("Failed test case {name}"))?; Ok(writer) })?; diff --git a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs index f8c129626cc00..10ecc01d3580c 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,55 +58,82 @@ 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. Tight, + /// This ratchet can't be applied. + /// State transitions from/to NonApplicable are always allowed + NonApplicable, } /// 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) + // - Anything involving NotApplicable, where we can't really make any good calls + _ => 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 +146,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 +157,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, } } } diff --git a/pkgs/test/nixpkgs-check-by-name/src/references.rs b/pkgs/test/nixpkgs-check-by-name/src/references.rs index 3b3b05419780a..ce7403afb32d6 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/references.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/references.rs @@ -106,6 +106,10 @@ fn check_nix_file( let root = Root::parse(&contents); if let Some(error) = root.errors().first() { + // NOTE: There's now another Nixpkgs CI check to make sure all changed Nix files parse + // correctly, though that uses mainline Nix instead of rnix, so it doesn't give the same + // errors. In the future we should unify these two checks, ideally moving the other CI + // check into this tool as well and checking for both mainline Nix and rnix. return Ok(NixpkgsProblem::CouldNotParseNix { relative_package_dir: relative_package_dir.to_path_buf(), subpath: subpath.to_path_buf(), |