From a53b07e2523978518efc6ae161ef9ffba7933d6e Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Thu, 22 Feb 2024 23:04:40 +0100 Subject: [PATCH] tests.nixpkgs-check-by-name: Improve error for wrong package file --- pkgs/test/nixpkgs-check-by-name/Cargo.lock | 7 +++ pkgs/test/nixpkgs-check-by-name/Cargo.toml | 3 +- pkgs/test/nixpkgs-check-by-name/src/eval.rs | 63 +++++++++++-------- pkgs/test/nixpkgs-check-by-name/src/main.rs | 2 +- .../src/nixpkgs_problem.rs | 53 +++++++++++++++- .../tests/override-different-file/expected | 9 ++- 6 files changed, 106 insertions(+), 31 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/Cargo.lock b/pkgs/test/nixpkgs-check-by-name/Cargo.lock index 904a9cff0e78..b02f7075ab61 100644 --- a/pkgs/test/nixpkgs-check-by-name/Cargo.lock +++ b/pkgs/test/nixpkgs-check-by-name/Cargo.lock @@ -299,6 +299,7 @@ dependencies = [ "itertools", "lazy_static", "regex", + "relative-path", "rnix", "rowan", "serde", @@ -392,6 +393,12 @@ version = "0.7.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e5ea92a5b6195c6ef2a0295ea818b312502c6fc94dde986c5553242e18fd4ce2" +[[package]] +name = "relative-path" +version = "1.9.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e898588f33fdd5b9420719948f9f2a32c922a246964576f71ba7f24f80610fbc" + [[package]] name = "rnix" version = "0.11.0" diff --git a/pkgs/test/nixpkgs-check-by-name/Cargo.toml b/pkgs/test/nixpkgs-check-by-name/Cargo.toml index 5240cd69f996..1b8f8e9085d4 100644 --- a/pkgs/test/nixpkgs-check-by-name/Cargo.toml +++ b/pkgs/test/nixpkgs-check-by-name/Cargo.toml @@ -15,7 +15,8 @@ lazy_static = "1.4.0" colored = "2.0.4" itertools = "0.11.0" rowan = "0.15.11" +indoc = "2.0.4" +relative-path = "1.9.2" [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 122c4b734897..fa06e38c357a 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs @@ -52,22 +52,19 @@ struct Location { } impl Location { - // Creates a [String] from a [Location], in a format like `{file}:{line}:{column}`, - // where `file` is relative to the given [Path]. - fn to_relative_string(&self, nixpkgs_path: &Path) -> anyhow::Result { - let relative = self.file.strip_prefix(nixpkgs_path).with_context(|| { - format!( - "An attributes location ({}) is outside Nixpkgs ({})", - self.file.display(), - nixpkgs_path.display() - ) - })?; - Ok(format!( - "{}:{}:{}", - relative.display(), - self.line, - self.column - )) + // Returns the [file] field, but relative to Nixpkgs + fn relative_file(&self, nixpkgs_path: &Path) -> anyhow::Result { + Ok(self + .file + .strip_prefix(nixpkgs_path) + .with_context(|| { + format!( + "The file ({}) is outside Nixpkgs ({})", + self.file.display(), + nixpkgs_path.display() + ) + })? + .to_path_buf()) } } @@ -295,16 +292,24 @@ fn by_name( // We should expect manual definitions to have a location, otherwise we can't // enforce the expected format if let Some(location) = location { - // Parse the Nix file in the location and figure out whether it's an - // attribute definition of the form `= callPackage `, + // Parse the Nix file in the location + let nix_file = nix_file_store.get(&location.file)?; + + // The relative path of the Nix file, for error messages + let relative_file = + location.relative_file(nixpkgs_path).with_context(|| { + format!( + "Failed to resolve location file of attribute {attribute_name}" + ) + })?; + + // Figure out whether it's an attribute definition of the form `= callPackage `, // returning the arguments if so. - let optional_syntactic_call_package = nix_file_store - .get(&location.file)? - .call_package_argument_info_at( + let optional_syntactic_call_package = nix_file.call_package_argument_info_at( location.line, location.column, nixpkgs_path, - )?; + ).with_context(|| format!("Failed to get the definition info for attribute {attribute_name}"))?; // At this point, we completed two different checks for whether it's a // `callPackage` @@ -312,7 +317,9 @@ fn by_name( // Something like ` = foo` (_, Err(definition)) => NixpkgsProblem::NonSyntacticCallPackage { package_name: attribute_name.to_owned(), - location: location.to_relative_string(nixpkgs_path)?, + file: relative_file, + line: location.line, + column: location.column, definition, } .into(), @@ -338,13 +345,19 @@ fn by_name( Tight }) } else { - NixpkgsProblem::WrongCallPackage { - relative_package_file: relative_package_file.to_owned(), + // Wrong path + NixpkgsProblem::WrongCallPackagePath { package_name: attribute_name.to_owned(), + file: relative_file, + line: location.line, + column: location.column, + actual_path: path, + expected_path: relative_package_file, } .into() } } else { + // No path NixpkgsProblem::WrongCallPackage { relative_package_file: relative_package_file.to_owned(), package_name: attribute_name.to_owned(), diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index 0d0ddcd7e632..24415424914e 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs @@ -249,7 +249,7 @@ mod tests { if actual_errors != expected_errors { panic!( - "Failed test case {name}, expected these errors:\n\n{}\n\nbut got these:\n\n{}", + "Failed test case {name}, expected these errors:\n=======\n{}\n=======\nbut got these:\n=======\n{}\n=======", expected_errors, actual_errors ); } 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 18001eb44ecd..dda89895697f 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs @@ -1,8 +1,11 @@ use crate::structure; use crate::utils::PACKAGE_NIX_FILENAME; +use indoc::writedoc; +use relative_path::RelativePath; use std::ffi::OsString; use std::fmt; use std::io; +use std::path::Path; use std::path::PathBuf; /// Any problem that can occur when checking Nixpkgs @@ -44,9 +47,19 @@ pub enum NixpkgsProblem { relative_package_file: PathBuf, package_name: String, }, + WrongCallPackagePath { + package_name: String, + file: PathBuf, + line: usize, + column: usize, + actual_path: PathBuf, + expected_path: PathBuf, + }, NonSyntacticCallPackage { package_name: String, - location: String, + file: PathBuf, + line: usize, + column: usize, definition: String, }, NonDerivation { @@ -169,12 +182,32 @@ impl fmt::Display for NixpkgsProblem { "pkgs.{package_name}: This attribute is manually defined (most likely in pkgs/top-level/all-packages.nix), which is only allowed if the definition is of the form `pkgs.callPackage {} {{ ... }}` with a non-empty second argument.", relative_package_file.display() ), - NixpkgsProblem::NonSyntacticCallPackage { package_name, location, definition } => + NixpkgsProblem::WrongCallPackagePath { package_name, file, line, column, actual_path, expected_path } => + writedoc! { + f, + " + - Because {} exists, the attribute `pkgs.{package_name}` must be defined like + + {package_name} = callPackage {} {{ /* ... */ }} + + This is however not the case: The first `callPackage` argument is the wrong path. + It is defined in {}:{}:{} as + + {package_name} = callPackage {} {{ /* ... */ }}", + structure::relative_dir_for_package(package_name).display(), + create_path_expr(file, expected_path), + file.display(), line, column, + create_path_expr(file, actual_path), + }, + NixpkgsProblem::NonSyntacticCallPackage { package_name, file, line, column, definition } => write!( f, - "Because {} exists, the attribute `pkgs.{package_name}` must be defined as `callPackage {} {{ ... }}`. This is however not the case: The attribute is defined in {location} as\n\t{}", + "Because {} exists, the attribute `pkgs.{package_name}` must be defined as `callPackage {} {{ ... }}`. This is however not the case: The attribute is defined in {}:{}:{} as\n\t{}", structure::relative_dir_for_package(package_name).display(), structure::relative_file_for_package(package_name).display(), + file.display(), + line, + column, definition, ), NixpkgsProblem::NonDerivation { relative_package_file, package_name } => @@ -284,3 +317,17 @@ impl fmt::Display for NixpkgsProblem { } } } + +/// Creates a Nix path expression that when put into Nix file `from_file`, would point to the `to_file`. +fn create_path_expr(from_file: impl AsRef, to_file: impl AsRef) -> String { + // FIXME: Clean these unwrap's up + // But these should never trigger because we only call this function with relative paths, and only + // with `from_file` being an actual file (so there's always a parent) + let from = RelativePath::from_path(&from_file) + .unwrap() + .parent() + .unwrap(); + let to = RelativePath::from_path(&to_file).unwrap(); + let rel = from.relative(to); + format!("./{rel}") +} diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected index 51479e22d26f..3566b15bdbc4 100644 --- a/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected @@ -1 +1,8 @@ -pkgs.nonDerivation: This attribute is manually defined (most likely in pkgs/top-level/all-packages.nix), which is only allowed if the definition is of the form `pkgs.callPackage pkgs/by-name/no/nonDerivation/package.nix { ... }` with a non-empty second argument. +- Because pkgs/by-name/no/nonDerivation exists, the attribute `pkgs.nonDerivation` must be defined like + + nonDerivation = callPackage ./pkgs/by-name/no/nonDerivation/package.nix { /* ... */ } + + This is however not the case: The first `callPackage` argument is the wrong path. + It is defined in all-packages.nix:2:3 as + + nonDerivation = callPackage ./someDrv.nix { /* ... */ }