From 37a54e3ad58bbcc7791fa683bbd79453d8e988ac Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Mon, 5 Feb 2024 01:18:12 +0100 Subject: [PATCH] tests.nixpkgs-check-by-name: Apply feedback from review https://github.com/NixOS/nixpkgs/pull/285089#pullrequestreview-1861099233 Many thanks, Philip Taron! --- pkgs/test/nixpkgs-check-by-name/Cargo.toml | 2 +- pkgs/test/nixpkgs-check-by-name/src/eval.rs | 328 +++++++++--------- pkgs/test/nixpkgs-check-by-name/src/main.rs | 8 +- .../nixpkgs-check-by-name/src/nix_file.rs | 68 ++-- .../src/nixpkgs_problem.rs | 14 - .../nixpkgs-check-by-name/src/references.rs | 101 +++--- .../nixpkgs-check-by-name/src/structure.rs | 10 +- 7 files changed, 252 insertions(+), 279 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/Cargo.toml b/pkgs/test/nixpkgs-check-by-name/Cargo.toml index be15ac5f2453..5240cd69f996 100644 --- a/pkgs/test/nixpkgs-check-by-name/Cargo.toml +++ b/pkgs/test/nixpkgs-check-by-name/Cargo.toml @@ -15,7 +15,7 @@ lazy_static = "1.4.0" colored = "2.0.4" itertools = "0.11.0" rowan = "0.15.11" -indoc = "2.0.4" [dev-dependencies] temp-env = "0.3.5" +indoc = "2.0.4" diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index 53714886fa87..bdde0e560057 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs @@ -2,9 +2,9 @@ use crate::nixpkgs_problem::NixpkgsProblem; use crate::ratchet; use crate::structure; use crate::utils; -use crate::validation::ResultIteratorExt; +use crate::validation::ResultIteratorExt as _; use crate::validation::{self, Validation::Success}; -use crate::NixFileCache; +use crate::NixFileStore; use std::path::Path; use anyhow::Context; @@ -82,7 +82,7 @@ enum CallPackageVariant { /// See the `eval.nix` file for how this is achieved on the Nix side pub fn check_values( nixpkgs_path: &Path, - nix_file_cache: &mut NixFileCache, + nix_file_store: &mut NixFileStore, package_names: Vec, keep_nix_path: bool, ) -> validation::Result { @@ -155,77 +155,77 @@ 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 check_result = validation::sequence( + attributes + .into_iter() + .map(|(attribute_name, attribute_value)| { + let relative_package_file = structure::relative_file_for_package(&attribute_name); - use ratchet::RatchetState::*; - use Attribute::*; - use AttributeInfo::*; - use ByNameAttribute::*; - use CallPackageVariant::*; - use NonByNameAttribute::*; + use ratchet::RatchetState::*; + use Attribute::*; + use AttributeInfo::*; + use ByNameAttribute::*; + use CallPackageVariant::*; + use NonByNameAttribute::*; - let check_result = match attribute_value { - // The attribute succeeds evaluation and is NOT defined in pkgs/by-name - NonByName(EvalSuccess(attribute_info)) => { - 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(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 { - call_package_variant: Auto, - .. - }) => { - // With the current detection mechanism, this also triggers for aliases - // to pkgs/by-name packages, and there's no good method of - // distinguishing alias vs non-alias. - // Using `config.allowAliases = false` at least currently doesn't work - // because there's nothing preventing people from defining aliases that - // are present even with that disabled. - // 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(NonApplicable) - } - // Only derivations can be in pkgs/by-name, - // so this attribute doesn't qualify - CallPackage(CallPackageInfo { - is_derivation: false, - .. - }) => Success(NonApplicable), - // A location of None indicates something weird, we can't really know where - // this attribute is defined, probably an alias - CallPackage(CallPackageInfo { location: None, .. }) => Success(Tight), - // The case of an attribute that qualifies: - // - Uses callPackage - // - Is a derivation - CallPackage(CallPackageInfo { - is_derivation: true, - call_package_variant: Manual { .. }, - location: Some(location), - }) => - // We'll use the attribute's location to parse the file that defines it - match nix_file_cache.get(&location.file)? { - Err(error) => - // This is a bad sign for rnix, because it means cpp Nix could parse - // something that rnix couldn't - anyhow::bail!( - "Could not parse file {} with rnix, even though it parsed with cpp Nix: {error}", - location.file.display() - ), - Ok(nix_file) => - match nix_file.call_package_argument_info_at( - location.line, - location.column, - nixpkgs_path, - )? { + let check_result = match attribute_value { + // The attribute succeeds evaluation and is NOT defined in pkgs/by-name + NonByName(EvalSuccess(attribute_info)) => { + 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(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 { + call_package_variant: Auto, + .. + }) => { + // With the current detection mechanism, this also triggers for aliases + // to pkgs/by-name packages, and there's no good method of + // distinguishing alias vs non-alias. + // Using `config.allowAliases = false` at least currently doesn't work + // because there's nothing preventing people from defining aliases that + // are present even with that disabled. + // 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(NonApplicable) + } + // Only derivations can be in pkgs/by-name, + // so this attribute doesn't qualify + CallPackage(CallPackageInfo { + is_derivation: false, + .. + }) => Success(NonApplicable), + // A location of None indicates something weird, we can't really know where + // this attribute is defined, probably an alias + CallPackage(CallPackageInfo { location: None, .. }) => Success(Tight), + // The case of an attribute that qualifies: + // - Uses callPackage + // - Is a derivation + CallPackage(CallPackageInfo { + is_derivation: true, + call_package_variant: Manual { .. }, + location: Some(location), + }) => + // We'll use the attribute's location to parse the file that defines it + { + match nix_file_store + .get(&location.file)? + .call_package_argument_info_at( + location.line, + location.column, + nixpkgs_path, + )? { + // If the definition is not of the form ` = callPackage ;`, + // it's generally not possible to migrate to `pkgs/by-name` None => Success(NonApplicable), Some(call_package_argument_info) => { - if let Some(ref rel_path) = call_package_argument_info.relative_path { + if let Some(ref rel_path) = + call_package_argument_info.relative_path + { if rel_path.starts_with(utils::BASE_SUBPATH) { // Package variants of by-name packages are explicitly allowed according to RFC 140 // https://github.com/NixOS/rfcs/blob/master/rfcs/0140-simple-package-paths.md#package-variants: @@ -244,100 +244,104 @@ pub fn check_values( } else { Success(Loose(call_package_argument_info)) } - }, - }, - }, - }; - uses_by_name.map(|x| ratchet::Package { - manual_definition: Tight, - uses_by_name: x, - }) - } - NonByName(EvalFailure) => { - // We don't know anything about this attribute really - Success(ratchet::Package { - // 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 { - relative_package_file: relative_package_file.clone(), - package_name: attribute_name.clone(), - } - .into(), - ByName(Existing(NonAttributeSet)) => NixpkgsProblem::NonDerivation { - relative_package_file: relative_package_file.clone(), - package_name: attribute_name.clone(), - } - .into(), - ByName(Existing(NonCallPackage)) => NixpkgsProblem::WrongCallPackage { - relative_package_file: relative_package_file.clone(), - package_name: attribute_name.clone(), - } - .into(), - ByName(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 { - manual_definition: Tight, - uses_by_name: Tight, - }), - // TODO: Use the call_package_argument_info_at instead/additionally and - // simplify the eval.nix code - Manual { path, empty_arg } => { - let correct_file = if let Some(call_package_path) = path { - relative_package_file == *call_package_path - } else { - false - }; - - if correct_file { - Success(ratchet::Package { - // Empty arguments for non-auto-called packages are not allowed anymore. - manual_definition: if *empty_arg { Loose(()) } else { Tight }, - uses_by_name: Tight, - }) - } else { - NixpkgsProblem::WrongCallPackage { - relative_package_file: relative_package_file.clone(), - package_name: attribute_name.clone(), + } } - .into() } - } - }) - } - }; - Ok::<_, anyhow::Error>(check_result.map(|value| (attribute_name.clone(), value))) - }) - .collect_vec()?, + }; + uses_by_name.map(|x| ratchet::Package { + manual_definition: Tight, + uses_by_name: x, + }) + } + NonByName(EvalFailure) => { + // We don't know anything about this attribute really + Success(ratchet::Package { + // 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 { + relative_package_file: relative_package_file.clone(), + package_name: attribute_name.clone(), + } + .into(), + ByName(Existing(NonAttributeSet)) => NixpkgsProblem::NonDerivation { + relative_package_file: relative_package_file.clone(), + package_name: attribute_name.clone(), + } + .into(), + ByName(Existing(NonCallPackage)) => NixpkgsProblem::WrongCallPackage { + relative_package_file: relative_package_file.clone(), + package_name: attribute_name.clone(), + } + .into(), + ByName(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 { + manual_definition: Tight, + uses_by_name: Tight, + }), + // TODO: Use the call_package_argument_info_at instead/additionally and + // simplify the eval.nix code + Manual { path, empty_arg } => { + let correct_file = if let Some(call_package_path) = path { + relative_package_file == *call_package_path + } else { + false + }; + + if correct_file { + Success(ratchet::Package { + // Empty arguments for non-auto-called packages are not allowed anymore. + manual_definition: if *empty_arg { + Loose(()) + } else { + Tight + }, + uses_by_name: Tight, + }) + } else { + NixpkgsProblem::WrongCallPackage { + relative_package_file: relative_package_file.clone(), + package_name: attribute_name.clone(), + } + .into() + } + } + }) + } + }; + Ok::<_, anyhow::Error>(check_result.map(|value| (attribute_name.clone(), value))) + }) + .collect_vec()?, ); Ok(check_result.map(|elems| ratchet::Nixpkgs { diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index 9a8a898a21be..0d0ddcd7e632 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs @@ -1,4 +1,4 @@ -use crate::nix_file::NixFileCache; +use crate::nix_file::NixFileStore; mod eval; mod nix_file; mod nixpkgs_problem; @@ -118,7 +118,7 @@ pub fn check_nixpkgs( keep_nix_path: bool, error_writer: &mut W, ) -> validation::Result { - let mut nix_file_cache = NixFileCache::new(); + let mut nix_file_store = NixFileStore::default(); Ok({ let nixpkgs_path = nixpkgs_path.canonicalize().with_context(|| { @@ -136,9 +136,9 @@ pub fn check_nixpkgs( )?; Success(ratchet::Nixpkgs::default()) } else { - check_structure(&nixpkgs_path, &mut nix_file_cache)?.result_map(|package_names| + check_structure(&nixpkgs_path, &mut nix_file_store)?.result_map(|package_names| // Only if we could successfully parse the structure, we do the evaluation checks - eval::check_values(&nixpkgs_path, &mut nix_file_cache, package_names, keep_nix_path))? + eval::check_values(&nixpkgs_path, &mut nix_file_store, package_names, keep_nix_path))? } }) } diff --git a/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs b/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs index e550f5c459f5..836c5e2dcdda 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs @@ -15,26 +15,20 @@ use std::fs::read_to_string; use std::path::Path; use std::path::PathBuf; -/// A structure to indefinitely cache parse results of Nix files in memory, +/// A structure to store parse results of Nix files in memory, /// making sure that the same file never has to be parsed twice -pub struct NixFileCache { - entries: HashMap, +#[derive(Default)] +pub struct NixFileStore { + entries: HashMap, } -impl NixFileCache { - /// Creates a new empty NixFileCache - pub fn new() -> NixFileCache { - NixFileCache { - entries: HashMap::new(), - } - } - - /// Get the cache entry for a Nix file if it exists, otherwise parse the file, insert it into - /// the cache, and return the value +impl NixFileStore { + /// Get the store entry for a Nix file if it exists, otherwise parse the file, insert it into + /// the store, and return the value /// /// Note that this function only gives an anyhow::Result::Err for I/O errors. /// A parse error is anyhow::Result::Ok(Result::Err(error)) - pub fn get(&mut self, path: &Path) -> anyhow::Result<&NixFileResult> { + pub fn get(&mut self, path: &Path) -> anyhow::Result<&NixFile> { match self.entries.entry(path.to_owned()) { Entry::Occupied(entry) => Ok(entry.into_mut()), Entry::Vacant(entry) => Ok(entry.insert(NixFile::new(path)?)), @@ -42,9 +36,6 @@ impl NixFileCache { } } -/// A type to represent the result when trying to initialise a `NixFile`. -type NixFileResult = Result; - /// A structure for storing a successfully parsed Nix file pub struct NixFile { /// The parent directory of the Nix file, for more convenient error handling @@ -57,7 +48,7 @@ pub struct NixFile { impl NixFile { /// Creates a new NixFile, failing for I/O or parse errors - fn new(path: impl AsRef) -> anyhow::Result { + fn new(path: impl AsRef) -> anyhow::Result { let Some(parent_dir) = path.as_ref().parent() else { anyhow::bail!("Could not get parent of path {}", path.as_ref().display()) }; @@ -66,14 +57,21 @@ impl NixFile { .with_context(|| format!("Could not read file {}", path.as_ref().display()))?; let line_index = LineIndex::new(&contents); - Ok(rnix::Root::parse(&contents) + // 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. + rnix::Root::parse(&contents) + // rnix's ::ok returns Result<_, _> , so no error is thrown away like it would be with + // std::result's ::ok .ok() .map(|syntax_root| NixFile { parent_dir: parent_dir.to_path_buf(), path: path.as_ref().to_owned(), syntax_root, line_index, - })) + }) + .with_context(|| format!("Could not parse file {} with rnix", path.as_ref().display())) } } @@ -88,7 +86,11 @@ pub struct CallPackageArgumentInfo { impl NixFile { /// Returns information about callPackage arguments for an attribute at a specific line/column - /// index. If there's no guaranteed `callPackage` at that location, `None` is returned. + /// index. + /// If the location is not of the form ` = callPackage ;`, `None` is + /// returned. + /// This function only returns `Err` for problems that can't be caused by the Nix contents, + /// but rather problems in this programs code itself. /// /// This is meant to be used with the location returned from `builtins.unsafeGetAttrPos`, e.g.: /// - Create file `default.nix` with contents @@ -102,7 +104,7 @@ impl NixFile { /// builtins.unsafeGetAttrPos "foo" (import ./default.nix { }) /// ``` /// results in `{ file = ./default.nix; line = 2; column = 3; }` - /// - Get the NixFile for `.file` from a `NixFileCache` + /// - Get the NixFile for `.file` from a `NixFileStore` /// - Call this function with `.line`, `.column` and `relative_to` as the (absolute) current directory /// /// You'll get back @@ -165,6 +167,7 @@ impl NixFile { }; if attrpath_node.kind() != SyntaxKind::NODE_ATTRPATH { + // This can happen for e.g. `inherit foo`, so definitely not a syntactic `callPackage` return Ok(None); } // attrpath_node looks like "foo.bar" @@ -183,7 +186,9 @@ impl NixFile { } // attrpath_value_node looks like "foo.bar = 10;" - // unwrap is fine because we confirmed that we can cast with the above check + // unwrap is fine because we confirmed that we can cast with the above check. + // We could avoid this `unwrap` for a `clone`, since `cast` consumes the argument, + // but we still need it for the error message when the cast fails. Ok(Some(ast::AttrpathValue::cast(attrpath_value_node).unwrap())) } @@ -272,9 +277,8 @@ impl NixFile { // Check that is an identifier, or an attribute path with an identifier at the end let ident = match function2 { - Expr::Ident(ident) => - // This means it's something like `foo = callPackage ` - { + Expr::Ident(ident) => { + // This means it's something like `foo = callPackage ` ident } Expr::Select(select) => { @@ -340,13 +344,12 @@ pub enum ResolvedPath { impl NixFile { /// Statically resolves a Nix path expression and checks that it's within an absolute path - /// path /// /// E.g. for the path expression `./bar.nix` in `./foo.nix` and an absolute path of the /// current directory, the function returns `ResolvedPath::Within(./bar.nix)` pub fn static_resolve_path(&self, node: ast::Path, relative_to: &Path) -> ResolvedPath { if node.parts().count() != 1 { - // Only if there's more than 1 interpolated part, it's of the form `./foo/${bar}/baz`. + // If there's more than 1 interpolated part, it's of the form `./foo/${bar}/baz`. return ResolvedPath::Interpolated; } @@ -364,9 +367,8 @@ impl NixFile { // That should be checked more strictly match self.parent_dir.join(Path::new(&text)).canonicalize() { Err(resolution_error) => ResolvedPath::Unresolvable(resolution_error), - Ok(resolved) => - // Check if it's within relative_to - { + Ok(resolved) => { + // Check if it's within relative_to match resolved.strip_prefix(relative_to) { Err(_prefix_error) => ResolvedPath::Outside, Ok(suffix) => ResolvedPath::Within(suffix.to_path_buf()), @@ -411,7 +413,7 @@ mod tests { std::fs::write(&file, contents)?; - let nix_file = NixFile::new(&file)??; + let nix_file = NixFile::new(&file)?; // These are builtins.unsafeGetAttrPos locations for the attributes let cases = [ @@ -454,7 +456,7 @@ mod tests { std::fs::write(&file, contents)?; - let nix_file = NixFile::new(&file)??; + let nix_file = NixFile::new(&file)?; let cases = [ (2, None), diff --git a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs index 16ea65deebfc..25e3ef4863e4 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs @@ -1,6 +1,5 @@ use crate::structure; use crate::utils::PACKAGE_NIX_FILENAME; -use rnix::parser::ParseError; use std::ffi::OsString; use std::fmt; use std::io; @@ -58,11 +57,6 @@ pub enum NixpkgsProblem { subpath: PathBuf, io_error: io::Error, }, - CouldNotParseNix { - relative_package_dir: PathBuf, - subpath: PathBuf, - error: ParseError, - }, PathInterpolation { relative_package_dir: PathBuf, subpath: PathBuf, @@ -184,14 +178,6 @@ impl fmt::Display for NixpkgsProblem { relative_package_dir.display(), subpath.display(), ), - NixpkgsProblem::CouldNotParseNix { relative_package_dir, subpath, error } => - write!( - f, - "{}: File {} could not be parsed by rnix: {}", - relative_package_dir.display(), - subpath.display(), - error, - ), NixpkgsProblem::PathInterpolation { relative_package_dir, subpath, line, text } => write!( f, diff --git a/pkgs/test/nixpkgs-check-by-name/src/references.rs b/pkgs/test/nixpkgs-check-by-name/src/references.rs index 5ff8cf516882..169e996300ba 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/references.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/references.rs @@ -1,7 +1,7 @@ use crate::nixpkgs_problem::NixpkgsProblem; use crate::utils; use crate::validation::{self, ResultIteratorExt, Validation::Success}; -use crate::NixFileCache; +use crate::NixFileStore; use anyhow::Context; use rowan::ast::AstNode; @@ -11,17 +11,20 @@ use std::path::Path; /// Check that every package directory in pkgs/by-name doesn't link to outside that directory. /// Both symlinks and Nix path expressions are checked. pub fn check_references( - nix_file_cache: &mut NixFileCache, + nix_file_store: &mut NixFileStore, relative_package_dir: &Path, absolute_package_dir: &Path, ) -> validation::Result<()> { - // The empty argument here is the subpath under the package directory to check - // An empty one means the package directory itself + // The first subpath to check is the package directory itself, which we can represent as an + // empty path, since the absolute package directory gets prepended to this. + // We don't use `./.` to keep the error messages cleaner + // (there's no canonicalisation going on underneath) + let subpath = Path::new(""); check_path( - nix_file_cache, + nix_file_store, relative_package_dir, absolute_package_dir, - Path::new(""), + subpath, ) .with_context(|| { format!( @@ -32,8 +35,12 @@ pub fn check_references( } /// Checks for a specific path to not have references outside +/// +/// The subpath is the relative path within the package directory we're currently checking. +/// A relative path so that the error messages don't get absolute paths (which are messy in CI). +/// The absolute package directory gets prepended before doing anything with it though. fn check_path( - nix_file_cache: &mut NixFileCache, + nix_file_store: &mut NixFileStore, relative_package_dir: &Path, absolute_package_dir: &Path, subpath: &Path, @@ -69,23 +76,22 @@ fn check_path( utils::read_dir_sorted(&path)? .into_iter() .map(|entry| { - let entry_subpath = subpath.join(entry.file_name()); check_path( - nix_file_cache, + nix_file_store, relative_package_dir, absolute_package_dir, - &entry_subpath, + &subpath.join(entry.file_name()), ) - .with_context(|| format!("Error while recursing into {}", subpath.display())) }) - .collect_vec()?, + .collect_vec() + .with_context(|| format!("Error while recursing into {}", subpath.display()))?, ) } else if path.is_file() { // Only check Nix files if let Some(ext) = path.extension() { if ext == OsStr::new("nix") { check_nix_file( - nix_file_cache, + nix_file_store, relative_package_dir, absolute_package_dir, subpath, @@ -106,29 +112,14 @@ fn check_path( /// Check whether a nix file contains path expression references pointing outside the package /// directory fn check_nix_file( - nix_file_cache: &mut NixFileCache, + nix_file_store: &mut NixFileStore, relative_package_dir: &Path, absolute_package_dir: &Path, subpath: &Path, ) -> validation::Result<()> { let path = absolute_package_dir.join(subpath); - let nix_file = match nix_file_cache.get(&path)? { - Ok(nix_file) => nix_file, - Err(error) => - // 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(), - error: error.clone(), - } - .into()) - } - }; + let nix_file = nix_file_store.get(&path)?; Ok(validation::sequence_( nix_file.syntax_root.syntax().descendants().map(|node| { @@ -140,40 +131,31 @@ fn check_nix_file( return Success(()); }; - use crate::nix_file::ResolvedPath::*; + use crate::nix_file::ResolvedPath; match nix_file.static_resolve_path(path, absolute_package_dir) { - Interpolated => - // Filters out ./foo/${bar}/baz - // TODO: We can just check ./foo - { - NixpkgsProblem::PathInterpolation { - relative_package_dir: relative_package_dir.to_path_buf(), - subpath: subpath.to_path_buf(), - line, - text, - } - .into() - } - SearchPath => - // Filters out search paths like - { - NixpkgsProblem::SearchPath { - relative_package_dir: relative_package_dir.to_path_buf(), - subpath: subpath.to_path_buf(), - line, - text, - } - .into() - } - Outside => NixpkgsProblem::OutsidePathReference { + ResolvedPath::Interpolated => NixpkgsProblem::PathInterpolation { relative_package_dir: relative_package_dir.to_path_buf(), subpath: subpath.to_path_buf(), line, text, } .into(), - Unresolvable(e) => NixpkgsProblem::UnresolvablePathReference { + ResolvedPath::SearchPath => NixpkgsProblem::SearchPath { + relative_package_dir: relative_package_dir.to_path_buf(), + subpath: subpath.to_path_buf(), + line, + text, + } + .into(), + ResolvedPath::Outside => NixpkgsProblem::OutsidePathReference { + relative_package_dir: relative_package_dir.to_path_buf(), + subpath: subpath.to_path_buf(), + line, + text, + } + .into(), + ResolvedPath::Unresolvable(e) => NixpkgsProblem::UnresolvablePathReference { relative_package_dir: relative_package_dir.to_path_buf(), subpath: subpath.to_path_buf(), line, @@ -181,10 +163,9 @@ fn check_nix_file( io_error: e, } .into(), - Within(_p) => - // No need to handle the case of it being inside the directory, since we scan through the - // entire directory recursively anyways - { + ResolvedPath::Within(..) => { + // No need to handle the case of it being inside the directory, since we scan through the + // entire directory recursively anyways Success(()) } } diff --git a/pkgs/test/nixpkgs-check-by-name/src/structure.rs b/pkgs/test/nixpkgs-check-by-name/src/structure.rs index 7f9e7d4f717f..9b615dd9969a 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/structure.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/structure.rs @@ -3,7 +3,7 @@ use crate::references; use crate::utils; use crate::utils::{BASE_SUBPATH, PACKAGE_NIX_FILENAME}; use crate::validation::{self, ResultIteratorExt, Validation::Success}; -use crate::NixFileCache; +use crate::NixFileStore; use itertools::concat; use lazy_static::lazy_static; use regex::Regex; @@ -37,7 +37,7 @@ pub fn relative_file_for_package(package_name: &str) -> PathBuf { /// `pkgs/by-name` pub fn check_structure( path: &Path, - nix_file_cache: &mut NixFileCache, + nix_file_store: &mut NixFileStore, ) -> validation::Result> { let base_dir = path.join(BASE_SUBPATH); @@ -93,7 +93,7 @@ pub fn check_structure( .into_iter() .map(|package_entry| { check_package( - nix_file_cache, + nix_file_store, path, &shard_name, shard_name_valid, @@ -112,7 +112,7 @@ pub fn check_structure( } fn check_package( - nix_file_cache: &mut NixFileCache, + nix_file_store: &mut NixFileStore, path: &Path, shard_name: &str, shard_name_valid: bool, @@ -172,7 +172,7 @@ fn check_package( }); let result = result.and(references::check_references( - nix_file_cache, + nix_file_store, &relative_package_dir, &path.join(&relative_package_dir), )?);