tests.nixpkgs-check-by-name: Improve error for wrong package file

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 904a9cff0e789..b02f7075ab613 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",
@@ -393,6 +394,12 @@ 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"
 source = "registry+https://github.com/rust-lang/crates.io-index"
diff --git a/pkgs/test/nixpkgs-check-by-name/Cargo.toml b/pkgs/test/nixpkgs-check-by-name/Cargo.toml
index 5240cd69f996e..1b8f8e9085d4e 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 122c4b7348971..fa06e38c357ad 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<String> {
-        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<PathBuf> {
+        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 <arg1> <arg2>`,
+                        // 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 <arg1> <arg2>`,
                         // 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 `<attr> = 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 0d0ddcd7e6327..24415424914e1 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 18001eb44ecd5..dda89895697f0 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<Path>, to_file: impl AsRef<Path>) -> 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 51479e22d26fe..3566b15bdbc40 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 { /* ... */ }