From 75cdccd6c4753efe4fa543a05e4d1741cfb63218 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 23 Feb 2024 00:10:20 +0100 Subject: tests.nixpkgs-check-by-name: Improve more errors --- pkgs/test/nixpkgs-check-by-name/src/eval.rs | 38 ++++++----- pkgs/test/nixpkgs-check-by-name/src/nix_file.rs | 35 +++++----- .../nixpkgs-check-by-name/src/nixpkgs_problem.rs | 79 ++++++++++++++++++---- .../tests/alt-callPackage/all-packages.nix | 7 ++ .../tests/alt-callPackage/default.nix | 1 + .../tests/alt-callPackage/expected | 8 +++ .../pkgs/by-name/fo/foo/package.nix | 1 + .../all-packages.nix | 1 - .../tests/override-no-file/expected | 9 ++- .../tests/override-non-path/expected | 9 ++- 10 files changed, 134 insertions(+), 54 deletions(-) create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/all-packages.nix create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/default.nix create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/pkgs/by-name/fo/foo/package.nix diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index fa06e38c357a..ddeac2a445b3 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs @@ -305,7 +305,7 @@ fn by_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.call_package_argument_info_at( + let (optional_syntactic_call_package, definition) = nix_file.call_package_argument_info_at( location.line, location.column, nixpkgs_path, @@ -315,7 +315,7 @@ fn by_name( // `callPackage` match (is_semantic_call_package, optional_syntactic_call_package) { // Something like ` = foo` - (_, Err(definition)) => NixpkgsProblem::NonSyntacticCallPackage { + (_, None) => NixpkgsProblem::NonSyntacticCallPackage { package_name: attribute_name.to_owned(), file: relative_file, line: location.line, @@ -324,17 +324,16 @@ fn by_name( } .into(), // Something like ` = pythonPackages.callPackage ...` - (false, Ok(_)) => { - // All of these are not of the expected form, so error out - // TODO: Make error more specific, don't lump everything together - NixpkgsProblem::WrongCallPackage { - relative_package_file: relative_package_file.to_owned(), - package_name: attribute_name.to_owned(), - } - .into() + (false, Some(_)) => NixpkgsProblem::NonToplevelCallPackage { + package_name: attribute_name.to_owned(), + file: relative_file, + line: location.line, + column: location.column, + definition, } + .into(), // Something like ` = pkgs.callPackage ...` - (true, Ok(syntactic_call_package)) => { + (true, Some(syntactic_call_package)) => { if let Some(path) = syntactic_call_package.relative_path { if path == relative_package_file { // Manual definitions with empty arguments are not allowed @@ -358,9 +357,12 @@ fn by_name( } } else { // No path - NixpkgsProblem::WrongCallPackage { - relative_package_file: relative_package_file.to_owned(), + NixpkgsProblem::NonPath { package_name: attribute_name.to_owned(), + file: relative_file, + line: location.line, + column: location.column, + definition, } .into() } @@ -451,7 +453,7 @@ fn handle_non_by_name_attribute( // Parse the Nix file in the location and 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 + let (optional_syntactic_call_package, _definition) = nix_file_store .get(&location.file)? .call_package_argument_info_at( location.line, @@ -466,16 +468,16 @@ fn handle_non_by_name_attribute( // `callPackage` match (is_semantic_call_package, optional_syntactic_call_package) { // Something like ` = { }` - (false, Err(_)) + (false, None) // Something like ` = pythonPackages.callPackage ...` - | (false, Ok(_)) + | (false, Some(_)) // Something like ` = bar` where `bar = pkgs.callPackage ...` - | (true, Err(_)) => { + | (true, None) => { // In all of these cases, it's not possible to migrate the package to `pkgs/by-name` NonApplicable } // Something like ` = pkgs.callPackage ...` - (true, Ok(syntactic_call_package)) => { + (true, Some(syntactic_call_package)) => { // It's only possible to migrate such a definitions if.. match syntactic_call_package.relative_path { Some(ref rel_path) if rel_path.starts_with(utils::BASE_SUBPATH) => { 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 3840f1dc479d..1382f389e2da 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs @@ -119,13 +119,14 @@ impl NixFile { line: usize, column: usize, relative_to: &Path, - ) -> anyhow::Result> { + ) -> anyhow::Result<(Option, String)> { Ok(match self.attrpath_value_at(line, column)? { - Err(definition) => Err(definition), + Err(definition) => (None, definition), Ok(attrpath_value) => { let definition = attrpath_value.to_string(); - self.attrpath_value_call_package_argument_info(attrpath_value, relative_to)? - .ok_or(definition) + let attrpath_value = + self.attrpath_value_call_package_argument_info(attrpath_value, relative_to)?; + (attrpath_value, definition) } }) } @@ -492,41 +493,41 @@ mod tests { let nix_file = NixFile::new(&file)?; let cases = [ - (2, Err("a.sub = null;")), - (3, Err("b = null;")), - (4, Err("c = import ./file.nix;")), - (5, Err("d = import ./file.nix { };")), + (2, None), + (3, None), + (4, None), + (5, None), ( 6, - Ok(CallPackageArgumentInfo { + Some(CallPackageArgumentInfo { relative_path: Some(PathBuf::from("file.nix")), empty_arg: true, }), ), ( 7, - Ok(CallPackageArgumentInfo { + Some(CallPackageArgumentInfo { relative_path: Some(PathBuf::from("file.nix")), empty_arg: true, }), ), ( 8, - Ok(CallPackageArgumentInfo { + Some(CallPackageArgumentInfo { relative_path: None, empty_arg: true, }), ), ( 9, - Ok(CallPackageArgumentInfo { + Some(CallPackageArgumentInfo { relative_path: Some(PathBuf::from("file.nix")), empty_arg: false, }), ), ( 10, - Ok(CallPackageArgumentInfo { + Some(CallPackageArgumentInfo { relative_path: None, empty_arg: false, }), @@ -534,12 +535,10 @@ mod tests { ]; for (line, expected_result) in cases { - let actual_result = nix_file + let (actual_result, _definition) = nix_file .call_package_argument_info_at(line, 3, temp_dir.path()) - .context(format!("line {line}"))? - .map_err(|x| x); - let owned_expected_result = expected_result.map_err(|x| x.to_string()); - assert_eq!(actual_result, owned_expected_result, "line {line}"); + .context(format!("line {line}"))?; + assert_eq!(actual_result, expected_result, "line {line}"); } Ok(()) 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 d8ae2db4dc22..54976306df32 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs @@ -47,6 +47,20 @@ pub enum NixpkgsProblem { relative_package_file: PathBuf, package_name: String, }, + NonToplevelCallPackage { + package_name: String, + file: PathBuf, + line: usize, + column: usize, + definition: String, + }, + NonPath { + package_name: String, + file: PathBuf, + line: usize, + column: usize, + definition: String, + }, WrongCallPackagePath { package_name: String, file: PathBuf, @@ -182,6 +196,42 @@ 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::NonToplevelCallPackage { package_name, file, line, column, definition } => + writedoc!( + f, + " + - Because {} exists, the attribute `pkgs.{package_name}` must be defined like + + {package_name} = callPackage {} {{ /* ... */ }}; + + This is however not the case: A different `callPackage` is used. + It is defined in {}:{} as + + {}", + structure::relative_dir_for_package(package_name).display(), + structure::relative_file_for_package(package_name).display(), + file.display(), + line, + indent_definition(*column, definition.clone()), + ), + NixpkgsProblem::NonPath { package_name, file, line, column, definition } => + 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 not right: + It is defined in {}:{} as + + {}", + structure::relative_dir_for_package(package_name).display(), + structure::relative_file_for_package(package_name).display(), + file.display(), + line, + indent_definition(*column, definition.clone()), + ), NixpkgsProblem::WrongCallPackagePath { package_name, file, line, column, actual_path, expected_path } => writedoc! { f, @@ -200,20 +250,6 @@ impl fmt::Display for NixpkgsProblem { create_path_expr(file, actual_path), }, NixpkgsProblem::NonSyntacticCallPackage { package_name, file, line, column, definition } => { - // This is needed such multi-line definitions don't look odd - // A bit round-about, but it works and we might not need anything more complicated - let definition_indented = - // The entire code should be indented 4 spaces - textwrap::indent( - // But first we want to strip the code's natural indentation - &textwrap::dedent( - // The definition _doesn't_ include the leading spaces, but we can - // recover those from the column - &format!("{}{definition}", - " ".repeat(column - 1)), - ), - " ", - ); writedoc!( f, " @@ -229,7 +265,7 @@ impl fmt::Display for NixpkgsProblem { structure::relative_file_for_package(package_name).display(), file.display(), line, - definition_indented, + indent_definition(*column, definition.clone()), ) } NixpkgsProblem::NonDerivation { relative_package_file, package_name } => @@ -340,6 +376,19 @@ impl fmt::Display for NixpkgsProblem { } } +fn indent_definition(column: usize, definition: String) -> String { + // The entire code should be indented 4 spaces + textwrap::indent( + // But first we want to strip the code's natural indentation + &textwrap::dedent( + // The definition _doesn't_ include the leading spaces, but we can + // recover those from the column + &format!("{}{definition}", " ".repeat(column - 1)), + ), + " ", + ) +} + /// 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 diff --git a/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/all-packages.nix b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/all-packages.nix new file mode 100644 index 000000000000..399f8eee0a18 --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/all-packages.nix @@ -0,0 +1,7 @@ +self: super: { + + alt.callPackage = self.lib.callPackageWith {}; + + foo = self.alt.callPackage ({ }: self.someDrv) { }; + +} diff --git a/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/default.nix b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/default.nix new file mode 100644 index 000000000000..861260cdca4b --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/default.nix @@ -0,0 +1 @@ +import { root = ./.; } diff --git a/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected new file mode 100644 index 000000000000..25d2e6c4fa6a --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected @@ -0,0 +1,8 @@ +- Because pkgs/by-name/fo/foo exists, the attribute `pkgs.foo` must be defined like + + foo = callPackage pkgs/by-name/fo/foo/package.nix { /* ... */ }; + + This is however not the case: A different `callPackage` is used. + It is defined in all-packages.nix:5 as + + foo = self.alt.callPackage ({ }: self.someDrv) { }; diff --git a/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/pkgs/by-name/fo/foo/package.nix b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/pkgs/by-name/fo/foo/package.nix new file mode 100644 index 000000000000..a1b92efbbadb --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/pkgs/by-name/fo/foo/package.nix @@ -0,0 +1 @@ +{ someDrv }: someDrv diff --git a/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/all-packages.nix b/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/all-packages.nix index 3e0ea20c2281..e31aa831b814 100644 --- a/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/all-packages.nix +++ b/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/all-packages.nix @@ -2,5 +2,4 @@ self: super: { bar = (x: x) self.callPackage ./pkgs/by-name/fo/foo/package.nix { someFlag = true; }; foo = self.bar; - } diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-no-file/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-no-file/expected index 51479e22d26f..0f8a4cb65eef 100644 --- a/pkgs/test/nixpkgs-check-by-name/tests/override-no-file/expected +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-no-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 not right: + It is defined in all-packages.nix:2 as + + nonDerivation = self.callPackage ({ someDrv }: someDrv) { }; diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-non-path/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-non-path/expected index 9df788191ece..41e40a9b3c3f 100644 --- a/pkgs/test/nixpkgs-check-by-name/tests/override-non-path/expected +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-non-path/expected @@ -1 +1,8 @@ -pkgs.foo: 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/fo/foo/package.nix { ... }` with a non-empty second argument. +- Because pkgs/by-name/fo/foo exists, the attribute `pkgs.foo` must be defined like + + foo = callPackage pkgs/by-name/fo/foo/package.nix { /* ... */ }; + + This is however not the case: The first callPackage argument is not right: + It is defined in all-packages.nix:3 as + + foo = self.callPackage ({ someDrv, someFlag }: someDrv) { someFlag = true; }; -- cgit 1.4.1