From 712ac796e5be1c6bcafad54d46b7cdcd7fdc7bcb Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Mon, 19 Feb 2024 22:02:27 +0100
Subject: [PATCH 01/18] tests.nixpkgs-check-by-name: Improve inherit detection

Detect inherit's better, such that a future commit can use this
information in an error message
---
 .../nixpkgs-check-by-name/src/nix_file.rs     | 36 +++++++++++++++----
 1 file changed, 30 insertions(+), 6 deletions(-)

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 836c5e2dcdda..ea0833792f3c 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs
@@ -5,7 +5,6 @@ use anyhow::Context;
 use rnix::ast;
 use rnix::ast::Expr;
 use rnix::ast::HasEntry;
-use rnix::SyntaxKind;
 use rowan::ast::AstNode;
 use rowan::TextSize;
 use rowan::TokenAtOffset;
@@ -158,6 +157,22 @@ impl NixFile {
             )
         };
 
+        if ast::Attr::can_cast(node.kind()) {
+            // Something like `foo`, `"foo"` or `${"foo"}`
+        } else if ast::Inherit::can_cast(node.kind()) {
+            // Something like `inherit <attr>` or `inherit (<source>) <attr>`
+            // This is the only other way how `builtins.unsafeGetAttrPos` can return
+            // attribute positions, but we only look for ones like `<attr-path> = <value>`, so
+            // ignore this
+            return Ok(None);
+        } else {
+            // However, anything else is not expected and smells like a bug
+            anyhow::bail!(
+                "Node in {} is neither an attribute node, nor an inherit node: {node:?}",
+                self.path.display()
+            )
+        }
+
         // node looks like "foo"
         let Some(attrpath_node) = node.parent() else {
             anyhow::bail!(
@@ -166,10 +181,14 @@ 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);
+        if !ast::Attrpath::can_cast(attrpath_node.kind()) {
+            // We know that `node` is an attribute, its parent should be an attribute path
+            anyhow::bail!(
+                "In {}, attribute parent node is not an attribute path node: {attrpath_node:?}",
+                self.path.display()
+            )
         }
+
         // attrpath_node looks like "foo.bar"
         let Some(attrpath_value_node) = attrpath_node.parent() else {
             anyhow::bail!(
@@ -408,6 +427,7 @@ mod tests {
               /**/quuux/**/=/**/5/**/;/*E*/
 
               inherit toInherit;
+              inherit (toInherit) toInherit;
             }
         "#};
 
@@ -424,11 +444,13 @@ mod tests {
             (8, 3, Some("quux\n  # B\n  =\n  # C\n  5\n  # D\n  ;")),
             (17, 7, Some("quuux/**/=/**/5/**/;")),
             (19, 10, None),
+            (20, 22, None),
         ];
 
         for (line, column, expected_result) in cases {
             let actual_result = nix_file
-                .attrpath_value_at(line, column)?
+                .attrpath_value_at(line, column)
+                .context(format!("line {line}, column {column}"))?
                 .map(|node| node.to_string());
             assert_eq!(actual_result.as_deref(), expected_result);
         }
@@ -501,7 +523,9 @@ mod tests {
         ];
 
         for (line, expected_result) in cases {
-            let actual_result = nix_file.call_package_argument_info_at(line, 3, temp_dir.path())?;
+            let actual_result = nix_file
+                .call_package_argument_info_at(line, 3, temp_dir.path())
+                .context(format!("line {line}"))?;
             assert_eq!(actual_result, expected_result);
         }
 

From a61c8c9f5c19030102f62820db1ad037311702f7 Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Mon, 19 Feb 2024 22:28:01 +0100
Subject: [PATCH 02/18] tests.nixpkgs-check-by-name: Fix allowing non-path
 overrides

An edge case was allowed when it shouldn't be: A package defined in
`pkgs/by-name` could be overridden in `all-packages.nix` if it was of
the form `callPackage (<expr>) { <non-empty> }`.

This is not right, it's required that the first argument be the path
matching the package to be overridden.
---
 pkgs/test/nixpkgs-check-by-name/src/eval.rs   | 36 +++++++++----------
 .../tests/override-non-path/all-packages.nix  |  5 +++
 .../tests/override-non-path/default.nix       |  1 +
 .../tests/override-non-path/expected          |  1 +
 .../pkgs/by-name/fo/foo/package.nix           |  1 +
 5 files changed, 26 insertions(+), 18 deletions(-)
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/override-non-path/all-packages.nix
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/override-non-path/default.nix
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/override-non-path/expected
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/override-non-path/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 e90a95533144..1322f0faaf91 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
@@ -195,7 +195,6 @@ fn by_name(
     use ByNameAttribute::*;
 
     let relative_package_file = structure::relative_file_for_package(attribute_name);
-    let absolute_package_file = nixpkgs_path.join(&relative_package_file);
 
     // At this point we know that `pkgs/by-name/fo/foo/package.nix` has to exists.
     // This match decides whether the attribute `foo` is defined accordingly
@@ -284,23 +283,17 @@ fn by_name(
                             .call_package_argument_info_at(
                             location.line,
                             location.column,
-                            // We're passing `pkgs/by-name/fo/foo/package.nix` here, which causes
-                            // the function to verify that `<arg1>` is the same path,
-                            // making `syntactic_call_package.relative_path` end up as `""`
-                            // TODO: This is confusing and should be improved
-                            &absolute_package_file,
+                            nixpkgs_path,
                         )?;
 
                         // At this point, we completed two different checks for whether it's a
                         // `callPackage`
                         match (is_semantic_call_package, optional_syntactic_call_package) {
                             // Something like `<attr> = { ... }`
-                            // or a `pkgs.callPackage` but with the wrong file
                             (false, None)
-                            // Something like `<attr> = pythonPackages.callPackage ./pkgs/by-name/...`
+                            // Something like `<attr> = pythonPackages.callPackage ...`
                             | (false, Some(_))
                             // Something like `<attr> = bar` where `bar = pkgs.callPackage ...`
-                            // or a `callPackage` but with the wrong file
                             | (true, None) => {
                                 // All of these are not of the expected form, so error out
                                 // TODO: Make error more specific, don't lump everything together
@@ -309,18 +302,25 @@ fn by_name(
                                       package_name: attribute_name.to_owned(),
                                 }.into()
                             }
-                            // Something like `<attr> = pkgs.callPackage ./pkgs/by-name/...`,
-                            // with the correct file
+                            // Something like `<attr> = pkgs.callPackage ...`
                             (true, Some(syntactic_call_package)) => {
-                                Success(
-                                    // Manual definitions with empty arguments are not allowed
-                                    // anymore
-                                    if syntactic_call_package.empty_arg {
-                                        Loose(())
+                                if let Some(path) = syntactic_call_package.relative_path {
+                                    if path == relative_package_file {
+                                        // Manual definitions with empty arguments are not allowed
+                                        // anymore
+                                        Success(if syntactic_call_package.empty_arg { Loose(()) } else { Tight })
                                     } else {
-                                        Tight
+                                        NixpkgsProblem::WrongCallPackage {
+                                              relative_package_file: relative_package_file.to_owned(),
+                                              package_name: attribute_name.to_owned(),
+                                        }.into()
                                     }
-                                )
+                                } else {
+                                    NixpkgsProblem::WrongCallPackage {
+                                          relative_package_file: relative_package_file.to_owned(),
+                                          package_name: attribute_name.to_owned(),
+                                    }.into()
+                                }
                             }
                         }
                     } else {
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-non-path/all-packages.nix b/pkgs/test/nixpkgs-check-by-name/tests/override-non-path/all-packages.nix
new file mode 100644
index 000000000000..f07e7a42744a
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-non-path/all-packages.nix
@@ -0,0 +1,5 @@
+self: super: {
+
+  foo = self.callPackage ({ someDrv, someFlag }: someDrv) { someFlag = true; };
+
+}
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-non-path/default.nix b/pkgs/test/nixpkgs-check-by-name/tests/override-non-path/default.nix
new file mode 100644
index 000000000000..861260cdca4b
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-non-path/default.nix
@@ -0,0 +1 @@
+import <test-nixpkgs> { root = ./.; }
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
new file mode 100644
index 000000000000..9df788191ece
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-non-path/expected
@@ -0,0 +1 @@
+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.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-non-path/pkgs/by-name/fo/foo/package.nix b/pkgs/test/nixpkgs-check-by-name/tests/override-non-path/pkgs/by-name/fo/foo/package.nix
new file mode 100644
index 000000000000..a1b92efbbadb
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-non-path/pkgs/by-name/fo/foo/package.nix
@@ -0,0 +1 @@
+{ someDrv }: someDrv

From 24069b982dcb3b64bb3de2fc3c7b3463f4b00723 Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Mon, 19 Feb 2024 23:35:18 +0100
Subject: [PATCH 03/18] tests.nixpkgs-check-by-name: Better error for
 non-syntactic callPackage

---
 pkgs/test/nixpkgs-check-by-name/src/eval.rs   | 70 ++++++++++++-----
 .../nixpkgs-check-by-name/src/nix_file.rs     | 75 +++++++++++--------
 .../src/nixpkgs_problem.rs                    | 13 ++++
 .../expected                                  |  3 +-
 .../tests/override-no-call-package/expected   |  3 +-
 5 files changed, 111 insertions(+), 53 deletions(-)

diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
index 1322f0faaf91..122c4b734897 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
@@ -51,6 +51,26 @@ struct Location {
     pub column: usize,
 }
 
+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
+        ))
+    }
+}
+
 #[derive(Deserialize)]
 pub enum AttributeVariant {
     /// The attribute is not an attribute set, we're limited in the amount of information we can get
@@ -289,37 +309,47 @@ fn by_name(
                         // At this point, we completed two different checks for whether it's a
                         // `callPackage`
                         match (is_semantic_call_package, optional_syntactic_call_package) {
-                            // Something like `<attr> = { ... }`
-                            (false, None)
+                            // Something like `<attr> = foo`
+                            (_, Err(definition)) => NixpkgsProblem::NonSyntacticCallPackage {
+                                package_name: attribute_name.to_owned(),
+                                location: location.to_relative_string(nixpkgs_path)?,
+                                definition,
+                            }
+                            .into(),
                             // Something like `<attr> = pythonPackages.callPackage ...`
-                            | (false, Some(_))
-                            // Something like `<attr> = bar` where `bar = pkgs.callPackage ...`
-                            | (true, None) => {
+                            (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()
+                                    relative_package_file: relative_package_file.to_owned(),
+                                    package_name: attribute_name.to_owned(),
+                                }
+                                .into()
                             }
                             // Something like `<attr> = pkgs.callPackage ...`
-                            (true, Some(syntactic_call_package)) => {
+                            (true, Ok(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
                                         // anymore
-                                        Success(if syntactic_call_package.empty_arg { Loose(()) } else { Tight })
+                                        Success(if syntactic_call_package.empty_arg {
+                                            Loose(())
+                                        } else {
+                                            Tight
+                                        })
                                     } else {
                                         NixpkgsProblem::WrongCallPackage {
-                                              relative_package_file: relative_package_file.to_owned(),
-                                              package_name: attribute_name.to_owned(),
-                                        }.into()
+                                            relative_package_file: relative_package_file.to_owned(),
+                                            package_name: attribute_name.to_owned(),
+                                        }
+                                        .into()
                                     }
                                 } else {
                                     NixpkgsProblem::WrongCallPackage {
-                                          relative_package_file: relative_package_file.to_owned(),
-                                          package_name: attribute_name.to_owned(),
-                                    }.into()
+                                        relative_package_file: relative_package_file.to_owned(),
+                                        package_name: attribute_name.to_owned(),
+                                    }
+                                    .into()
                                 }
                             }
                         }
@@ -423,16 +453,16 @@ fn handle_non_by_name_attribute(
         // `callPackage`
         match (is_semantic_call_package, optional_syntactic_call_package) {
             // Something like `<attr> = { }`
-            (false, None)
+            (false, Err(_))
             // Something like `<attr> = pythonPackages.callPackage ...`
-            | (false, Some(_))
+            | (false, Ok(_))
             // Something like `<attr> = bar` where `bar = pkgs.callPackage ...`
-            | (true, None) => {
+            | (true, Err(_)) => {
                 // In all of these cases, it's not possible to migrate the package to `pkgs/by-name`
                 NonApplicable
             }
             // Something like `<attr> = pkgs.callPackage ...`
-            (true, Some(syntactic_call_package)) => {
+            (true, Ok(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 ea0833792f3c..3840f1dc479d 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs
@@ -86,8 +86,9 @@ pub struct CallPackageArgumentInfo {
 impl NixFile {
     /// Returns information about callPackage arguments for an attribute at a specific line/column
     /// index.
-    /// If the location is not of the form `<attr> = callPackage <arg1> <arg2>;`, `None` is
-    /// returned.
+    /// If the location is not of the form `<attr> = callPackage <arg1> <arg2>;`, `Ok(Err(String))` is
+    /// returned, with String being how the definition looks like.
+    ///
     /// This function only returns `Err` for problems that can't be caused by the Nix contents,
     /// but rather problems in this programs code itself.
     ///
@@ -108,7 +109,7 @@ impl NixFile {
     ///
     /// You'll get back
     /// ```rust
-    /// Some(CallPackageArgumentInfo { path = Some("default.nix"), empty_arg: true })
+    /// Ok(Ok(CallPackageArgumentInfo { path = Some("default.nix"), empty_arg: true }))
     /// ```
     ///
     /// Note that this also returns the same for `pythonPackages.callPackage`. It doesn't make an
@@ -118,11 +119,15 @@ impl NixFile {
         line: usize,
         column: usize,
         relative_to: &Path,
-    ) -> anyhow::Result<Option<CallPackageArgumentInfo>> {
-        let Some(attrpath_value) = self.attrpath_value_at(line, column)? else {
-            return Ok(None);
-        };
-        self.attrpath_value_call_package_argument_info(attrpath_value, relative_to)
+    ) -> anyhow::Result<Result<CallPackageArgumentInfo, String>> {
+        Ok(match self.attrpath_value_at(line, column)? {
+            Err(definition) => Err(definition),
+            Ok(attrpath_value) => {
+                let definition = attrpath_value.to_string();
+                self.attrpath_value_call_package_argument_info(attrpath_value, relative_to)?
+                    .ok_or(definition)
+            }
+        })
     }
 
     // Internal function mainly to make it independently testable
@@ -130,7 +135,7 @@ impl NixFile {
         &self,
         line: usize,
         column: usize,
-    ) -> anyhow::Result<Option<ast::AttrpathValue>> {
+    ) -> anyhow::Result<Result<ast::AttrpathValue, String>> {
         let index = self.line_index.fromlinecolumn(line, column);
 
         let token_at_offset = self
@@ -164,7 +169,7 @@ impl NixFile {
             // This is the only other way how `builtins.unsafeGetAttrPos` can return
             // attribute positions, but we only look for ones like `<attr-path> = <value>`, so
             // ignore this
-            return Ok(None);
+            return Ok(Err(node.to_string()));
         } else {
             // However, anything else is not expected and smells like a bug
             anyhow::bail!(
@@ -208,7 +213,7 @@ impl NixFile {
         // 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()))
+        Ok(Ok(ast::AttrpathValue::cast(attrpath_value_node).unwrap()))
     }
 
     // Internal function mainly to make attrpath_value_at independently testable
@@ -437,14 +442,14 @@ mod tests {
 
         // These are builtins.unsafeGetAttrPos locations for the attributes
         let cases = [
-            (2, 3, Some("foo = 1;")),
-            (3, 3, Some(r#""bar" = 2;"#)),
-            (4, 3, Some(r#"${"baz"} = 3;"#)),
-            (5, 3, Some(r#""${"qux"}" = 4;"#)),
-            (8, 3, Some("quux\n  # B\n  =\n  # C\n  5\n  # D\n  ;")),
-            (17, 7, Some("quuux/**/=/**/5/**/;")),
-            (19, 10, None),
-            (20, 22, None),
+            (2, 3, Ok("foo = 1;")),
+            (3, 3, Ok(r#""bar" = 2;"#)),
+            (4, 3, Ok(r#"${"baz"} = 3;"#)),
+            (5, 3, Ok(r#""${"qux"}" = 4;"#)),
+            (8, 3, Ok("quux\n  # B\n  =\n  # C\n  5\n  # D\n  ;")),
+            (17, 7, Ok("quuux/**/=/**/5/**/;")),
+            (19, 10, Err("inherit toInherit;")),
+            (20, 22, Err("inherit (toInherit) toInherit;")),
         ];
 
         for (line, column, expected_result) in cases {
@@ -452,7 +457,13 @@ mod tests {
                 .attrpath_value_at(line, column)
                 .context(format!("line {line}, column {column}"))?
                 .map(|node| node.to_string());
-            assert_eq!(actual_result.as_deref(), expected_result);
+            let owned_expected_result = expected_result
+                .map(|x| x.to_string())
+                .map_err(|x| x.to_string());
+            assert_eq!(
+                actual_result, owned_expected_result,
+                "line {line}, column {column}"
+            );
         }
 
         Ok(())
@@ -481,41 +492,41 @@ mod tests {
         let nix_file = NixFile::new(&file)?;
 
         let cases = [
-            (2, None),
-            (3, None),
-            (4, None),
-            (5, None),
+            (2, Err("a.sub = null;")),
+            (3, Err("b = null;")),
+            (4, Err("c = import ./file.nix;")),
+            (5, Err("d = import ./file.nix { };")),
             (
                 6,
-                Some(CallPackageArgumentInfo {
+                Ok(CallPackageArgumentInfo {
                     relative_path: Some(PathBuf::from("file.nix")),
                     empty_arg: true,
                 }),
             ),
             (
                 7,
-                Some(CallPackageArgumentInfo {
+                Ok(CallPackageArgumentInfo {
                     relative_path: Some(PathBuf::from("file.nix")),
                     empty_arg: true,
                 }),
             ),
             (
                 8,
-                Some(CallPackageArgumentInfo {
+                Ok(CallPackageArgumentInfo {
                     relative_path: None,
                     empty_arg: true,
                 }),
             ),
             (
                 9,
-                Some(CallPackageArgumentInfo {
+                Ok(CallPackageArgumentInfo {
                     relative_path: Some(PathBuf::from("file.nix")),
                     empty_arg: false,
                 }),
             ),
             (
                 10,
-                Some(CallPackageArgumentInfo {
+                Ok(CallPackageArgumentInfo {
                     relative_path: None,
                     empty_arg: false,
                 }),
@@ -525,8 +536,10 @@ mod tests {
         for (line, expected_result) in cases {
             let actual_result = nix_file
                 .call_package_argument_info_at(line, 3, temp_dir.path())
-                .context(format!("line {line}"))?;
-            assert_eq!(actual_result, expected_result);
+                .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}");
         }
 
         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 e13869adaa41..18001eb44ecd 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
@@ -44,6 +44,11 @@ pub enum NixpkgsProblem {
         relative_package_file: PathBuf,
         package_name: String,
     },
+    NonSyntacticCallPackage {
+        package_name: String,
+        location: String,
+        definition: String,
+    },
     NonDerivation {
         relative_package_file: PathBuf,
         package_name: String,
@@ -164,6 +169,14 @@ 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 } =>
+                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{}",
+                    structure::relative_dir_for_package(package_name).display(),
+                    structure::relative_file_for_package(package_name).display(),
+                    definition,
+                ),
             NixpkgsProblem::NonDerivation { relative_package_file, package_name } =>
                 write!(
                     f,
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected
index 9df788191ece..f590ef4ff9fa 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected
@@ -1 +1,2 @@
-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 as `callPackage pkgs/by-name/fo/foo/package.nix { ... }`. This is however not the case: The attribute is defined in all-packages.nix:4:3 as
+	foo = self.bar;
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected
index 51479e22d26f..25fc867b04fe 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected
@@ -1 +1,2 @@
-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 as `callPackage pkgs/by-name/no/nonDerivation/package.nix { ... }`. This is however not the case: The attribute is defined in all-packages.nix:2:3 as
+	nonDerivation = self.someDrv;

From a53b07e2523978518efc6ae161ef9ffba7933d6e Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Thu, 22 Feb 2024 23:04:40 +0100
Subject: [PATCH 04/18] 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<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 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<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 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 { /* ... */ }

From 7258d472bcd8886be8b1463bea6030d1afe93bbf Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Thu, 22 Feb 2024 23:43:02 +0100
Subject: [PATCH 05/18] tests.nixpkgs-check-by-name: Improve non-syntactic
 callPackage error more

---
 pkgs/test/nixpkgs-check-by-name/Cargo.lock    | 30 +++++++++++++++
 pkgs/test/nixpkgs-check-by-name/Cargo.toml    |  1 +
 .../src/nixpkgs_problem.rs                    | 38 +++++++++++++++----
 .../expected                                  | 10 ++++-
 .../tests/override-different-file/expected    |  4 +-
 .../tests/override-no-call-package/expected   | 10 ++++-
 6 files changed, 79 insertions(+), 14 deletions(-)

diff --git a/pkgs/test/nixpkgs-check-by-name/Cargo.lock b/pkgs/test/nixpkgs-check-by-name/Cargo.lock
index b02f7075ab61..19435c2ab76e 100644
--- a/pkgs/test/nixpkgs-check-by-name/Cargo.lock
+++ b/pkgs/test/nixpkgs-check-by-name/Cargo.lock
@@ -306,6 +306,7 @@ dependencies = [
  "serde_json",
  "temp-env",
  "tempfile",
+ "textwrap",
 ]
 
 [[package]]
@@ -489,6 +490,12 @@ version = "1.11.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "942b4a808e05215192e39f4ab80813e599068285906cc91aa64f923db842bd5a"
 
+[[package]]
+name = "smawk"
+version = "0.3.2"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "b7c388c1b5e93756d0c740965c41e8822f866621d41acbdf6336a6a168f8840c"
+
 [[package]]
 name = "strsim"
 version = "0.10.0"
@@ -534,12 +541,35 @@ version = "1.1.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "f18aa187839b2bdb1ad2fa35ead8c4c2976b64e4363c386d45ac0f7ee85c9233"
 
+[[package]]
+name = "textwrap"
+version = "0.16.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "23d434d3f8967a09480fb04132ebe0a3e088c173e6d0ee7897abbdf4eab0f8b9"
+dependencies = [
+ "smawk",
+ "unicode-linebreak",
+ "unicode-width",
+]
+
 [[package]]
 name = "unicode-ident"
 version = "1.0.11"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "301abaae475aa91687eb82514b328ab47a211a533026cb25fc3e519b86adfc3c"
 
+[[package]]
+name = "unicode-linebreak"
+version = "0.1.5"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "3b09c83c3c29d37506a3e260c08c03743a6bb66a9cd432c6934ab501a190571f"
+
+[[package]]
+name = "unicode-width"
+version = "0.1.11"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "e51733f11c9c4f72aa0c160008246859e340b00807569a0da0e7a1079b27ba85"
+
 [[package]]
 name = "utf8parse"
 version = "0.2.1"
diff --git a/pkgs/test/nixpkgs-check-by-name/Cargo.toml b/pkgs/test/nixpkgs-check-by-name/Cargo.toml
index 1b8f8e9085d4..50cdabb7e2dd 100644
--- a/pkgs/test/nixpkgs-check-by-name/Cargo.toml
+++ b/pkgs/test/nixpkgs-check-by-name/Cargo.toml
@@ -17,6 +17,7 @@ itertools = "0.11.0"
 rowan = "0.15.11"
 indoc = "2.0.4"
 relative-path = "1.9.2"
+textwrap = "0.16.1"
 
 [dev-dependencies]
 temp-env = "0.3.5"
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 dda89895697f..d8ae2db4dc22 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
@@ -188,28 +188,50 @@ impl fmt::Display for NixpkgsProblem {
                     "
                     - Because {} exists, the attribute `pkgs.{package_name}` must be defined like
 
-                        {package_name} = callPackage {} {{ /* ... */ }}
+                        {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 {} {{ /* ... */ }}",
+                        {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!(
+            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,
-                    "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{}",
+                    "
+                    - Because {} exists, the attribute `pkgs.{package_name}` must be defined like
+
+                        {package_name} = callPackage {} {{ /* ... */ }};
+
+                      This is however not the case.
+                      It is defined in {}:{} as
+
+                    {}",
                     structure::relative_dir_for_package(package_name).display(),
                     structure::relative_file_for_package(package_name).display(),
                     file.display(),
                     line,
-                    column,
-                    definition,
-                ),
+                    definition_indented,
+                )
+            }
             NixpkgsProblem::NonDerivation { relative_package_file, package_name } =>
                 write!(
                     f,
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected
index f590ef4ff9fa..e62009633885 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected
@@ -1,2 +1,8 @@
-Because pkgs/by-name/fo/foo exists, the attribute `pkgs.foo` must be defined as `callPackage pkgs/by-name/fo/foo/package.nix { ... }`. This is however not the case: The attribute is defined in all-packages.nix:4:3 as
-	foo = self.bar;
+- 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.
+  It is defined in all-packages.nix:4 as
+
+    foo = self.bar;
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 3566b15bdbc4..18bf137364fe 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,8 +1,8 @@
 - Because pkgs/by-name/no/nonDerivation exists, the attribute `pkgs.nonDerivation` must be defined like
 
-    nonDerivation = callPackage ./pkgs/by-name/no/nonDerivation/package.nix { /* ... */ }
+    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 { /* ... */ }
+    nonDerivation = callPackage ./someDrv.nix { /* ... */ };
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected
index 25fc867b04fe..e31f9eb6d539 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected
@@ -1,2 +1,8 @@
-Because pkgs/by-name/no/nonDerivation exists, the attribute `pkgs.nonDerivation` must be defined as `callPackage pkgs/by-name/no/nonDerivation/package.nix { ... }`. This is however not the case: The attribute is defined in all-packages.nix:2:3 as
-	nonDerivation = self.someDrv;
+- 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.
+  It is defined in all-packages.nix:2 as
+
+    nonDerivation = self.someDrv;

From 75cdccd6c4753efe4fa543a05e4d1741cfb63218 Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Fri, 23 Feb 2024 00:10:20 +0100
Subject: [PATCH 06/18] tests.nixpkgs-check-by-name: Improve more errors

---
 pkgs/test/nixpkgs-check-by-name/src/eval.rs   | 38 ++++-----
 .../nixpkgs-check-by-name/src/nix_file.rs     | 35 ++++----
 .../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 <arg1> <arg2>`,
                         // 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 `<attr> = 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 `<attr> = 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 `<attr> = 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 <arg1> <arg2>`,
         // 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 `<attr> = { }`
-            (false, Err(_))
+            (false, None)
             // Something like `<attr> = pythonPackages.callPackage ...`
-            | (false, Ok(_))
+            | (false, Some(_))
             // Something like `<attr> = 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 `<attr> = 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<Result<CallPackageArgumentInfo, String>> {
+    ) -> anyhow::Result<(Option<CallPackageArgumentInfo>, 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<Path>, to_file: impl AsRef<Path>) -> 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 <test-nixpkgs> { 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; };

From db7562e886d07ab365e8c55c50cf93dd709121ab Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Fri, 23 Feb 2024 01:01:57 +0100
Subject: [PATCH 07/18] tests.nixpkgs-check-by-name: Improve errors relating to
 the base branch

---
 pkgs/test/nixpkgs-check-by-name/src/main.rs   | 94 ++++++++++++-------
 .../tests/aliases/expected                    |  1 +
 .../tests/alt-callPackage/expected            |  1 +
 .../tests/base-fixed/base/default.nix         |  1 +
 .../tests/base-fixed/base/pkgs/by-name/foo    |  0
 .../tests/base-fixed/default.nix              |  1 +
 .../tests/base-fixed/expected                 |  1 +
 .../tests/base-fixed/pkgs/by-name/README.md   |  0
 .../tests/base-still-broken/base/default.nix  |  1 +
 .../base-still-broken/base/pkgs/by-name/foo   |  0
 .../tests/base-still-broken/default.nix       |  1 +
 .../tests/base-still-broken/expected          |  3 +
 .../tests/base-still-broken/pkgs/by-name/bar  |  0
 .../tests/broken-autocall/expected            |  1 +
 .../tests/callPackage-syntax/expected         |  1 +
 .../tests/empty-base/expected                 |  1 +
 .../tests/incorrect-shard/expected            |  1 +
 .../tests/internalCallPackage/expected        |  1 +
 .../tests/invalid-package-name/expected       |  1 +
 .../tests/invalid-shard-name/expected         |  1 +
 .../tests/manual-definition/expected          |  1 +
 .../tests/missing-package-nix/expected        |  1 +
 .../tests/move-to-non-by-name/expected        |  1 +
 .../tests/multiple-failures/expected          |  1 +
 .../tests/new-package-non-by-name/expected    |  1 +
 .../tests/no-by-name/expected                 |  2 +-
 .../tests/no-eval/expected                    |  1 +
 .../tests/non-attrs/expected                  |  1 +
 .../tests/non-derivation/expected             |  1 +
 .../expected                                  |  1 +
 .../tests/one-letter/expected                 |  1 +
 .../only-callPackage-derivations/expected     |  1 +
 .../tests/override-different-file/expected    |  1 +
 .../tests/override-empty-arg-gradual/expected |  1 +
 .../tests/override-empty-arg/expected         |  1 +
 .../tests/override-no-call-package/expected   |  1 +
 .../tests/override-no-file/expected           |  1 +
 .../tests/override-non-path/expected          |  1 +
 .../tests/override-success/expected           |  1 +
 .../tests/package-dir-is-file/expected        |  1 +
 .../tests/package-nix-dir/expected            |  1 +
 .../tests/package-variants/expected           |  1 +
 .../tests/ref-absolute/expected               |  1 +
 .../tests/ref-escape/expected                 |  1 +
 .../tests/ref-nix-path/expected               |  1 +
 .../tests/ref-path-subexpr/expected           |  1 +
 .../tests/ref-success/expected                |  1 +
 .../tests/shard-file/expected                 |  1 +
 .../tests/sorted-order/expected               |  1 +
 .../tests/success/expected                    |  1 +
 .../tests/symlink-escape/expected             |  1 +
 .../tests/symlink-invalid/expected            |  1 +
 .../tests/unknown-location/expected           |  1 +
 .../tests/uppercase/expected                  |  1 +
 .../tests/with-readme/expected                |  1 +
 55 files changed, 110 insertions(+), 37 deletions(-)
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/aliases/expected
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/base-fixed/base/default.nix
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/base-fixed/base/pkgs/by-name/foo
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/base-fixed/default.nix
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/base-fixed/expected
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/base-fixed/pkgs/by-name/README.md
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/base/default.nix
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/base/pkgs/by-name/foo
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/default.nix
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/expected
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/pkgs/by-name/bar
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/expected
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/empty-base/expected
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/no-eval/expected
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/one-letter/expected
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/only-callPackage-derivations/expected
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/override-success/expected
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/package-variants/expected
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/ref-success/expected
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/success/expected
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/uppercase/expected
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/with-readme/expected

diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs
index 24415424914e..d5ea2c67438b 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/main.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs
@@ -47,14 +47,8 @@ pub struct Args {
 fn main() -> ExitCode {
     let args = Args::parse();
     match process(&args.base, &args.nixpkgs, false, &mut io::stderr()) {
-        Ok(true) => {
-            eprintln!("{}", "Validated successfully".green());
-            ExitCode::SUCCESS
-        }
-        Ok(false) => {
-            eprintln!("{}", "Validation failed, see above errors".yellow());
-            ExitCode::from(1)
-        }
+        Ok(true) => ExitCode::SUCCESS,
+        Ok(false) => ExitCode::from(1),
         Err(e) => {
             eprintln!("{} {:#}", "I/O error: ".yellow(), e);
             ExitCode::from(2)
@@ -82,29 +76,58 @@ pub fn process<W: io::Write>(
     keep_nix_path: bool,
     error_writer: &mut W,
 ) -> anyhow::Result<bool> {
-    // Check the main Nixpkgs first
-    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, keep_nix_path, error_writer)?.result_map(
-            |base_nixpkgs_version| {
-                Ok(ratchet::Nixpkgs::compare(
-                    base_nixpkgs_version,
-                    nixpkgs_version,
-                ))
-            },
-        )
-    })?;
 
-    match check_result {
-        Failure(errors) => {
+    // TODO: Run in parallel
+    let base_result = check_nixpkgs(base_nixpkgs, keep_nix_path)?;
+    let main_result = check_nixpkgs(main_nixpkgs, keep_nix_path)?;
+
+    match (base_result, main_result) {
+        (Failure(_), Failure(errors)) => {
+            // Base branch fails and the PR doesn't fix it and may also introduce additional problems
             for error in errors {
                 writeln!(error_writer, "{}", error.to_string().red())?
             }
+            writeln!(error_writer, "{}", "The base branch is broken and still has above problems with this PR, these need to be fixed first.\nConsider reverting the PR that introduced these problems in order to prevent more failures of unrelated PRs.".yellow())?;
             Ok(false)
         }
-        Success(()) => Ok(true),
+        (Failure(_), Success(_)) => {
+            // Base branch fails, but the PR fixes it
+            writeln!(
+                error_writer,
+                "{}",
+                "The base branch was broken, but this PR fixes it, nice job!".green()
+            )?;
+            Ok(true)
+        }
+        (Success(_), Failure(errors)) => {
+            // Base branch succeeds, the PR breaks it
+            for error in errors {
+                writeln!(error_writer, "{}", error.to_string().red())?
+            }
+            writeln!(
+                error_writer,
+                "{}",
+                "This PR introduces the above problems, merging would break the base branch"
+                    .yellow()
+            )?;
+            Ok(false)
+        }
+        (Success(base), Success(main)) => {
+            // Both base and main branch succeed, check ratchet state
+            match ratchet::Nixpkgs::compare(base, main) {
+                Failure(errors) => {
+                    for error in errors {
+                        writeln!(error_writer, "{}", error.to_string().red())?
+                    }
+                    writeln!(error_writer, "{}", "This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch".yellow())?;
+                    Ok(false)
+                }
+                Success(()) => {
+                    writeln!(error_writer, "{}", "Validated successfully".green())?;
+                    Ok(true)
+                }
+            }
+        }
     }
 }
 
@@ -113,10 +136,9 @@ pub fn process<W: io::Write>(
 /// This does not include ratchet checks, see ../README.md#ratchet-checks
 /// Instead a `ratchet::Nixpkgs` value is returned, whose `compare` method allows performing the
 /// ratchet check against another result.
-pub fn check_nixpkgs<W: io::Write>(
+pub fn check_nixpkgs(
     nixpkgs_path: &Path,
     keep_nix_path: bool,
-    error_writer: &mut W,
 ) -> validation::Result<ratchet::Nixpkgs> {
     let mut nix_file_store = NixFileStore::default();
 
@@ -129,11 +151,7 @@ pub fn check_nixpkgs<W: io::Write>(
         })?;
 
         if !nixpkgs_path.join(utils::BASE_SUBPATH).exists() {
-            writeln!(
-                error_writer,
-                "Given Nixpkgs path does not contain a {} subdirectory, no check necessary.",
-                utils::BASE_SUBPATH
-            )?;
+            // No pkgs/by-name directory, always valid
             Success(ratchet::Nixpkgs::default())
         } else {
             check_structure(&nixpkgs_path, &mut nix_file_store)?.result_map(|package_names|
@@ -163,8 +181,8 @@ mod tests {
                 continue;
             }
 
-            let expected_errors =
-                fs::read_to_string(path.join("expected")).unwrap_or(String::new());
+            let expected_errors = fs::read_to_string(path.join("expected"))
+                .expect("No expected file for test {name}");
 
             test_nixpkgs(&name, &path, &expected_errors)?;
         }
@@ -201,7 +219,7 @@ mod tests {
         test_nixpkgs(
             "case_sensitive",
             &path,
-            "pkgs/by-name/fo: Duplicate case-sensitive package directories \"foO\" and \"foo\".\n",
+            "pkgs/by-name/fo: Duplicate case-sensitive package directories \"foO\" and \"foo\".\nThis PR introduces the above problems, merging would break the base branch\n",
         )?;
 
         Ok(())
@@ -225,7 +243,11 @@ mod tests {
         let tmpdir = temp_root.path().join("symlinked");
 
         temp_env::with_var("TMPDIR", Some(&tmpdir), || {
-            test_nixpkgs("symlinked_tmpdir", Path::new("tests/success"), "")
+            test_nixpkgs(
+                "symlinked_tmpdir",
+                Path::new("tests/success"),
+                "Validated successfully\n",
+            )
         })
     }
 
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/aliases/expected b/pkgs/test/nixpkgs-check-by-name/tests/aliases/expected
new file mode 100644
index 000000000000..defae2634c34
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/aliases/expected
@@ -0,0 +1 @@
+Validated successfully
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected
index 25d2e6c4fa6a..3abb613de096 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected
@@ -6,3 +6,4 @@
   It is defined in all-packages.nix:5 as
 
     foo = self.alt.callPackage ({ }: self.someDrv) { };
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/base/default.nix b/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/base/default.nix
new file mode 100644
index 000000000000..861260cdca4b
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/base/default.nix
@@ -0,0 +1 @@
+import <test-nixpkgs> { root = ./.; }
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/base/pkgs/by-name/foo b/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/base/pkgs/by-name/foo
new file mode 100644
index 000000000000..e69de29bb2d1
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/default.nix b/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/default.nix
new file mode 100644
index 000000000000..861260cdca4b
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/default.nix
@@ -0,0 +1 @@
+import <test-nixpkgs> { root = ./.; }
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/expected b/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/expected
new file mode 100644
index 000000000000..feb6cce2fd04
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/expected
@@ -0,0 +1 @@
+The base branch was broken, but this PR fixes it, nice job!
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/pkgs/by-name/README.md b/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/pkgs/by-name/README.md
new file mode 100644
index 000000000000..e69de29bb2d1
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/base/default.nix b/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/base/default.nix
new file mode 100644
index 000000000000..861260cdca4b
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/base/default.nix
@@ -0,0 +1 @@
+import <test-nixpkgs> { root = ./.; }
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/base/pkgs/by-name/foo b/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/base/pkgs/by-name/foo
new file mode 100644
index 000000000000..e69de29bb2d1
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/default.nix b/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/default.nix
new file mode 100644
index 000000000000..861260cdca4b
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/default.nix
@@ -0,0 +1 @@
+import <test-nixpkgs> { root = ./.; }
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/expected b/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/expected
new file mode 100644
index 000000000000..4f0d08357d1f
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/expected
@@ -0,0 +1,3 @@
+pkgs/by-name/bar: This is a file, but it should be a directory.
+The base branch is broken and still has above problems with this PR, these need to be fixed first.
+Consider reverting the PR that introduced these problems in order to prevent more failures of unrelated PRs.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/pkgs/by-name/bar b/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/pkgs/by-name/bar
new file mode 100644
index 000000000000..e69de29bb2d1
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/broken-autocall/expected b/pkgs/test/nixpkgs-check-by-name/tests/broken-autocall/expected
index fff17c6c7cd5..6b937106b806 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/broken-autocall/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/broken-autocall/expected
@@ -1 +1,2 @@
 pkgs.foo: This attribute is not defined but it should be defined automatically as pkgs/by-name/fo/foo/package.nix
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/expected b/pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/expected
new file mode 100644
index 000000000000..defae2634c34
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/expected
@@ -0,0 +1 @@
+Validated successfully
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/empty-base/expected b/pkgs/test/nixpkgs-check-by-name/tests/empty-base/expected
new file mode 100644
index 000000000000..defae2634c34
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/empty-base/expected
@@ -0,0 +1 @@
+Validated successfully
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/incorrect-shard/expected b/pkgs/test/nixpkgs-check-by-name/tests/incorrect-shard/expected
index 3627368c0ef0..ec1918cace8c 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/incorrect-shard/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/incorrect-shard/expected
@@ -1 +1,2 @@
 pkgs/by-name/aa/FOO: Incorrect directory location, should be pkgs/by-name/fo/FOO instead.
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/internalCallPackage/expected b/pkgs/test/nixpkgs-check-by-name/tests/internalCallPackage/expected
index 404795ee5c79..9fef30d14f37 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/internalCallPackage/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/internalCallPackage/expected
@@ -1 +1,2 @@
 pkgs.foo: This attribute is defined using `_internalCallByNamePackageFile`, which is an internal function not intended for manual use.
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/invalid-package-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/invalid-package-name/expected
index 8c8eafdcb3d1..aac1b186ca49 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/invalid-package-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/invalid-package-name/expected
@@ -1 +1,2 @@
 pkgs/by-name/fo/fo@: Invalid package directory name "fo@", must be ASCII characters consisting of a-z, A-Z, 0-9, "-" or "_".
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/invalid-shard-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/invalid-shard-name/expected
index 248aa8877966..ce7c2695424c 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/invalid-shard-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/invalid-shard-name/expected
@@ -1 +1,2 @@
 pkgs/by-name/A: Invalid directory name "A", must be at most 2 ASCII characters consisting of a-z, 0-9, "-" or "_".
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected b/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
index 29d33f7dbdc0..1cc970b7fd72 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
@@ -1,2 +1,3 @@
 pkgs.noEval: 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/noEval/package.nix { ... }` with a non-empty second argument.
 pkgs.onlyMove: 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/on/onlyMove/package.nix { ... }` with a non-empty second argument.
+This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/missing-package-nix/expected b/pkgs/test/nixpkgs-check-by-name/tests/missing-package-nix/expected
index ce1afcbf2d34..93c882b2bd3c 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/missing-package-nix/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/missing-package-nix/expected
@@ -1 +1,2 @@
 pkgs/by-name/fo/foo: Missing required "package.nix" file.
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected
index 96da50b52491..8a2116aebcb9 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected
@@ -2,3 +2,4 @@ pkgs.foo1: This top-level package was previously defined in pkgs/by-name/fo/foo1
 pkgs.foo2: This top-level package was previously defined in pkgs/by-name/fo/foo2/package.nix, but is now manually defined as `callPackage ./without-config.nix { }` (e.g. in `pkgs/top-level/all-packages.nix`). Please move the package back and remove the manual `callPackage`.
 pkgs.foo3: This top-level package was previously defined in pkgs/by-name/fo/foo3/package.nix, but is now manually defined as `callPackage ... { ... }` (e.g. in `pkgs/top-level/all-packages.nix`). While the manual `callPackage` is still needed, it's not necessary to move the package files.
 pkgs.foo4: This top-level package was previously defined in pkgs/by-name/fo/foo4/package.nix, but is now manually defined as `callPackage ./with-config.nix { ... }` (e.g. in `pkgs/top-level/all-packages.nix`). While the manual `callPackage` is still needed, it's not necessary to move the package files.
+This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/multiple-failures/expected b/pkgs/test/nixpkgs-check-by-name/tests/multiple-failures/expected
index ff5d18556ef0..303884cb7944 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/multiple-failures/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/multiple-failures/expected
@@ -11,3 +11,4 @@ pkgs/by-name/ba/foo: File package.nix at line 3 contains the path expression "..
 pkgs/by-name/ba/foo: File package.nix at line 4 contains the nix search path expression "<nixpkgs>" which may point outside the directory of that package.
 pkgs/by-name/ba/foo: File package.nix at line 5 contains the path expression "./${"test"}", which is not yet supported and may point outside the directory of that package.
 pkgs/by-name/fo/foo: Missing required "package.nix" file.
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected
index 3f294f26dfd8..31eff28f3c88 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected
@@ -2,3 +2,4 @@ pkgs.new1: This is a new top-level package of the form `callPackage ... { }`. Pl
 pkgs.new2: This is a new top-level package of the form `callPackage ./without-config.nix { }`. Please define it in pkgs/by-name/ne/new2/package.nix instead. See `pkgs/by-name/README.md` for more details. Since the second `callPackage` argument is `{ }`, no manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is needed anymore.
 pkgs.new3: This is a new top-level package of the form `callPackage ... { }`. Please define it in pkgs/by-name/ne/new3/package.nix instead. See `pkgs/by-name/README.md` for more details. Since the second `callPackage` argument is not `{ }`, the manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is still needed.
 pkgs.new4: This is a new top-level package of the form `callPackage ./with-config.nix { }`. Please define it in pkgs/by-name/ne/new4/package.nix instead. See `pkgs/by-name/README.md` for more details. Since the second `callPackage` argument is not `{ }`, the manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is still needed.
+This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/no-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/no-by-name/expected
index ddcb2df46e5f..defae2634c34 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/no-by-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/no-by-name/expected
@@ -1 +1 @@
-Given Nixpkgs path does not contain a pkgs/by-name subdirectory, no check necessary.
+Validated successfully
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/no-eval/expected b/pkgs/test/nixpkgs-check-by-name/tests/no-eval/expected
new file mode 100644
index 000000000000..defae2634c34
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/no-eval/expected
@@ -0,0 +1 @@
+Validated successfully
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/non-attrs/expected b/pkgs/test/nixpkgs-check-by-name/tests/non-attrs/expected
index e6c923790102..48acbec1210d 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/non-attrs/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/non-attrs/expected
@@ -1 +1,2 @@
 pkgs.nonDerivation: This attribute defined by pkgs/by-name/no/nonDerivation/package.nix is not a derivation
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/non-derivation/expected b/pkgs/test/nixpkgs-check-by-name/tests/non-derivation/expected
index e6c923790102..48acbec1210d 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/non-derivation/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/non-derivation/expected
@@ -1 +1,2 @@
 pkgs.nonDerivation: This attribute defined by pkgs/by-name/no/nonDerivation/package.nix is not a derivation
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected
index e62009633885..d1a31336504a 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected
@@ -6,3 +6,4 @@
   It is defined in all-packages.nix:4 as
 
     foo = self.bar;
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/one-letter/expected b/pkgs/test/nixpkgs-check-by-name/tests/one-letter/expected
new file mode 100644
index 000000000000..defae2634c34
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/one-letter/expected
@@ -0,0 +1 @@
+Validated successfully
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/only-callPackage-derivations/expected b/pkgs/test/nixpkgs-check-by-name/tests/only-callPackage-derivations/expected
new file mode 100644
index 000000000000..defae2634c34
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/only-callPackage-derivations/expected
@@ -0,0 +1 @@
+Validated successfully
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 18bf137364fe..6f439dd4b7d0 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
@@ -6,3 +6,4 @@
   It is defined in all-packages.nix:2:3 as
 
     nonDerivation = callPackage ./someDrv.nix { /* ... */ };
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/expected
index e69de29bb2d1..defae2634c34 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/expected
@@ -0,0 +1 @@
+Validated successfully
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected
index 51479e22d26f..bf96b181d7fe 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected
@@ -1 +1,2 @@
 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.
+This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected
index e31f9eb6d539..54d1eb51be34 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected
@@ -6,3 +6,4 @@
   It is defined in all-packages.nix:2 as
 
     nonDerivation = self.someDrv;
+This PR introduces the above problems, merging would break the base branch
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 0f8a4cb65eef..a1980fdc08da 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
@@ -6,3 +6,4 @@
   It is defined in all-packages.nix:2 as
 
     nonDerivation = self.callPackage ({ someDrv }: someDrv) { };
+This PR introduces the above problems, merging would break the base branch
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 41e40a9b3c3f..c8cb5be6d8ee 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
@@ -6,3 +6,4 @@
   It is defined in all-packages.nix:3 as
 
     foo = self.callPackage ({ someDrv, someFlag }: someDrv) { someFlag = true; };
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-success/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-success/expected
new file mode 100644
index 000000000000..defae2634c34
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-success/expected
@@ -0,0 +1 @@
+Validated successfully
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/package-dir-is-file/expected b/pkgs/test/nixpkgs-check-by-name/tests/package-dir-is-file/expected
index 3ad4b8f820f5..864978b9b731 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/package-dir-is-file/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/package-dir-is-file/expected
@@ -1 +1,2 @@
 pkgs/by-name/fo/foo: This path is a file, but it should be a directory.
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/package-nix-dir/expected b/pkgs/test/nixpkgs-check-by-name/tests/package-nix-dir/expected
index 67a0c69fe29e..82ad2ce85bbf 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/package-nix-dir/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/package-nix-dir/expected
@@ -1 +1,2 @@
 pkgs/by-name/fo/foo: "package.nix" must be a file.
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/package-variants/expected b/pkgs/test/nixpkgs-check-by-name/tests/package-variants/expected
new file mode 100644
index 000000000000..defae2634c34
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/package-variants/expected
@@ -0,0 +1 @@
+Validated successfully
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/ref-absolute/expected b/pkgs/test/nixpkgs-check-by-name/tests/ref-absolute/expected
index 7d20c32aad68..0e0e876d5561 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/ref-absolute/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/ref-absolute/expected
@@ -1 +1,2 @@
 pkgs/by-name/aa/aa: File package.nix at line 2 contains the path expression "/foo" which cannot be resolved: No such file or directory (os error 2).
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/ref-escape/expected b/pkgs/test/nixpkgs-check-by-name/tests/ref-escape/expected
index 3d7fb64e80a3..f414e4145674 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/ref-escape/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/ref-escape/expected
@@ -1 +1,2 @@
 pkgs/by-name/aa/aa: File package.nix at line 2 contains the path expression "../." which may point outside the directory of that package.
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/ref-nix-path/expected b/pkgs/test/nixpkgs-check-by-name/tests/ref-nix-path/expected
index b0cdff4a4778..7b47a786a8be 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/ref-nix-path/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/ref-nix-path/expected
@@ -1 +1,2 @@
 pkgs/by-name/aa/aa: File package.nix at line 2 contains the nix search path expression "<nixpkgs>" which may point outside the directory of that package.
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/ref-path-subexpr/expected b/pkgs/test/nixpkgs-check-by-name/tests/ref-path-subexpr/expected
index ad662af27a86..1905841a63df 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/ref-path-subexpr/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/ref-path-subexpr/expected
@@ -1 +1,2 @@
 pkgs/by-name/aa/aa: File package.nix at line 2 contains the path expression "./${"test"}", which is not yet supported and may point outside the directory of that package.
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/ref-success/expected b/pkgs/test/nixpkgs-check-by-name/tests/ref-success/expected
new file mode 100644
index 000000000000..defae2634c34
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/ref-success/expected
@@ -0,0 +1 @@
+Validated successfully
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/shard-file/expected b/pkgs/test/nixpkgs-check-by-name/tests/shard-file/expected
index 447b38e6b6c1..4a02d99778a9 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/shard-file/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/shard-file/expected
@@ -1 +1,2 @@
 pkgs/by-name/fo: This is a file, but it should be a directory.
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected b/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
index 349e9ad47c41..b835348cc3cf 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
@@ -2,3 +2,4 @@ pkgs.a: This attribute is manually defined (most likely in pkgs/top-level/all-pa
 pkgs.b: This is a new top-level package of the form `callPackage ... { }`. Please define it in pkgs/by-name/b/b/package.nix instead. See `pkgs/by-name/README.md` for more details. Since the second `callPackage` argument is `{ }`, no manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is needed anymore.
 pkgs.c: 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/c/c/package.nix { ... }` with a non-empty second argument.
 pkgs.d: This is a new top-level package of the form `callPackage ... { }`. Please define it in pkgs/by-name/d/d/package.nix instead. See `pkgs/by-name/README.md` for more details. Since the second `callPackage` argument is `{ }`, no manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is needed anymore.
+This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/success/expected b/pkgs/test/nixpkgs-check-by-name/tests/success/expected
new file mode 100644
index 000000000000..defae2634c34
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/success/expected
@@ -0,0 +1 @@
+Validated successfully
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/symlink-escape/expected b/pkgs/test/nixpkgs-check-by-name/tests/symlink-escape/expected
index 335c5d6b6e5d..e6c183207e57 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/symlink-escape/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/symlink-escape/expected
@@ -1 +1,2 @@
 pkgs/by-name/fo/foo: Path package.nix is a symlink pointing to a path outside the directory of that package.
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/symlink-invalid/expected b/pkgs/test/nixpkgs-check-by-name/tests/symlink-invalid/expected
index c1e7a28205a7..01de2702f7d5 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/symlink-invalid/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/symlink-invalid/expected
@@ -1 +1,2 @@
 pkgs/by-name/fo/foo: Path foo is a symlink which cannot be resolved: No such file or directory (os error 2).
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/unknown-location/expected b/pkgs/test/nixpkgs-check-by-name/tests/unknown-location/expected
index 2a248c23ab50..d66a3185e701 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/unknown-location/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/unknown-location/expected
@@ -1 +1,2 @@
 pkgs.foo: Cannot determine the location of this attribute using `builtins.unsafeGetAttrPos`.
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/uppercase/expected b/pkgs/test/nixpkgs-check-by-name/tests/uppercase/expected
new file mode 100644
index 000000000000..defae2634c34
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/uppercase/expected
@@ -0,0 +1 @@
+Validated successfully
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/with-readme/expected b/pkgs/test/nixpkgs-check-by-name/tests/with-readme/expected
new file mode 100644
index 000000000000..defae2634c34
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/with-readme/expected
@@ -0,0 +1 @@
+Validated successfully

From f5e9b88af41e5c11b766fa9846d5f65b68fb2351 Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Fri, 23 Feb 2024 01:15:22 +0100
Subject: [PATCH 08/18] tests.nixpkgs-check-by-name: Evaluate base and main
 branch in parallel

---
 pkgs/test/nixpkgs-check-by-name/src/main.rs | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs
index d5ea2c67438b..c92d0af64969 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/main.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs
@@ -1,4 +1,5 @@
 use crate::nix_file::NixFileStore;
+use std::panic;
 mod eval;
 mod nix_file;
 mod nixpkgs_problem;
@@ -17,6 +18,7 @@ use colored::Colorize;
 use std::io;
 use std::path::{Path, PathBuf};
 use std::process::ExitCode;
+use std::thread;
 
 /// Program to check the validity of pkgs/by-name
 ///
@@ -46,7 +48,7 @@ pub struct Args {
 
 fn main() -> ExitCode {
     let args = Args::parse();
-    match process(&args.base, &args.nixpkgs, false, &mut io::stderr()) {
+    match process(args.base, args.nixpkgs, false, &mut io::stderr()) {
         Ok(true) => ExitCode::SUCCESS,
         Ok(false) => ExitCode::from(1),
         Err(e) => {
@@ -71,15 +73,19 @@ fn main() -> ExitCode {
 /// - `Ok(false)` if there are problems, all of which will be written to `error_writer`.
 /// - `Ok(true)` if there are no problems
 pub fn process<W: io::Write>(
-    base_nixpkgs: &Path,
-    main_nixpkgs: &Path,
+    base_nixpkgs: PathBuf,
+    main_nixpkgs: PathBuf,
     keep_nix_path: bool,
     error_writer: &mut W,
 ) -> anyhow::Result<bool> {
+    // Very easy to parallelise this, since it's totally independent
+    let base_thread = thread::spawn(move || check_nixpkgs(&base_nixpkgs, keep_nix_path));
+    let main_result = check_nixpkgs(&main_nixpkgs, keep_nix_path)?;
 
-    // TODO: Run in parallel
-    let base_result = check_nixpkgs(base_nixpkgs, keep_nix_path)?;
-    let main_result = check_nixpkgs(main_nixpkgs, keep_nix_path)?;
+    let base_result = match base_thread.join() {
+        Ok(res) => res?,
+        Err(e) => panic::resume_unwind(e),
+    };
 
     match (base_result, main_result) {
         (Failure(_), Failure(errors)) => {
@@ -262,7 +268,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, true, &mut writer)
+            process(base_nixpkgs.to_owned(), path.to_owned(), true, &mut writer)
                 .with_context(|| format!("Failed test case {name}"))?;
             Ok(writer)
         })?;

From eb1f01440af0f73fdc8e4e214a9a8b70358df971 Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Fri, 23 Feb 2024 01:53:45 +0100
Subject: [PATCH 09/18] tests.nixpkgs-check-by-name: More consistent errors

---
 pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs      | 6 +++---
 .../nixpkgs-check-by-name/tests/alt-callPackage/expected    | 2 +-
 .../tests/non-syntactical-callPackage-by-name/expected      | 2 +-
 .../tests/override-no-call-package/expected                 | 2 +-
 .../nixpkgs-check-by-name/tests/override-no-file/expected   | 2 +-
 .../nixpkgs-check-by-name/tests/override-non-path/expected  | 2 +-
 6 files changed, 8 insertions(+), 8 deletions(-)

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 54976306df32..808cda47cc1a 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
@@ -202,7 +202,7 @@ impl fmt::Display for NixpkgsProblem {
                     "
                     - Because {} exists, the attribute `pkgs.{package_name}` must be defined like
 
-                        {package_name} = callPackage {} {{ /* ... */ }};
+                        {package_name} = callPackage ./{} {{ /* ... */ }};
 
                       This is however not the case: A different `callPackage` is used.
                       It is defined in {}:{} as
@@ -220,7 +220,7 @@ impl fmt::Display for NixpkgsProblem {
                     "
                     - Because {} exists, the attribute `pkgs.{package_name}` must be defined like
 
-                        {package_name} = callPackage {} {{ /* ... */ }};
+                        {package_name} = callPackage ./{} {{ /* ... */ }};
 
                       This is however not the case: The first callPackage argument is not right:
                       It is defined in {}:{} as
@@ -255,7 +255,7 @@ impl fmt::Display for NixpkgsProblem {
                     "
                     - Because {} exists, the attribute `pkgs.{package_name}` must be defined like
 
-                        {package_name} = callPackage {} {{ /* ... */ }};
+                        {package_name} = callPackage ./{} {{ /* ... */ }};
 
                       This is however not the case.
                       It is defined in {}:{} as
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected
index 3abb613de096..80d79ecaba12 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected
@@ -1,6 +1,6 @@
 - Because pkgs/by-name/fo/foo exists, the attribute `pkgs.foo` must be defined like
 
-    foo = callPackage pkgs/by-name/fo/foo/package.nix { /* ... */ };
+    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
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected
index d1a31336504a..5f914d1fce96 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected
@@ -1,6 +1,6 @@
 - Because pkgs/by-name/fo/foo exists, the attribute `pkgs.foo` must be defined like
 
-    foo = callPackage pkgs/by-name/fo/foo/package.nix { /* ... */ };
+    foo = callPackage ./pkgs/by-name/fo/foo/package.nix { /* ... */ };
 
   This is however not the case.
   It is defined in all-packages.nix:4 as
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected
index 54d1eb51be34..f1fc4892b2ab 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected
@@ -1,6 +1,6 @@
 - Because pkgs/by-name/no/nonDerivation exists, the attribute `pkgs.nonDerivation` must be defined like
 
-    nonDerivation = callPackage pkgs/by-name/no/nonDerivation/package.nix { /* ... */ };
+    nonDerivation = callPackage ./pkgs/by-name/no/nonDerivation/package.nix { /* ... */ };
 
   This is however not the case.
   It is defined in all-packages.nix:2 as
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 a1980fdc08da..040d7c635f7a 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,6 +1,6 @@
 - Because pkgs/by-name/no/nonDerivation exists, the attribute `pkgs.nonDerivation` must be defined like
 
-    nonDerivation = callPackage pkgs/by-name/no/nonDerivation/package.nix { /* ... */ };
+    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
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 c8cb5be6d8ee..1f15c6473b4a 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,6 +1,6 @@
 - Because pkgs/by-name/fo/foo exists, the attribute `pkgs.foo` must be defined like
 
-    foo = callPackage pkgs/by-name/fo/foo/package.nix { /* ... */ };
+    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

From 1c4308c35211e91bb97dedaaa1f0cba6e5b8322c Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Fri, 23 Feb 2024 01:54:05 +0100
Subject: [PATCH 10/18] tests.nixpkgs-check-by-name: Better error for empty
 second arg

---
 pkgs/test/nixpkgs-check-by-name/src/eval.rs   |  8 ++++-
 .../src/nixpkgs_problem.rs                    | 33 ++++++++++++++-----
 .../test/nixpkgs-check-by-name/src/ratchet.rs | 12 +++----
 .../nixpkgs-check-by-name/src/references.rs   |  4 +--
 .../tests/manual-definition/expected          | 18 ++++++++--
 .../tests/override-empty-arg/expected         |  9 ++++-
 .../tests/sorted-order/expected               | 18 ++++++++--
 7 files changed, 77 insertions(+), 25 deletions(-)

diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
index ddeac2a445b3..4748e907db72 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
@@ -339,7 +339,13 @@ fn by_name(
                                         // Manual definitions with empty arguments are not allowed
                                         // anymore
                                         Success(if syntactic_call_package.empty_arg {
-                                            Loose(())
+                                            Loose(NixpkgsProblem::EmptyArgument {
+                                                package_name: attribute_name.to_owned(),
+                                                file: relative_file,
+                                                line: location.line,
+                                                column: location.column,
+                                                definition,
+                                            })
                                         } else {
                                             Tight
                                         })
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 808cda47cc1a..cf40238f9cb6 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
@@ -4,11 +4,11 @@ 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
+#[derive(Clone)]
 pub enum NixpkgsProblem {
     ShardNonDir {
         relative_shard_path: PathBuf,
@@ -43,9 +43,12 @@ pub enum NixpkgsProblem {
         relative_package_file: PathBuf,
         package_name: String,
     },
-    WrongCallPackage {
-        relative_package_file: PathBuf,
+    EmptyArgument {
         package_name: String,
+        file: PathBuf,
+        line: usize,
+        column: usize,
+        definition: String,
     },
     NonToplevelCallPackage {
         package_name: String,
@@ -87,7 +90,7 @@ pub enum NixpkgsProblem {
     UnresolvableSymlink {
         relative_package_dir: PathBuf,
         subpath: PathBuf,
-        io_error: io::Error,
+        io_error: String,
     },
     PathInterpolation {
         relative_package_dir: PathBuf,
@@ -112,7 +115,7 @@ pub enum NixpkgsProblem {
         subpath: PathBuf,
         line: usize,
         text: String,
-        io_error: io::Error,
+        io_error: String,
     },
     MovedOutOfByName {
         package_name: String,
@@ -190,11 +193,23 @@ impl fmt::Display for NixpkgsProblem {
                     "pkgs.{package_name}: This attribute is not defined but it should be defined automatically as {}",
                     relative_package_file.display()
                 ),
-            NixpkgsProblem::WrongCallPackage { relative_package_file, package_name } =>
-                write!(
+            NixpkgsProblem::EmptyArgument { package_name, file, line, column, definition } =>
+                writedoc!(
                     f,
-                    "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()
+                    "
+                    - Because {} exists, the attribute `pkgs.{package_name}` must be defined like
+
+                        {package_name} = callPackage ./{} {{ /* ... */ }};
+
+                      Notably the second argument must not be empty, which is not the case.
+                      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::NonToplevelCallPackage { package_name, file, line, column, definition } =>
                 writedoc!(
diff --git a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
index 200bf92c516a..bdc1fbec4a27 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
@@ -4,7 +4,6 @@
 
 use crate::nix_file::CallPackageArgumentInfo;
 use crate::nixpkgs_problem::NixpkgsProblem;
-use crate::structure;
 use crate::validation::{self, Validation, Validation::Success};
 use std::collections::HashMap;
 
@@ -127,17 +126,14 @@ impl<Context: ToNixpkgsProblem> RatchetState<Context> {
 pub enum ManualDefinition {}
 
 impl ToNixpkgsProblem for ManualDefinition {
-    type ToContext = ();
+    type ToContext = NixpkgsProblem;
 
     fn to_nixpkgs_problem(
-        name: &str,
+        _name: &str,
         _optional_from: Option<()>,
-        _to: &Self::ToContext,
+        to: &Self::ToContext,
     ) -> NixpkgsProblem {
-        NixpkgsProblem::WrongCallPackage {
-            relative_package_file: structure::relative_file_for_package(name),
-            package_name: name.to_owned(),
-        }
+        (*to).clone()
     }
 }
 
diff --git a/pkgs/test/nixpkgs-check-by-name/src/references.rs b/pkgs/test/nixpkgs-check-by-name/src/references.rs
index 169e996300ba..b716b99099c4 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/references.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/references.rs
@@ -66,7 +66,7 @@ fn check_path(
             Err(io_error) => NixpkgsProblem::UnresolvableSymlink {
                 relative_package_dir: relative_package_dir.to_path_buf(),
                 subpath: subpath.to_path_buf(),
-                io_error,
+                io_error: io_error.to_string(),
             }
             .into(),
         }
@@ -160,7 +160,7 @@ fn check_nix_file(
                     subpath: subpath.to_path_buf(),
                     line,
                     text,
-                    io_error: e,
+                    io_error: e.to_string(),
                 }
                 .into(),
                 ResolvedPath::Within(..) => {
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected b/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
index 1cc970b7fd72..e619887012d0 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
@@ -1,3 +1,17 @@
-pkgs.noEval: 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/noEval/package.nix { ... }` with a non-empty second argument.
-pkgs.onlyMove: 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/on/onlyMove/package.nix { ... }` with a non-empty second argument.
+- Because pkgs/by-name/no/noEval exists, the attribute `pkgs.noEval` must be defined like
+
+    noEval = callPackage ./pkgs/by-name/no/noEval/package.nix { /* ... */ };
+
+  Notably the second argument must not be empty, which is not the case.
+  It is defined in all-packages.nix:9 as
+
+    noEval = self.callPackage ./pkgs/by-name/no/noEval/package.nix { };
+- Because pkgs/by-name/on/onlyMove exists, the attribute `pkgs.onlyMove` must be defined like
+
+    onlyMove = callPackage ./pkgs/by-name/on/onlyMove/package.nix { /* ... */ };
+
+  Notably the second argument must not be empty, which is not the case.
+  It is defined in all-packages.nix:7 as
+
+    onlyMove = self.callPackage ./pkgs/by-name/on/onlyMove/package.nix { };
 This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected
index bf96b181d7fe..dc64698f4bfa 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected
@@ -1,2 +1,9 @@
-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 { /* ... */ };
+
+  Notably the second argument must not be empty, which is not the case.
+  It is defined in all-packages.nix:2 as
+
+    nonDerivation = self.callPackage ./pkgs/by-name/no/nonDerivation/package.nix { };
 This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected b/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
index b835348cc3cf..d96d0d5638e5 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
@@ -1,5 +1,19 @@
-pkgs.a: 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/a/a/package.nix { ... }` with a non-empty second argument.
+- Because pkgs/by-name/a/a exists, the attribute `pkgs.a` must be defined like
+
+    a = callPackage ./pkgs/by-name/a/a/package.nix { /* ... */ };
+
+  Notably the second argument must not be empty, which is not the case.
+  It is defined in all-packages.nix:2 as
+
+    a = self.callPackage ./pkgs/by-name/a/a/package.nix { };
 pkgs.b: This is a new top-level package of the form `callPackage ... { }`. Please define it in pkgs/by-name/b/b/package.nix instead. See `pkgs/by-name/README.md` for more details. Since the second `callPackage` argument is `{ }`, no manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is needed anymore.
-pkgs.c: 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/c/c/package.nix { ... }` with a non-empty second argument.
+- Because pkgs/by-name/c/c exists, the attribute `pkgs.c` must be defined like
+
+    c = callPackage ./pkgs/by-name/c/c/package.nix { /* ... */ };
+
+  Notably the second argument must not be empty, which is not the case.
+  It is defined in all-packages.nix:4 as
+
+    c = self.callPackage ./pkgs/by-name/c/c/package.nix { };
 pkgs.d: This is a new top-level package of the form `callPackage ... { }`. Please define it in pkgs/by-name/d/d/package.nix instead. See `pkgs/by-name/README.md` for more details. Since the second `callPackage` argument is `{ }`, no manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is needed anymore.
 This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch

From ce271786deaef526b9c99da85bf84ab4ff24a472 Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Fri, 23 Feb 2024 02:16:49 +0100
Subject: [PATCH 11/18] tests.nixpkgs-check-by-name: More spaces in errors

---
 .../nixpkgs-check-by-name/src/nixpkgs_problem.rs  | 15 ++++++++++-----
 .../tests/alt-callPackage/expected                |  1 +
 .../tests/manual-definition/expected              |  2 ++
 .../non-syntactical-callPackage-by-name/expected  |  1 +
 .../tests/override-different-file/expected        |  1 +
 .../tests/override-empty-arg/expected             |  1 +
 .../tests/override-no-call-package/expected       |  1 +
 .../tests/override-no-file/expected               |  1 +
 .../tests/override-non-path/expected              |  1 +
 9 files changed, 19 insertions(+), 5 deletions(-)

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 cf40238f9cb6..762d0e9fb0e0 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
@@ -204,7 +204,8 @@ impl fmt::Display for NixpkgsProblem {
                       Notably the second argument must not be empty, which is not the case.
                       It is defined in {}:{} as
 
-                    {}",
+                    {}
+                    ",
                     structure::relative_dir_for_package(package_name).display(),
                     structure::relative_file_for_package(package_name).display(),
                     file.display(),
@@ -222,7 +223,8 @@ impl fmt::Display for NixpkgsProblem {
                       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(),
@@ -240,7 +242,8 @@ impl fmt::Display for NixpkgsProblem {
                       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(),
@@ -258,7 +261,8 @@ impl fmt::Display for NixpkgsProblem {
                       This is however not the case: The first `callPackage` argument is the wrong path.
                       It is defined in {}:{}:{} as
 
-                        {package_name} = callPackage {} {{ /* ... */ }};",
+                        {package_name} = callPackage {} {{ /* ... */ }};
+                    ",
                     structure::relative_dir_for_package(package_name).display(),
                     create_path_expr(file, expected_path),
                     file.display(), line, column,
@@ -275,7 +279,8 @@ impl fmt::Display for NixpkgsProblem {
                       This is however not the case.
                       It is defined in {}:{} as
 
-                    {}",
+                    {}
+                    ",
                     structure::relative_dir_for_package(package_name).display(),
                     structure::relative_file_for_package(package_name).display(),
                     file.display(),
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected
index 80d79ecaba12..2d05db0d4699 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected
@@ -6,4 +6,5 @@
   It is defined in all-packages.nix:5 as
 
     foo = self.alt.callPackage ({ }: self.someDrv) { };
+
 This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected b/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
index e619887012d0..71d78019604b 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
@@ -6,6 +6,7 @@
   It is defined in all-packages.nix:9 as
 
     noEval = self.callPackage ./pkgs/by-name/no/noEval/package.nix { };
+
 - Because pkgs/by-name/on/onlyMove exists, the attribute `pkgs.onlyMove` must be defined like
 
     onlyMove = callPackage ./pkgs/by-name/on/onlyMove/package.nix { /* ... */ };
@@ -14,4 +15,5 @@
   It is defined in all-packages.nix:7 as
 
     onlyMove = self.callPackage ./pkgs/by-name/on/onlyMove/package.nix { };
+
 This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected
index 5f914d1fce96..f56585ac4678 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected
@@ -6,4 +6,5 @@
   It is defined in all-packages.nix:4 as
 
     foo = self.bar;
+
 This PR introduces the above problems, merging would break the base branch
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 6f439dd4b7d0..571ec8e09a5c 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
@@ -6,4 +6,5 @@
   It is defined in all-packages.nix:2:3 as
 
     nonDerivation = callPackage ./someDrv.nix { /* ... */ };
+
 This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected
index dc64698f4bfa..257030e9a6c0 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected
@@ -6,4 +6,5 @@
   It is defined in all-packages.nix:2 as
 
     nonDerivation = self.callPackage ./pkgs/by-name/no/nonDerivation/package.nix { };
+
 This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected
index f1fc4892b2ab..eb13c4df1e88 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected
@@ -6,4 +6,5 @@
   It is defined in all-packages.nix:2 as
 
     nonDerivation = self.someDrv;
+
 This PR introduces the above problems, merging would break the base branch
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 040d7c635f7a..31679e55b7d6 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
@@ -6,4 +6,5 @@
   It is defined in all-packages.nix:2 as
 
     nonDerivation = self.callPackage ({ someDrv }: someDrv) { };
+
 This PR introduces the above problems, merging would break the base branch
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 1f15c6473b4a..e7fd8242f5f4 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
@@ -6,4 +6,5 @@
   It is defined in all-packages.nix:3 as
 
     foo = self.callPackage ({ someDrv, someFlag }: someDrv) { someFlag = true; };
+
 This PR introduces the above problems, merging would break the base branch

From d2fa5bafa9faff354c13c3117deb3025ca4a5795 Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Fri, 23 Feb 2024 02:17:47 +0100
Subject: [PATCH 12/18] tests.nixpkgs-check-by-name: Improve errors for new
 packages

Or rather, make it more consistent
---
 pkgs/test/nixpkgs-check-by-name/src/eval.rs   |  2 +-
 .../src/nixpkgs_problem.rs                    | 35 +++++++++++++------
 .../test/nixpkgs-check-by-name/src/ratchet.rs |  7 ++--
 .../tests/move-to-non-by-name/expected        | 16 ++++++---
 .../tests/new-package-non-by-name/expected    | 24 ++++++++++---
 .../tests/sorted-order/expected               | 14 ++++++--
 6 files changed, 75 insertions(+), 23 deletions(-)

diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
index 4748e907db72..a8427a27ee43 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
@@ -504,7 +504,7 @@ fn handle_non_by_name_attribute(
                     _ => {
                         // Otherwise, the path is outside `pkgs/by-name`, which means it can be
                         // migrated
-                        Loose(syntactic_call_package)
+                        Loose((syntactic_call_package, location.file))
                     }
                 }
             }
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 762d0e9fb0e0..fac789e3e03f 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
@@ -121,11 +121,13 @@ pub enum NixpkgsProblem {
         package_name: String,
         call_package_path: Option<PathBuf>,
         empty_arg: bool,
+        file: PathBuf,
     },
     NewPackageNotUsingByName {
         package_name: String,
         call_package_path: Option<PathBuf>,
         empty_arg: bool,
+        file: PathBuf,
     },
     InternalCallPackageUsed {
         attr_name: String,
@@ -340,7 +342,7 @@ impl fmt::Display for NixpkgsProblem {
                     subpath.display(),
                     text,
                 ),
-            NixpkgsProblem::MovedOutOfByName { package_name, call_package_path, empty_arg } => {
+            NixpkgsProblem::MovedOutOfByName { package_name, call_package_path, empty_arg, file } => {
                 let call_package_arg =
                     if let Some(path) = &call_package_path {
                         format!("./{}", path.display())
@@ -348,22 +350,30 @@ impl fmt::Display for NixpkgsProblem {
                         "...".into()
                     };
                 if *empty_arg {
-                    write!(
+                    writedoc!(
                         f,
-                        "pkgs.{package_name}: This top-level package was previously defined in {}, but is now manually defined as `callPackage {call_package_arg} {{ }}` (e.g. in `pkgs/top-level/all-packages.nix`). Please move the package back and remove the manual `callPackage`.",
+                        "
+                        - Attribute `pkgs.{package_name} was previously defined in {}, but is now manually defined as `callPackage {call_package_arg} {{ /* ... */ }}` in {}.
+                          Please move the package back and remove the manual `callPackage`.
+                        ",
                         structure::relative_file_for_package(package_name).display(),
+                        file.display(),
                         )
                 } else {
                     // This can happen if users mistakenly assume that for custom arguments,
                     // pkgs/by-name can't be used.
-                    write!(
+                    writedoc!(
                         f,
-                        "pkgs.{package_name}: This top-level package was previously defined in {}, but is now manually defined as `callPackage {call_package_arg} {{ ... }}` (e.g. in `pkgs/top-level/all-packages.nix`). While the manual `callPackage` is still needed, it's not necessary to move the package files.",
+                        "
+                        - Attribute `pkgs.{package_name}` was previously defined in {}, but is now manually defined as `callPackage {call_package_arg} {{ ... }}` in {}.
+                          While the manual `callPackage` is still needed, it's not necessary to move the package files.
+                        ",
                         structure::relative_file_for_package(package_name).display(),
+                        file.display(),
                         )
                 }
             },
-            NixpkgsProblem::NewPackageNotUsingByName { package_name, call_package_path, empty_arg } => {
+            NixpkgsProblem::NewPackageNotUsingByName { package_name, call_package_path, empty_arg, file } => {
                 let call_package_arg =
                     if let Some(path) = &call_package_path {
                         format!("./{}", path.display())
@@ -372,13 +382,18 @@ impl fmt::Display for NixpkgsProblem {
                     };
                 let extra =
                     if *empty_arg {
-                        "Since the second `callPackage` argument is `{ }`, no manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is needed anymore."
+                        format!("Since the second `callPackage` argument is `{{ }}`, no manual `callPackage` in {} is needed anymore.", file.display())
                     } else {
-                        "Since the second `callPackage` argument is not `{ }`, the manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is still needed."
+                        format!("Since the second `callPackage` argument is not `{{ }}`, the manual `callPackage` in {} is still needed.", file.display())
                     };
-                write!(
+                writedoc!(
                     f,
-                    "pkgs.{package_name}: This is a new top-level package of the form `callPackage {call_package_arg} {{ }}`. Please define it in {} instead. See `pkgs/by-name/README.md` for more details. {extra}",
+                    "
+                    - Attribute `pkgs.{package_name}` is a new top-level package using `pkgs.callPackage {call_package_arg} {{ /* ... */ }}`.
+                      Please define it in {} instead.
+                      See `pkgs/by-name/README.md` for more details.
+                      {extra}
+                    ",
                     structure::relative_file_for_package(package_name).display(),
                 )
             },
diff --git a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
index bdc1fbec4a27..28036d3689f1 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
@@ -6,6 +6,7 @@ use crate::nix_file::CallPackageArgumentInfo;
 use crate::nixpkgs_problem::NixpkgsProblem;
 use crate::validation::{self, Validation, Validation::Success};
 use std::collections::HashMap;
+use std::path::PathBuf;
 
 /// The ratchet value for the entirety of Nixpkgs.
 #[derive(Default)]
@@ -145,24 +146,26 @@ impl ToNixpkgsProblem for ManualDefinition {
 pub enum UsesByName {}
 
 impl ToNixpkgsProblem for UsesByName {
-    type ToContext = CallPackageArgumentInfo;
+    type ToContext = (CallPackageArgumentInfo, PathBuf);
 
     fn to_nixpkgs_problem(
         name: &str,
         optional_from: Option<()>,
-        to: &Self::ToContext,
+        (to, file): &Self::ToContext,
     ) -> NixpkgsProblem {
         if let Some(()) = optional_from {
             NixpkgsProblem::MovedOutOfByName {
                 package_name: name.to_owned(),
                 call_package_path: to.relative_path.clone(),
                 empty_arg: to.empty_arg,
+                file: file.to_owned(),
             }
         } else {
             NixpkgsProblem::NewPackageNotUsingByName {
                 package_name: name.to_owned(),
                 call_package_path: to.relative_path.clone(),
                 empty_arg: to.empty_arg,
+                file: file.to_owned(),
             }
         }
     }
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected
index 8a2116aebcb9..3319cc271ac9 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected
@@ -1,5 +1,13 @@
-pkgs.foo1: This top-level package was previously defined in pkgs/by-name/fo/foo1/package.nix, but is now manually defined as `callPackage ... { }` (e.g. in `pkgs/top-level/all-packages.nix`). Please move the package back and remove the manual `callPackage`.
-pkgs.foo2: This top-level package was previously defined in pkgs/by-name/fo/foo2/package.nix, but is now manually defined as `callPackage ./without-config.nix { }` (e.g. in `pkgs/top-level/all-packages.nix`). Please move the package back and remove the manual `callPackage`.
-pkgs.foo3: This top-level package was previously defined in pkgs/by-name/fo/foo3/package.nix, but is now manually defined as `callPackage ... { ... }` (e.g. in `pkgs/top-level/all-packages.nix`). While the manual `callPackage` is still needed, it's not necessary to move the package files.
-pkgs.foo4: This top-level package was previously defined in pkgs/by-name/fo/foo4/package.nix, but is now manually defined as `callPackage ./with-config.nix { ... }` (e.g. in `pkgs/top-level/all-packages.nix`). While the manual `callPackage` is still needed, it's not necessary to move the package files.
+- Attribute `pkgs.foo1 was previously defined in pkgs/by-name/fo/foo1/package.nix, but is now manually defined as `callPackage ... { /* ... */ }` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/all-packages.nix.
+  Please move the package back and remove the manual `callPackage`.
+
+- Attribute `pkgs.foo2 was previously defined in pkgs/by-name/fo/foo2/package.nix, but is now manually defined as `callPackage ./without-config.nix { /* ... */ }` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/all-packages.nix.
+  Please move the package back and remove the manual `callPackage`.
+
+- Attribute `pkgs.foo3` was previously defined in pkgs/by-name/fo/foo3/package.nix, but is now manually defined as `callPackage ... { ... }` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/all-packages.nix.
+  While the manual `callPackage` is still needed, it's not necessary to move the package files.
+
+- Attribute `pkgs.foo4` was previously defined in pkgs/by-name/fo/foo4/package.nix, but is now manually defined as `callPackage ./with-config.nix { ... }` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/all-packages.nix.
+  While the manual `callPackage` is still needed, it's not necessary to move the package files.
+
 This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected
index 31eff28f3c88..a7db19e5fd90 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected
@@ -1,5 +1,21 @@
-pkgs.new1: This is a new top-level package of the form `callPackage ... { }`. Please define it in pkgs/by-name/ne/new1/package.nix instead. See `pkgs/by-name/README.md` for more details. Since the second `callPackage` argument is `{ }`, no manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is needed anymore.
-pkgs.new2: This is a new top-level package of the form `callPackage ./without-config.nix { }`. Please define it in pkgs/by-name/ne/new2/package.nix instead. See `pkgs/by-name/README.md` for more details. Since the second `callPackage` argument is `{ }`, no manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is needed anymore.
-pkgs.new3: This is a new top-level package of the form `callPackage ... { }`. Please define it in pkgs/by-name/ne/new3/package.nix instead. See `pkgs/by-name/README.md` for more details. Since the second `callPackage` argument is not `{ }`, the manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is still needed.
-pkgs.new4: This is a new top-level package of the form `callPackage ./with-config.nix { }`. Please define it in pkgs/by-name/ne/new4/package.nix instead. See `pkgs/by-name/README.md` for more details. Since the second `callPackage` argument is not `{ }`, the manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is still needed.
+- Attribute `pkgs.new1` is a new top-level package using `pkgs.callPackage ... { /* ... */ }`.
+  Please define it in pkgs/by-name/ne/new1/package.nix instead.
+  See `pkgs/by-name/README.md` for more details.
+  Since the second `callPackage` argument is `{ }`, no manual `callPackage` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/all-packages.nix is needed anymore.
+
+- Attribute `pkgs.new2` is a new top-level package using `pkgs.callPackage ./without-config.nix { /* ... */ }`.
+  Please define it in pkgs/by-name/ne/new2/package.nix instead.
+  See `pkgs/by-name/README.md` for more details.
+  Since the second `callPackage` argument is `{ }`, no manual `callPackage` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/all-packages.nix is needed anymore.
+
+- Attribute `pkgs.new3` is a new top-level package using `pkgs.callPackage ... { /* ... */ }`.
+  Please define it in pkgs/by-name/ne/new3/package.nix instead.
+  See `pkgs/by-name/README.md` for more details.
+  Since the second `callPackage` argument is not `{ }`, the manual `callPackage` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/all-packages.nix is still needed.
+
+- Attribute `pkgs.new4` is a new top-level package using `pkgs.callPackage ./with-config.nix { /* ... */ }`.
+  Please define it in pkgs/by-name/ne/new4/package.nix instead.
+  See `pkgs/by-name/README.md` for more details.
+  Since the second `callPackage` argument is not `{ }`, the manual `callPackage` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/all-packages.nix is still needed.
+
 This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected b/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
index d96d0d5638e5..2b3c2cdeee18 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
@@ -6,7 +6,12 @@
   It is defined in all-packages.nix:2 as
 
     a = self.callPackage ./pkgs/by-name/a/a/package.nix { };
-pkgs.b: This is a new top-level package of the form `callPackage ... { }`. Please define it in pkgs/by-name/b/b/package.nix instead. See `pkgs/by-name/README.md` for more details. Since the second `callPackage` argument is `{ }`, no manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is needed anymore.
+
+- Attribute `pkgs.b` is a new top-level package using `pkgs.callPackage ... { /* ... */ }`.
+  Please define it in pkgs/by-name/b/b/package.nix instead.
+  See `pkgs/by-name/README.md` for more details.
+  Since the second `callPackage` argument is `{ }`, no manual `callPackage` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/all-packages.nix is needed anymore.
+
 - Because pkgs/by-name/c/c exists, the attribute `pkgs.c` must be defined like
 
     c = callPackage ./pkgs/by-name/c/c/package.nix { /* ... */ };
@@ -15,5 +20,10 @@ pkgs.b: This is a new top-level package of the form `callPackage ... { }`. Pleas
   It is defined in all-packages.nix:4 as
 
     c = self.callPackage ./pkgs/by-name/c/c/package.nix { };
-pkgs.d: This is a new top-level package of the form `callPackage ... { }`. Please define it in pkgs/by-name/d/d/package.nix instead. See `pkgs/by-name/README.md` for more details. Since the second `callPackage` argument is `{ }`, no manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is needed anymore.
+
+- Attribute `pkgs.d` is a new top-level package using `pkgs.callPackage ... { /* ... */ }`.
+  Please define it in pkgs/by-name/d/d/package.nix instead.
+  See `pkgs/by-name/README.md` for more details.
+  Since the second `callPackage` argument is `{ }`, no manual `callPackage` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/all-packages.nix is needed anymore.
+
 This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch

From 64da6178bf10b92f57352d7d0cfca7eebbd527df Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Fri, 23 Feb 2024 02:32:27 +0100
Subject: [PATCH 13/18] tests.nixpkgs-check-by-name: Fix internal docs

---
 pkgs/test/nixpkgs-check-by-name/src/nix_file.rs | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

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 1382f389e2da..0176164e2cea 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs
@@ -86,7 +86,7 @@ pub struct CallPackageArgumentInfo {
 impl NixFile {
     /// Returns information about callPackage arguments for an attribute at a specific line/column
     /// index.
-    /// If the location is not of the form `<attr> = callPackage <arg1> <arg2>;`, `Ok(Err(String))` is
+    /// If the location is not of the form `<attr> = callPackage <arg1> <arg2>;`, `Ok((None, String))` is
     /// returned, with String being how the definition looks like.
     ///
     /// This function only returns `Err` for problems that can't be caused by the Nix contents,
@@ -109,7 +109,10 @@ impl NixFile {
     ///
     /// You'll get back
     /// ```rust
-    /// Ok(Ok(CallPackageArgumentInfo { path = Some("default.nix"), empty_arg: true }))
+    /// Ok((
+    ///   Some(CallPackageArgumentInfo { path = Some("default.nix"), empty_arg: true }),
+    ///   "foo = self.callPackage ./default.nix { };",
+    /// ))
     /// ```
     ///
     /// Note that this also returns the same for `pythonPackages.callPackage`. It doesn't make an

From 2e8d778994b4e2a285bb599808a1b168077e5a66 Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Fri, 1 Mar 2024 01:12:11 +0100
Subject: [PATCH 14/18] tests.nixpkgs-check-by-name: Various minor improvements

Co-Authored-By: Philip Taron <philip.taron@gmail.com>
---
 pkgs/test/nixpkgs-check-by-name/src/eval.rs   | 173 ++++++++++--------
 pkgs/test/nixpkgs-check-by-name/src/main.rs   |  13 +-
 .../nixpkgs-check-by-name/src/nix_file.rs     |  39 ++--
 .../src/nixpkgs_problem.rs                    | 126 +++++++------
 .../test/nixpkgs-check-by-name/src/ratchet.rs |  20 +-
 .../tests/alt-callPackage/expected            |   5 +-
 .../tests/base-fixed/expected                 |   2 +-
 .../tests/base-still-broken/expected          |   2 +-
 .../tests/broken-autocall/expected            |   2 +-
 .../tests/incorrect-shard/expected            |   2 +-
 .../tests/internalCallPackage/expected        |   2 +-
 .../tests/invalid-package-name/expected       |   2 +-
 .../tests/invalid-shard-name/expected         |   2 +-
 .../tests/manual-definition/expected          |   8 +-
 .../tests/missing-package-nix/expected        |   2 +-
 .../tests/move-to-non-by-name/expected        |   6 +-
 .../tests/multiple-failures/expected          |   2 +-
 .../tests/new-package-non-by-name/expected    |   2 +-
 .../tests/non-attrs/expected                  |   2 +-
 .../tests/non-derivation/expected             |   2 +-
 .../all-packages.nix                          |   1 +
 .../expected                                  |   5 +-
 .../tests/override-different-file/expected    |   5 +-
 .../tests/override-empty-arg/expected         |   5 +-
 .../tests/override-no-call-package/expected   |   5 +-
 .../tests/override-no-file/expected           |   5 +-
 .../tests/override-non-path/expected          |   5 +-
 .../tests/package-dir-is-file/expected        |   2 +-
 .../tests/package-nix-dir/expected            |   2 +-
 .../tests/ref-absolute/expected               |   2 +-
 .../tests/ref-escape/expected                 |   2 +-
 .../tests/ref-nix-path/expected               |   2 +-
 .../tests/ref-path-subexpr/expected           |   2 +-
 .../tests/shard-file/expected                 |   2 +-
 .../tests/sorted-order/expected               |   8 +-
 .../tests/symlink-escape/expected             |   2 +-
 .../tests/symlink-invalid/expected            |   2 +-
 .../tests/unknown-location/expected           |   2 +-
 38 files changed, 261 insertions(+), 212 deletions(-)

diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
index a8427a27ee43..a43a0cd743a7 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
@@ -1,5 +1,8 @@
+use crate::nix_file::CallPackageArgumentInfo;
 use crate::nixpkgs_problem::NixpkgsProblem;
 use crate::ratchet;
+use crate::ratchet::RatchetState::Loose;
+use crate::ratchet::RatchetState::Tight;
 use crate::structure;
 use crate::utils;
 use crate::validation::ResultIteratorExt as _;
@@ -296,84 +299,27 @@ fn by_name(
                         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}"
-                                )
-                            })?;
+                        let relative_location_file = location.relative_file(nixpkgs_path).with_context(|| {
+                            format!("Failed to resolve the file where attribute {attribute_name} is defined")
+                        })?;
 
                         // Figure out whether it's an attribute definition of the form `= callPackage <arg1> <arg2>`,
                         // returning the arguments if so.
-                        let (optional_syntactic_call_package, definition) = 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}"))?;
+                        let (optional_syntactic_call_package, definition) = 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`
-                        match (is_semantic_call_package, optional_syntactic_call_package) {
-                            // Something like `<attr> = foo`
-                            (_, None) => NixpkgsProblem::NonSyntacticCallPackage {
-                                package_name: attribute_name.to_owned(),
-                                file: relative_file,
-                                line: location.line,
-                                column: location.column,
-                                definition,
-                            }
-                            .into(),
-                            // Something like `<attr> = pythonPackages.callPackage ...`
-                            (false, Some(_)) => NixpkgsProblem::NonToplevelCallPackage {
-                                package_name: attribute_name.to_owned(),
-                                file: relative_file,
-                                line: location.line,
-                                column: location.column,
-                                definition,
-                            }
-                            .into(),
-                            // Something like `<attr> = pkgs.callPackage ...`
-                            (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
-                                        // anymore
-                                        Success(if syntactic_call_package.empty_arg {
-                                            Loose(NixpkgsProblem::EmptyArgument {
-                                                package_name: attribute_name.to_owned(),
-                                                file: relative_file,
-                                                line: location.line,
-                                                column: location.column,
-                                                definition,
-                                            })
-                                        } else {
-                                            Tight
-                                        })
-                                    } else {
-                                        // 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::NonPath {
-                                        package_name: attribute_name.to_owned(),
-                                        file: relative_file,
-                                        line: location.line,
-                                        column: location.column,
-                                        definition,
-                                    }
-                                    .into()
-                                }
-                            }
-                        }
+                        by_name_override(
+                            attribute_name,
+                            relative_package_file,
+                            is_semantic_call_package,
+                            optional_syntactic_call_package,
+                            definition,
+                            location,
+                            relative_location_file,
+                        )
                     } else {
                         // If manual definitions don't have a location, it's likely `mapAttrs`'d
                         // over, e.g. if it's defined in aliases.nix.
@@ -401,6 +347,85 @@ fn by_name(
     )
 }
 
+/// Handles the case for packages in `pkgs/by-name` that are manually overridden, e.g. in
+/// all-packages.nix
+fn by_name_override(
+    attribute_name: &str,
+    expected_package_file: PathBuf,
+    is_semantic_call_package: bool,
+    optional_syntactic_call_package: Option<CallPackageArgumentInfo>,
+    definition: String,
+    location: Location,
+    relative_location_file: PathBuf,
+) -> validation::Validation<ratchet::RatchetState<ratchet::ManualDefinition>> {
+    // At this point, we completed two different checks for whether it's a
+    // `callPackage`
+    match (is_semantic_call_package, optional_syntactic_call_package) {
+        // Something like `<attr> = foo`
+        (_, None) => NixpkgsProblem::NonSyntacticCallPackage {
+            package_name: attribute_name.to_owned(),
+            file: relative_location_file,
+            line: location.line,
+            column: location.column,
+            definition,
+        }
+        .into(),
+        // Something like `<attr> = pythonPackages.callPackage ...`
+        (false, Some(_)) => NixpkgsProblem::NonToplevelCallPackage {
+            package_name: attribute_name.to_owned(),
+            file: relative_location_file,
+            line: location.line,
+            column: location.column,
+            definition,
+        }
+        .into(),
+        // Something like `<attr> = pkgs.callPackage ...`
+        (true, Some(syntactic_call_package)) => {
+            if let Some(actual_package_file) = syntactic_call_package.relative_path {
+                if actual_package_file != expected_package_file {
+                    // Wrong path
+                    NixpkgsProblem::WrongCallPackagePath {
+                        package_name: attribute_name.to_owned(),
+                        file: relative_location_file,
+                        line: location.line,
+                        actual_path: actual_package_file,
+                        expected_path: expected_package_file,
+                    }
+                    .into()
+                } else {
+                    // Manual definitions with empty arguments are not allowed
+                    // anymore, but existing ones should continue to be allowed
+                    let manual_definition_ratchet = if syntactic_call_package.empty_arg {
+                        // This is the state to migrate away from
+                        Loose(NixpkgsProblem::EmptyArgument {
+                            package_name: attribute_name.to_owned(),
+                            file: relative_location_file,
+                            line: location.line,
+                            column: location.column,
+                            definition,
+                        })
+                    } else {
+                        // This is the state to migrate to
+                        Tight
+                    };
+
+                    Success(manual_definition_ratchet)
+                }
+            } else {
+                // No path
+                NixpkgsProblem::NonPath {
+                    package_name: attribute_name.to_owned(),
+                    file: relative_location_file,
+                    line: location.line,
+                    column: location.column,
+                    definition,
+                }
+                .into()
+            }
+        }
+    }
+}
+
 /// Handles the evaluation result for an attribute _not_ in `pkgs/by-name`,
 /// turning it into a validation result.
 fn handle_non_by_name_attribute(
diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs
index c92d0af64969..dcc9cb9e716d 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/main.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs
@@ -93,27 +93,25 @@ pub fn process<W: io::Write>(
             for error in errors {
                 writeln!(error_writer, "{}", error.to_string().red())?
             }
-            writeln!(error_writer, "{}", "The base branch is broken and still has above problems with this PR, these need to be fixed first.\nConsider reverting the PR that introduced these problems in order to prevent more failures of unrelated PRs.".yellow())?;
+            writeln!(error_writer, "{}", "The base branch is broken and still has above problems with this PR, which need to be fixed first.\nConsider reverting the PR that introduced these problems in order to prevent more failures of unrelated PRs.".yellow())?;
             Ok(false)
         }
         (Failure(_), Success(_)) => {
-            // Base branch fails, but the PR fixes it
             writeln!(
                 error_writer,
                 "{}",
-                "The base branch was broken, but this PR fixes it, nice job!".green()
+                "The base branch is broken, but this PR fixes it. Nice job!".green()
             )?;
             Ok(true)
         }
         (Success(_), Failure(errors)) => {
-            // Base branch succeeds, the PR breaks it
             for error in errors {
                 writeln!(error_writer, "{}", error.to_string().red())?
             }
             writeln!(
                 error_writer,
                 "{}",
-                "This PR introduces the above problems, merging would break the base branch"
+                "This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break."
                     .yellow()
             )?;
             Ok(false)
@@ -125,7 +123,8 @@ pub fn process<W: io::Write>(
                     for error in errors {
                         writeln!(error_writer, "{}", error.to_string().red())?
                     }
-                    writeln!(error_writer, "{}", "This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch".yellow())?;
+                    writeln!(error_writer, "{}", "This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.".yellow())?;
+
                     Ok(false)
                 }
                 Success(()) => {
@@ -225,7 +224,7 @@ mod tests {
         test_nixpkgs(
             "case_sensitive",
             &path,
-            "pkgs/by-name/fo: Duplicate case-sensitive package directories \"foO\" and \"foo\".\nThis PR introduces the above problems, merging would break the base branch\n",
+            "pkgs/by-name/fo: Duplicate case-sensitive package directories \"foO\" and \"foo\".\nThis PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.\n",
         )?;
 
         Ok(())
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 0176164e2cea..1cb5d1c0d08d 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs
@@ -2,6 +2,7 @@
 
 use crate::utils::LineIndex;
 use anyhow::Context;
+use itertools::Either::{self, Left, Right};
 use rnix::ast;
 use rnix::ast::Expr;
 use rnix::ast::HasEntry;
@@ -86,8 +87,8 @@ pub struct CallPackageArgumentInfo {
 impl NixFile {
     /// Returns information about callPackage arguments for an attribute at a specific line/column
     /// index.
-    /// If the location is not of the form `<attr> = callPackage <arg1> <arg2>;`, `Ok((None, String))` is
-    /// returned, with String being how the definition looks like.
+    /// If the definition at the given location is not of the form `<attr> = callPackage <arg1> <arg2>;`,
+    /// `Ok((None, String))` is returned, with `String` being the definition itself.
     ///
     /// This function only returns `Err` for problems that can't be caused by the Nix contents,
     /// but rather problems in this programs code itself.
@@ -124,8 +125,8 @@ impl NixFile {
         relative_to: &Path,
     ) -> anyhow::Result<(Option<CallPackageArgumentInfo>, String)> {
         Ok(match self.attrpath_value_at(line, column)? {
-            Err(definition) => (None, definition),
-            Ok(attrpath_value) => {
+            Left(definition) => (None, definition),
+            Right(attrpath_value) => {
                 let definition = attrpath_value.to_string();
                 let attrpath_value =
                     self.attrpath_value_call_package_argument_info(attrpath_value, relative_to)?;
@@ -139,7 +140,7 @@ impl NixFile {
         &self,
         line: usize,
         column: usize,
-    ) -> anyhow::Result<Result<ast::AttrpathValue, String>> {
+    ) -> anyhow::Result<Either<String, ast::AttrpathValue>> {
         let index = self.line_index.fromlinecolumn(line, column);
 
         let token_at_offset = self
@@ -173,11 +174,11 @@ impl NixFile {
             // This is the only other way how `builtins.unsafeGetAttrPos` can return
             // attribute positions, but we only look for ones like `<attr-path> = <value>`, so
             // ignore this
-            return Ok(Err(node.to_string()));
+            return Ok(Left(node.to_string()));
         } else {
             // However, anything else is not expected and smells like a bug
             anyhow::bail!(
-                "Node in {} is neither an attribute node, nor an inherit node: {node:?}",
+                "Node in {} is neither an attribute node nor an inherit node: {node:?}",
                 self.path.display()
             )
         }
@@ -217,7 +218,9 @@ impl NixFile {
         // 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(Ok(ast::AttrpathValue::cast(attrpath_value_node).unwrap()))
+        Ok(Right(
+            ast::AttrpathValue::cast(attrpath_value_node).unwrap(),
+        ))
     }
 
     // Internal function mainly to make attrpath_value_at independently testable
@@ -446,24 +449,24 @@ mod tests {
 
         // These are builtins.unsafeGetAttrPos locations for the attributes
         let cases = [
-            (2, 3, Ok("foo = 1;")),
-            (3, 3, Ok(r#""bar" = 2;"#)),
-            (4, 3, Ok(r#"${"baz"} = 3;"#)),
-            (5, 3, Ok(r#""${"qux"}" = 4;"#)),
-            (8, 3, Ok("quux\n  # B\n  =\n  # C\n  5\n  # D\n  ;")),
-            (17, 7, Ok("quuux/**/=/**/5/**/;")),
-            (19, 10, Err("inherit toInherit;")),
-            (20, 22, Err("inherit (toInherit) toInherit;")),
+            (2, 3, Right("foo = 1;")),
+            (3, 3, Right(r#""bar" = 2;"#)),
+            (4, 3, Right(r#"${"baz"} = 3;"#)),
+            (5, 3, Right(r#""${"qux"}" = 4;"#)),
+            (8, 3, Right("quux\n  # B\n  =\n  # C\n  5\n  # D\n  ;")),
+            (17, 7, Right("quuux/**/=/**/5/**/;")),
+            (19, 10, Left("inherit toInherit;")),
+            (20, 22, Left("inherit (toInherit) toInherit;")),
         ];
 
         for (line, column, expected_result) in cases {
             let actual_result = nix_file
                 .attrpath_value_at(line, column)
                 .context(format!("line {line}, column {column}"))?
-                .map(|node| node.to_string());
+                .map_right(|node| node.to_string());
             let owned_expected_result = expected_result
                 .map(|x| x.to_string())
-                .map_err(|x| x.to_string());
+                .map_left(|x| x.to_string());
             assert_eq!(
                 actual_result, owned_expected_result,
                 "line {line}, column {column}"
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 fac789e3e03f..03203073fd4a 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
@@ -68,7 +68,6 @@ pub enum NixpkgsProblem {
         package_name: String,
         file: PathBuf,
         line: usize,
-        column: usize,
         actual_path: PathBuf,
         expected_path: PathBuf,
     },
@@ -117,16 +116,24 @@ pub enum NixpkgsProblem {
         text: String,
         io_error: String,
     },
-    MovedOutOfByName {
+    MovedOutOfByNameEmptyArg {
         package_name: String,
         call_package_path: Option<PathBuf>,
-        empty_arg: bool,
         file: PathBuf,
     },
-    NewPackageNotUsingByName {
+    MovedOutOfByNameNonEmptyArg {
+        package_name: String,
+        call_package_path: Option<PathBuf>,
+        file: PathBuf,
+    },
+    NewPackageNotUsingByNameEmptyArg {
+        package_name: String,
+        call_package_path: Option<PathBuf>,
+        file: PathBuf,
+    },
+    NewPackageNotUsingByNameNonEmptyArg {
         package_name: String,
         call_package_path: Option<PathBuf>,
-        empty_arg: bool,
         file: PathBuf,
     },
     InternalCallPackageUsed {
@@ -203,8 +210,7 @@ impl fmt::Display for NixpkgsProblem {
 
                         {package_name} = callPackage ./{} {{ /* ... */ }};
 
-                      Notably the second argument must not be empty, which is not the case.
-                      It is defined in {}:{} as
+                      However, in this PR, the second argument is empty. See the definition in {}:{}:
 
                     {}
                     ",
@@ -222,8 +228,7 @@ impl fmt::Display for NixpkgsProblem {
 
                         {package_name} = callPackage ./{} {{ /* ... */ }};
 
-                      This is however not the case: A different `callPackage` is used.
-                      It is defined in {}:{} as
+                      However, in this PR, a different `callPackage` is used. See the definition in {}:{}:
 
                     {}
                     ",
@@ -241,8 +246,7 @@ impl fmt::Display for NixpkgsProblem {
 
                         {package_name} = callPackage ./{} {{ /* ... */ }};
 
-                      This is however not the case: The first callPackage argument is not right:
-                      It is defined in {}:{} as
+                      However, in this PR, the first `callPackage` argument is not a path. See the definition in {}:{}:
 
                     {}
                     ",
@@ -252,7 +256,7 @@ impl fmt::Display for NixpkgsProblem {
                     line,
                     indent_definition(*column, definition.clone()),
                 ),
-            NixpkgsProblem::WrongCallPackagePath { package_name, file, line, column, actual_path, expected_path } =>
+            NixpkgsProblem::WrongCallPackagePath { package_name, file, line, actual_path, expected_path } =>
                 writedoc! {
                     f,
                     "
@@ -260,14 +264,13 @@ impl fmt::Display for NixpkgsProblem {
 
                         {package_name} = callPackage {} {{ /* ... */ }};
 
-                      This is however not the case: The first `callPackage` argument is the wrong path.
-                      It is defined in {}:{}:{} as
+                      However, in this PR, the first `callPackage` argument is the wrong path. See the definition in {}:{}:
 
                         {package_name} = callPackage {} {{ /* ... */ }};
                     ",
                     structure::relative_dir_for_package(package_name).display(),
                     create_path_expr(file, expected_path),
-                    file.display(), line, column,
+                    file.display(), line,
                     create_path_expr(file, actual_path),
                 },
             NixpkgsProblem::NonSyntacticCallPackage { package_name, file, line, column, definition } => {
@@ -278,8 +281,7 @@ impl fmt::Display for NixpkgsProblem {
 
                         {package_name} = callPackage ./{} {{ /* ... */ }};
 
-                      This is however not the case.
-                      It is defined in {}:{} as
+                      However, in this PR, it isn't defined that way. See the definition in {}:{}
 
                     {}
                     ",
@@ -342,49 +344,48 @@ impl fmt::Display for NixpkgsProblem {
                     subpath.display(),
                     text,
                 ),
-            NixpkgsProblem::MovedOutOfByName { package_name, call_package_path, empty_arg, file } => {
+            NixpkgsProblem::MovedOutOfByNameEmptyArg { package_name, call_package_path, file } => {
                 let call_package_arg =
                     if let Some(path) = &call_package_path {
                         format!("./{}", path.display())
                     } else {
                         "...".into()
                     };
-                if *empty_arg {
-                    writedoc!(
-                        f,
-                        "
-                        - Attribute `pkgs.{package_name} was previously defined in {}, but is now manually defined as `callPackage {call_package_arg} {{ /* ... */ }}` in {}.
-                          Please move the package back and remove the manual `callPackage`.
-                        ",
-                        structure::relative_file_for_package(package_name).display(),
-                        file.display(),
-                        )
-                } else {
-                    // This can happen if users mistakenly assume that for custom arguments,
-                    // pkgs/by-name can't be used.
-                    writedoc!(
-                        f,
-                        "
-                        - Attribute `pkgs.{package_name}` was previously defined in {}, but is now manually defined as `callPackage {call_package_arg} {{ ... }}` in {}.
-                          While the manual `callPackage` is still needed, it's not necessary to move the package files.
-                        ",
-                        structure::relative_file_for_package(package_name).display(),
-                        file.display(),
-                        )
-                }
+                writedoc!(
+                    f,
+                    "
+                    - Attribute `pkgs.{package_name}` was previously defined in {}, but is now manually defined as `callPackage {call_package_arg} {{ /* ... */ }}` in {}.
+                      Please move the package back and remove the manual `callPackage`.
+                    ",
+                    structure::relative_file_for_package(package_name).display(),
+                    file.display(),
+                    )
             },
-            NixpkgsProblem::NewPackageNotUsingByName { package_name, call_package_path, empty_arg, file } => {
+            NixpkgsProblem::MovedOutOfByNameNonEmptyArg { package_name, call_package_path, file } => {
                 let call_package_arg =
                     if let Some(path) = &call_package_path {
                         format!("./{}", path.display())
                     } else {
                         "...".into()
                     };
-                let extra =
-                    if *empty_arg {
-                        format!("Since the second `callPackage` argument is `{{ }}`, no manual `callPackage` in {} is needed anymore.", file.display())
+                // This can happen if users mistakenly assume that for custom arguments,
+                // pkgs/by-name can't be used.
+                writedoc!(
+                    f,
+                    "
+                    - Attribute `pkgs.{package_name}` was previously defined in {}, but is now manually defined as `callPackage {call_package_arg} {{ ... }}` in {}.
+                      While the manual `callPackage` is still needed, it's not necessary to move the package files.
+                    ",
+                    structure::relative_file_for_package(package_name).display(),
+                    file.display(),
+                    )
+            },
+            NixpkgsProblem::NewPackageNotUsingByNameEmptyArg { package_name, call_package_path, file } => {
+                let call_package_arg =
+                    if let Some(path) = &call_package_path {
+                        format!("./{}", path.display())
                     } else {
-                        format!("Since the second `callPackage` argument is not `{{ }}`, the manual `callPackage` in {} is still needed.", file.display())
+                        "...".into()
                     };
                 writedoc!(
                     f,
@@ -392,9 +393,29 @@ impl fmt::Display for NixpkgsProblem {
                     - Attribute `pkgs.{package_name}` is a new top-level package using `pkgs.callPackage {call_package_arg} {{ /* ... */ }}`.
                       Please define it in {} instead.
                       See `pkgs/by-name/README.md` for more details.
-                      {extra}
+                      Since the second `callPackage` argument is `{{ }}`, no manual `callPackage` in {} is needed anymore.
                     ",
                     structure::relative_file_for_package(package_name).display(),
+                    file.display(),
+                )
+            },
+            NixpkgsProblem::NewPackageNotUsingByNameNonEmptyArg { package_name, call_package_path, file } => {
+                let call_package_arg =
+                    if let Some(path) = &call_package_path {
+                        format!("./{}", path.display())
+                    } else {
+                        "...".into()
+                    };
+                writedoc!(
+                    f,
+                    "
+                    - Attribute `pkgs.{package_name}` is a new top-level package using `pkgs.callPackage {call_package_arg} {{ /* ... */ }}`.
+                      Please define it in {} instead.
+                      See `pkgs/by-name/README.md` for more details.
+                      Since the second `callPackage` argument is not `{{ }}`, the manual `callPackage` in {} is still needed.
+                    ",
+                    structure::relative_file_for_package(package_name).display(),
+                    file.display(),
                 )
             },
             NixpkgsProblem::InternalCallPackageUsed { attr_name } =>
@@ -426,14 +447,13 @@ fn indent_definition(column: usize, definition: String) -> String {
 
 /// 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)
+    // These `expect` calls should never trigger because we only call this function with
+    // relative paths that have a parent. That's why we `expect` them!
     let from = RelativePath::from_path(&from_file)
-        .unwrap()
+        .expect("a relative path")
         .parent()
-        .unwrap();
-    let to = RelativePath::from_path(&to_file).unwrap();
+        .expect("a parent for this path");
+    let to = RelativePath::from_path(&to_file).expect("a path");
     let rel = from.relative(to);
     format!("./{rel}")
 }
diff --git a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
index 28036d3689f1..eed5071a5a2a 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
@@ -154,17 +154,29 @@ impl ToNixpkgsProblem for UsesByName {
         (to, file): &Self::ToContext,
     ) -> NixpkgsProblem {
         if let Some(()) = optional_from {
-            NixpkgsProblem::MovedOutOfByName {
+            if to.empty_arg {
+                NixpkgsProblem::MovedOutOfByNameEmptyArg {
+                    package_name: name.to_owned(),
+                    call_package_path: to.relative_path.clone(),
+                    file: file.to_owned(),
+                }
+            } else {
+                NixpkgsProblem::MovedOutOfByNameNonEmptyArg {
+                    package_name: name.to_owned(),
+                    call_package_path: to.relative_path.clone(),
+                    file: file.to_owned(),
+                }
+            }
+        } else if to.empty_arg {
+            NixpkgsProblem::NewPackageNotUsingByNameEmptyArg {
                 package_name: name.to_owned(),
                 call_package_path: to.relative_path.clone(),
-                empty_arg: to.empty_arg,
                 file: file.to_owned(),
             }
         } else {
-            NixpkgsProblem::NewPackageNotUsingByName {
+            NixpkgsProblem::NewPackageNotUsingByNameNonEmptyArg {
                 package_name: name.to_owned(),
                 call_package_path: to.relative_path.clone(),
-                empty_arg: to.empty_arg,
                 file: file.to_owned(),
             }
         }
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected
index 2d05db0d4699..1d92e652200e 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected
@@ -2,9 +2,8 @@
 
     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
+  However, in this PR, a different `callPackage` is used. See the definition in all-packages.nix:5:
 
     foo = self.alt.callPackage ({ }: self.someDrv) { };
 
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/expected b/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/expected
index feb6cce2fd04..e209e1855314 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/expected
@@ -1 +1 @@
-The base branch was broken, but this PR fixes it, nice job!
+The base branch is broken, but this PR fixes it. Nice job!
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/expected b/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/expected
index 4f0d08357d1f..c25f06b4150e 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/expected
@@ -1,3 +1,3 @@
 pkgs/by-name/bar: This is a file, but it should be a directory.
-The base branch is broken and still has above problems with this PR, these need to be fixed first.
+The base branch is broken and still has above problems with this PR, which need to be fixed first.
 Consider reverting the PR that introduced these problems in order to prevent more failures of unrelated PRs.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/broken-autocall/expected b/pkgs/test/nixpkgs-check-by-name/tests/broken-autocall/expected
index 6b937106b806..15b3e3ff6ede 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/broken-autocall/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/broken-autocall/expected
@@ -1,2 +1,2 @@
 pkgs.foo: This attribute is not defined but it should be defined automatically as pkgs/by-name/fo/foo/package.nix
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/incorrect-shard/expected b/pkgs/test/nixpkgs-check-by-name/tests/incorrect-shard/expected
index ec1918cace8c..3b0146cdc146 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/incorrect-shard/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/incorrect-shard/expected
@@ -1,2 +1,2 @@
 pkgs/by-name/aa/FOO: Incorrect directory location, should be pkgs/by-name/fo/FOO instead.
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/internalCallPackage/expected b/pkgs/test/nixpkgs-check-by-name/tests/internalCallPackage/expected
index 9fef30d14f37..b3d0c6fc1a40 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/internalCallPackage/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/internalCallPackage/expected
@@ -1,2 +1,2 @@
 pkgs.foo: This attribute is defined using `_internalCallByNamePackageFile`, which is an internal function not intended for manual use.
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/invalid-package-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/invalid-package-name/expected
index aac1b186ca49..80f6e7dd5998 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/invalid-package-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/invalid-package-name/expected
@@ -1,2 +1,2 @@
 pkgs/by-name/fo/fo@: Invalid package directory name "fo@", must be ASCII characters consisting of a-z, A-Z, 0-9, "-" or "_".
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/invalid-shard-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/invalid-shard-name/expected
index ce7c2695424c..7ca9ff8565bd 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/invalid-shard-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/invalid-shard-name/expected
@@ -1,2 +1,2 @@
 pkgs/by-name/A: Invalid directory name "A", must be at most 2 ASCII characters consisting of a-z, 0-9, "-" or "_".
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected b/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
index 71d78019604b..8822e0cc24d4 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
@@ -2,8 +2,7 @@
 
     noEval = callPackage ./pkgs/by-name/no/noEval/package.nix { /* ... */ };
 
-  Notably the second argument must not be empty, which is not the case.
-  It is defined in all-packages.nix:9 as
+  However, in this PR, the second argument is empty. See the definition in all-packages.nix:9:
 
     noEval = self.callPackage ./pkgs/by-name/no/noEval/package.nix { };
 
@@ -11,9 +10,8 @@
 
     onlyMove = callPackage ./pkgs/by-name/on/onlyMove/package.nix { /* ... */ };
 
-  Notably the second argument must not be empty, which is not the case.
-  It is defined in all-packages.nix:7 as
+  However, in this PR, the second argument is empty. See the definition in all-packages.nix:7:
 
     onlyMove = self.callPackage ./pkgs/by-name/on/onlyMove/package.nix { };
 
-This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
+This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/missing-package-nix/expected b/pkgs/test/nixpkgs-check-by-name/tests/missing-package-nix/expected
index 93c882b2bd3c..1b67704817cf 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/missing-package-nix/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/missing-package-nix/expected
@@ -1,2 +1,2 @@
 pkgs/by-name/fo/foo: Missing required "package.nix" file.
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected
index 3319cc271ac9..0e5c07cc04c6 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected
@@ -1,7 +1,7 @@
-- Attribute `pkgs.foo1 was previously defined in pkgs/by-name/fo/foo1/package.nix, but is now manually defined as `callPackage ... { /* ... */ }` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/all-packages.nix.
+- Attribute `pkgs.foo1` was previously defined in pkgs/by-name/fo/foo1/package.nix, but is now manually defined as `callPackage ... { /* ... */ }` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/all-packages.nix.
   Please move the package back and remove the manual `callPackage`.
 
-- Attribute `pkgs.foo2 was previously defined in pkgs/by-name/fo/foo2/package.nix, but is now manually defined as `callPackage ./without-config.nix { /* ... */ }` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/all-packages.nix.
+- Attribute `pkgs.foo2` was previously defined in pkgs/by-name/fo/foo2/package.nix, but is now manually defined as `callPackage ./without-config.nix { /* ... */ }` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/all-packages.nix.
   Please move the package back and remove the manual `callPackage`.
 
 - Attribute `pkgs.foo3` was previously defined in pkgs/by-name/fo/foo3/package.nix, but is now manually defined as `callPackage ... { ... }` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/all-packages.nix.
@@ -10,4 +10,4 @@
 - Attribute `pkgs.foo4` was previously defined in pkgs/by-name/fo/foo4/package.nix, but is now manually defined as `callPackage ./with-config.nix { ... }` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/all-packages.nix.
   While the manual `callPackage` is still needed, it's not necessary to move the package files.
 
-This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
+This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/multiple-failures/expected b/pkgs/test/nixpkgs-check-by-name/tests/multiple-failures/expected
index 303884cb7944..0105b19078c7 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/multiple-failures/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/multiple-failures/expected
@@ -11,4 +11,4 @@ pkgs/by-name/ba/foo: File package.nix at line 3 contains the path expression "..
 pkgs/by-name/ba/foo: File package.nix at line 4 contains the nix search path expression "<nixpkgs>" which may point outside the directory of that package.
 pkgs/by-name/ba/foo: File package.nix at line 5 contains the path expression "./${"test"}", which is not yet supported and may point outside the directory of that package.
 pkgs/by-name/fo/foo: Missing required "package.nix" file.
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected
index a7db19e5fd90..9cfb996f381e 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected
@@ -18,4 +18,4 @@
   See `pkgs/by-name/README.md` for more details.
   Since the second `callPackage` argument is not `{ }`, the manual `callPackage` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/all-packages.nix is still needed.
 
-This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
+This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/non-attrs/expected b/pkgs/test/nixpkgs-check-by-name/tests/non-attrs/expected
index 48acbec1210d..1705d22be798 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/non-attrs/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/non-attrs/expected
@@ -1,2 +1,2 @@
 pkgs.nonDerivation: This attribute defined by pkgs/by-name/no/nonDerivation/package.nix is not a derivation
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/non-derivation/expected b/pkgs/test/nixpkgs-check-by-name/tests/non-derivation/expected
index 48acbec1210d..1705d22be798 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/non-derivation/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/non-derivation/expected
@@ -1,2 +1,2 @@
 pkgs.nonDerivation: This attribute defined by pkgs/by-name/no/nonDerivation/package.nix is not a derivation
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
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 e31aa831b814..3e0ea20c2281 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,4 +2,5 @@ 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/non-syntactical-callPackage-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected
index f56585ac4678..e09e931bb658 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected
@@ -2,9 +2,8 @@
 
     foo = callPackage ./pkgs/by-name/fo/foo/package.nix { /* ... */ };
 
-  This is however not the case.
-  It is defined in all-packages.nix:4 as
+  However, in this PR, it isn't defined that way. See the definition in all-packages.nix:4
 
     foo = self.bar;
 
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
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 571ec8e09a5c..16292c0c0eb1 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
@@ -2,9 +2,8 @@
 
     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
+  However, in this PR, the first `callPackage` argument is the wrong path. See the definition in all-packages.nix:2:
 
     nonDerivation = callPackage ./someDrv.nix { /* ... */ };
 
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected
index 257030e9a6c0..79f78e2437a0 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected
@@ -2,9 +2,8 @@
 
     nonDerivation = callPackage ./pkgs/by-name/no/nonDerivation/package.nix { /* ... */ };
 
-  Notably the second argument must not be empty, which is not the case.
-  It is defined in all-packages.nix:2 as
+  However, in this PR, the second argument is empty. See the definition in all-packages.nix:2:
 
     nonDerivation = self.callPackage ./pkgs/by-name/no/nonDerivation/package.nix { };
 
-This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
+This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected
index eb13c4df1e88..d91d58d629f2 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected
@@ -2,9 +2,8 @@
 
     nonDerivation = callPackage ./pkgs/by-name/no/nonDerivation/package.nix { /* ... */ };
 
-  This is however not the case.
-  It is defined in all-packages.nix:2 as
+  However, in this PR, it isn't defined that way. See the definition in all-packages.nix:2
 
     nonDerivation = self.someDrv;
 
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
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 31679e55b7d6..807c440dd3d2 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
@@ -2,9 +2,8 @@
 
     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
+  However, in this PR, the first `callPackage` argument is not a path. See the definition in all-packages.nix:2:
 
     nonDerivation = self.callPackage ({ someDrv }: someDrv) { };
 
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
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 e7fd8242f5f4..4adbaf66edc0 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
@@ -2,9 +2,8 @@
 
     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
+  However, in this PR, the first `callPackage` argument is not a path. See the definition in all-packages.nix:3:
 
     foo = self.callPackage ({ someDrv, someFlag }: someDrv) { someFlag = true; };
 
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/package-dir-is-file/expected b/pkgs/test/nixpkgs-check-by-name/tests/package-dir-is-file/expected
index 864978b9b731..adee1d0137fa 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/package-dir-is-file/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/package-dir-is-file/expected
@@ -1,2 +1,2 @@
 pkgs/by-name/fo/foo: This path is a file, but it should be a directory.
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/package-nix-dir/expected b/pkgs/test/nixpkgs-check-by-name/tests/package-nix-dir/expected
index 82ad2ce85bbf..d03e1eceea26 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/package-nix-dir/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/package-nix-dir/expected
@@ -1,2 +1,2 @@
 pkgs/by-name/fo/foo: "package.nix" must be a file.
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/ref-absolute/expected b/pkgs/test/nixpkgs-check-by-name/tests/ref-absolute/expected
index 0e0e876d5561..0bdb00f20cb9 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/ref-absolute/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/ref-absolute/expected
@@ -1,2 +1,2 @@
 pkgs/by-name/aa/aa: File package.nix at line 2 contains the path expression "/foo" which cannot be resolved: No such file or directory (os error 2).
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/ref-escape/expected b/pkgs/test/nixpkgs-check-by-name/tests/ref-escape/expected
index f414e4145674..2e4338ccc7ae 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/ref-escape/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/ref-escape/expected
@@ -1,2 +1,2 @@
 pkgs/by-name/aa/aa: File package.nix at line 2 contains the path expression "../." which may point outside the directory of that package.
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/ref-nix-path/expected b/pkgs/test/nixpkgs-check-by-name/tests/ref-nix-path/expected
index 7b47a786a8be..30125570794a 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/ref-nix-path/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/ref-nix-path/expected
@@ -1,2 +1,2 @@
 pkgs/by-name/aa/aa: File package.nix at line 2 contains the nix search path expression "<nixpkgs>" which may point outside the directory of that package.
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/ref-path-subexpr/expected b/pkgs/test/nixpkgs-check-by-name/tests/ref-path-subexpr/expected
index 1905841a63df..6567439b77f8 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/ref-path-subexpr/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/ref-path-subexpr/expected
@@ -1,2 +1,2 @@
 pkgs/by-name/aa/aa: File package.nix at line 2 contains the path expression "./${"test"}", which is not yet supported and may point outside the directory of that package.
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/shard-file/expected b/pkgs/test/nixpkgs-check-by-name/tests/shard-file/expected
index 4a02d99778a9..689cee41f1e3 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/shard-file/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/shard-file/expected
@@ -1,2 +1,2 @@
 pkgs/by-name/fo: This is a file, but it should be a directory.
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected b/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
index 2b3c2cdeee18..8f574d9dddc3 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
@@ -2,8 +2,7 @@
 
     a = callPackage ./pkgs/by-name/a/a/package.nix { /* ... */ };
 
-  Notably the second argument must not be empty, which is not the case.
-  It is defined in all-packages.nix:2 as
+  However, in this PR, the second argument is empty. See the definition in all-packages.nix:2:
 
     a = self.callPackage ./pkgs/by-name/a/a/package.nix { };
 
@@ -16,8 +15,7 @@
 
     c = callPackage ./pkgs/by-name/c/c/package.nix { /* ... */ };
 
-  Notably the second argument must not be empty, which is not the case.
-  It is defined in all-packages.nix:4 as
+  However, in this PR, the second argument is empty. See the definition in all-packages.nix:4:
 
     c = self.callPackage ./pkgs/by-name/c/c/package.nix { };
 
@@ -26,4 +24,4 @@
   See `pkgs/by-name/README.md` for more details.
   Since the second `callPackage` argument is `{ }`, no manual `callPackage` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/all-packages.nix is needed anymore.
 
-This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
+This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/symlink-escape/expected b/pkgs/test/nixpkgs-check-by-name/tests/symlink-escape/expected
index e6c183207e57..cd555c658a09 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/symlink-escape/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/symlink-escape/expected
@@ -1,2 +1,2 @@
 pkgs/by-name/fo/foo: Path package.nix is a symlink pointing to a path outside the directory of that package.
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/symlink-invalid/expected b/pkgs/test/nixpkgs-check-by-name/tests/symlink-invalid/expected
index 01de2702f7d5..1b06bcf4972b 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/symlink-invalid/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/symlink-invalid/expected
@@ -1,2 +1,2 @@
 pkgs/by-name/fo/foo: Path foo is a symlink which cannot be resolved: No such file or directory (os error 2).
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/unknown-location/expected b/pkgs/test/nixpkgs-check-by-name/tests/unknown-location/expected
index d66a3185e701..608843d93903 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/unknown-location/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/unknown-location/expected
@@ -1,2 +1,2 @@
 pkgs.foo: Cannot determine the location of this attribute using `builtins.unsafeGetAttrPos`.
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.

From f056449c462ff6c5f933f3c297d605ff02147a99 Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Fri, 1 Mar 2024 01:42:28 +0100
Subject: [PATCH 15/18] tests.nixpkgs-check-by-name: Fix absolute path in
 errors

---
 pkgs/test/nixpkgs-check-by-name/src/eval.rs   | 23 ++++++++++++++-----
 .../tests/mock-nixpkgs.nix                    | 23 +++++++++++++------
 .../tests/move-to-non-by-name/expected        |  8 +++----
 .../tests/new-package-non-by-name/expected    |  8 +++----
 .../tests/sorted-order/expected               |  4 ++--
 5 files changed, 43 insertions(+), 23 deletions(-)

diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
index a43a0cd743a7..978fe9295336 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
@@ -183,6 +183,7 @@ pub fn check_values(
                     Attribute::NonByName(non_by_name_attribute) => handle_non_by_name_attribute(
                         nixpkgs_path,
                         nix_file_store,
+                        &attribute_name,
                         non_by_name_attribute,
                     )?,
                     Attribute::ByName(by_name_attribute) => by_name(
@@ -431,6 +432,7 @@ fn by_name_override(
 fn handle_non_by_name_attribute(
     nixpkgs_path: &Path,
     nix_file_store: &mut NixFileStore,
+    attribute_name: &str,
     non_by_name_attribute: NonByNameAttribute,
 ) -> validation::Result<ratchet::Package> {
     use ratchet::RatchetState::*;
@@ -481,11 +483,17 @@ fn handle_non_by_name_attribute(
             location: Some(location),
         }) = non_by_name_attribute {
 
-        // 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_location_file = location.relative_file(nixpkgs_path).with_context(|| {
+            format!("Failed to resolve the file where attribute {attribute_name} is defined")
+        })?;
+
+        // Figure out whether it's an attribute definition of the form `= callPackage <arg1> <arg2>`,
         // returning the arguments if so.
-        let (optional_syntactic_call_package, _definition) = nix_file_store
-            .get(&location.file)?
+        let (optional_syntactic_call_package, _definition) = nix_file
             .call_package_argument_info_at(
                 location.line,
                 location.column,
@@ -493,7 +501,10 @@ fn handle_non_by_name_attribute(
                 // strips the absolute Nixpkgs path from it, such that
                 // syntactic_call_package.relative_path is relative to Nixpkgs
                 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`
@@ -529,7 +540,7 @@ fn handle_non_by_name_attribute(
                     _ => {
                         // Otherwise, the path is outside `pkgs/by-name`, which means it can be
                         // migrated
-                        Loose((syntactic_call_package, location.file))
+                        Loose((syntactic_call_package, relative_location_file))
                     }
                 }
             }
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/mock-nixpkgs.nix b/pkgs/test/nixpkgs-check-by-name/tests/mock-nixpkgs.nix
index 81a9c916ac2d..fbd51f656138 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/mock-nixpkgs.nix
+++ b/pkgs/test/nixpkgs-check-by-name/tests/mock-nixpkgs.nix
@@ -28,13 +28,22 @@ let
   lib = import <test-nixpkgs/lib>;
 
   # The base fixed-point function to populate the resulting attribute set
-  pkgsFun = self: {
-    inherit lib;
-    newScope = extra: lib.callPackageWith (self // extra);
-    callPackage = self.newScope { };
-    callPackages = lib.callPackagesWith self;
-    someDrv = { type = "derivation"; };
-  };
+  pkgsFun =
+    self: {
+      inherit lib;
+      newScope = extra: lib.callPackageWith (self // extra);
+      callPackage = self.newScope { };
+      callPackages = lib.callPackagesWith self;
+    }
+    # This mapAttrs is a very hacky workaround necessary because for all attributes defined in Nixpkgs,
+    # the files that define them are verified to be within Nixpkgs.
+    # This is usually a very safe assumption, but it fails in these tests for someDrv,
+    # because it's technically defined outside the Nixpkgs directories of each test case.
+    # By using `mapAttrs`, `builtins.unsafeGetAttrPos` just returns `null`,
+    # which then doesn't trigger this check
+    // lib.mapAttrs (name: value: value) {
+      someDrv = { type = "derivation"; };
+    };
 
   baseDirectory = root + "/pkgs/by-name";
 
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected
index 0e5c07cc04c6..123e24daab8a 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected
@@ -1,13 +1,13 @@
-- Attribute `pkgs.foo1` was previously defined in pkgs/by-name/fo/foo1/package.nix, but is now manually defined as `callPackage ... { /* ... */ }` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/all-packages.nix.
+- Attribute `pkgs.foo1` was previously defined in pkgs/by-name/fo/foo1/package.nix, but is now manually defined as `callPackage ... { /* ... */ }` in all-packages.nix.
   Please move the package back and remove the manual `callPackage`.
 
-- Attribute `pkgs.foo2` was previously defined in pkgs/by-name/fo/foo2/package.nix, but is now manually defined as `callPackage ./without-config.nix { /* ... */ }` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/all-packages.nix.
+- Attribute `pkgs.foo2` was previously defined in pkgs/by-name/fo/foo2/package.nix, but is now manually defined as `callPackage ./without-config.nix { /* ... */ }` in all-packages.nix.
   Please move the package back and remove the manual `callPackage`.
 
-- Attribute `pkgs.foo3` was previously defined in pkgs/by-name/fo/foo3/package.nix, but is now manually defined as `callPackage ... { ... }` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/all-packages.nix.
+- Attribute `pkgs.foo3` was previously defined in pkgs/by-name/fo/foo3/package.nix, but is now manually defined as `callPackage ... { ... }` in all-packages.nix.
   While the manual `callPackage` is still needed, it's not necessary to move the package files.
 
-- Attribute `pkgs.foo4` was previously defined in pkgs/by-name/fo/foo4/package.nix, but is now manually defined as `callPackage ./with-config.nix { ... }` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/all-packages.nix.
+- Attribute `pkgs.foo4` was previously defined in pkgs/by-name/fo/foo4/package.nix, but is now manually defined as `callPackage ./with-config.nix { ... }` in all-packages.nix.
   While the manual `callPackage` is still needed, it's not necessary to move the package files.
 
 This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected
index 9cfb996f381e..92668a231b48 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected
@@ -1,21 +1,21 @@
 - Attribute `pkgs.new1` is a new top-level package using `pkgs.callPackage ... { /* ... */ }`.
   Please define it in pkgs/by-name/ne/new1/package.nix instead.
   See `pkgs/by-name/README.md` for more details.
-  Since the second `callPackage` argument is `{ }`, no manual `callPackage` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/all-packages.nix is needed anymore.
+  Since the second `callPackage` argument is `{ }`, no manual `callPackage` in all-packages.nix is needed anymore.
 
 - Attribute `pkgs.new2` is a new top-level package using `pkgs.callPackage ./without-config.nix { /* ... */ }`.
   Please define it in pkgs/by-name/ne/new2/package.nix instead.
   See `pkgs/by-name/README.md` for more details.
-  Since the second `callPackage` argument is `{ }`, no manual `callPackage` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/all-packages.nix is needed anymore.
+  Since the second `callPackage` argument is `{ }`, no manual `callPackage` in all-packages.nix is needed anymore.
 
 - Attribute `pkgs.new3` is a new top-level package using `pkgs.callPackage ... { /* ... */ }`.
   Please define it in pkgs/by-name/ne/new3/package.nix instead.
   See `pkgs/by-name/README.md` for more details.
-  Since the second `callPackage` argument is not `{ }`, the manual `callPackage` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/all-packages.nix is still needed.
+  Since the second `callPackage` argument is not `{ }`, the manual `callPackage` in all-packages.nix is still needed.
 
 - Attribute `pkgs.new4` is a new top-level package using `pkgs.callPackage ./with-config.nix { /* ... */ }`.
   Please define it in pkgs/by-name/ne/new4/package.nix instead.
   See `pkgs/by-name/README.md` for more details.
-  Since the second `callPackage` argument is not `{ }`, the manual `callPackage` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/all-packages.nix is still needed.
+  Since the second `callPackage` argument is not `{ }`, the manual `callPackage` in all-packages.nix is still needed.
 
 This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected b/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
index 8f574d9dddc3..9c7b0d22a610 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
@@ -9,7 +9,7 @@
 - Attribute `pkgs.b` is a new top-level package using `pkgs.callPackage ... { /* ... */ }`.
   Please define it in pkgs/by-name/b/b/package.nix instead.
   See `pkgs/by-name/README.md` for more details.
-  Since the second `callPackage` argument is `{ }`, no manual `callPackage` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/all-packages.nix is needed anymore.
+  Since the second `callPackage` argument is `{ }`, no manual `callPackage` in all-packages.nix is needed anymore.
 
 - Because pkgs/by-name/c/c exists, the attribute `pkgs.c` must be defined like
 
@@ -22,6 +22,6 @@
 - Attribute `pkgs.d` is a new top-level package using `pkgs.callPackage ... { /* ... */ }`.
   Please define it in pkgs/by-name/d/d/package.nix instead.
   See `pkgs/by-name/README.md` for more details.
-  Since the second `callPackage` argument is `{ }`, no manual `callPackage` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/all-packages.nix is needed anymore.
+  Since the second `callPackage` argument is `{ }`, no manual `callPackage` in all-packages.nix is needed anymore.
 
 This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.

From 7858f062884e2318dda212ab7cc1ceda871195b2 Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Fri, 1 Mar 2024 01:50:05 +0100
Subject: [PATCH 16/18] tests.nixpkgs-check-by-name: Better empty argument
 error

---
 pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs        | 2 ++
 .../nixpkgs-check-by-name/tests/manual-definition/expected    | 4 ++++
 .../nixpkgs-check-by-name/tests/override-empty-arg/expected   | 2 ++
 pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected   | 4 ++++
 4 files changed, 12 insertions(+)

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 03203073fd4a..498284ac6f24 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
@@ -213,6 +213,8 @@ impl fmt::Display for NixpkgsProblem {
                       However, in this PR, the second argument is empty. See the definition in {}:{}:
 
                     {}
+
+                      Such a definition is provided automatically and therefore not necessary. Please remove it.
                     ",
                     structure::relative_dir_for_package(package_name).display(),
                     structure::relative_file_for_package(package_name).display(),
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected b/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
index 8822e0cc24d4..4d906ec0d086 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
@@ -6,6 +6,8 @@
 
     noEval = self.callPackage ./pkgs/by-name/no/noEval/package.nix { };
 
+  Such a definition is provided automatically and therefore not necessary. Please remove it.
+
 - Because pkgs/by-name/on/onlyMove exists, the attribute `pkgs.onlyMove` must be defined like
 
     onlyMove = callPackage ./pkgs/by-name/on/onlyMove/package.nix { /* ... */ };
@@ -14,4 +16,6 @@
 
     onlyMove = self.callPackage ./pkgs/by-name/on/onlyMove/package.nix { };
 
+  Such a definition is provided automatically and therefore not necessary. Please remove it.
+
 This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected
index 79f78e2437a0..f3306aadbbb7 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected
@@ -6,4 +6,6 @@
 
     nonDerivation = self.callPackage ./pkgs/by-name/no/nonDerivation/package.nix { };
 
+  Such a definition is provided automatically and therefore not necessary. Please remove it.
+
 This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected b/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
index 9c7b0d22a610..8a8104b73720 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
@@ -6,6 +6,8 @@
 
     a = self.callPackage ./pkgs/by-name/a/a/package.nix { };
 
+  Such a definition is provided automatically and therefore not necessary. Please remove it.
+
 - Attribute `pkgs.b` is a new top-level package using `pkgs.callPackage ... { /* ... */ }`.
   Please define it in pkgs/by-name/b/b/package.nix instead.
   See `pkgs/by-name/README.md` for more details.
@@ -19,6 +21,8 @@
 
     c = self.callPackage ./pkgs/by-name/c/c/package.nix { };
 
+  Such a definition is provided automatically and therefore not necessary. Please remove it.
+
 - Attribute `pkgs.d` is a new top-level package using `pkgs.callPackage ... { /* ... */ }`.
   Please define it in pkgs/by-name/d/d/package.nix instead.
   See `pkgs/by-name/README.md` for more details.

From 5981aff2125baba1430e5a96338a71e2e1fad18d Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Fri, 1 Mar 2024 02:29:24 +0100
Subject: [PATCH 17/18] tests.nixpkgs-check-by-name: Use RelativePath for
 relative paths

Makes the code easier to understand and less error-prone
---
 pkgs/test/nixpkgs-check-by-name/src/eval.rs   |  26 ++-
 .../nixpkgs-check-by-name/src/nix_file.rs     |  17 +-
 .../src/nixpkgs_problem.rs                    | 196 +++++++++---------
 .../test/nixpkgs-check-by-name/src/ratchet.rs |   4 +-
 .../nixpkgs-check-by-name/src/references.rs   |  52 ++---
 .../nixpkgs-check-by-name/src/structure.rs    |  16 +-
 6 files changed, 158 insertions(+), 153 deletions(-)

diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
index 978fe9295336..094508f595d8 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
@@ -8,6 +8,7 @@ use crate::utils;
 use crate::validation::ResultIteratorExt as _;
 use crate::validation::{self, Validation::Success};
 use crate::NixFileStore;
+use relative_path::RelativePathBuf;
 use std::path::Path;
 
 use anyhow::Context;
@@ -56,18 +57,15 @@ struct Location {
 
 impl Location {
     // 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())
+    fn relative_file(&self, nixpkgs_path: &Path) -> anyhow::Result<RelativePathBuf> {
+        let path = self.file.strip_prefix(nixpkgs_path).with_context(|| {
+            format!(
+                "The file ({}) is outside Nixpkgs ({})",
+                self.file.display(),
+                nixpkgs_path.display()
+            )
+        })?;
+        Ok(RelativePathBuf::from_path(path).expect("relative path"))
     }
 }
 
@@ -352,12 +350,12 @@ fn by_name(
 /// all-packages.nix
 fn by_name_override(
     attribute_name: &str,
-    expected_package_file: PathBuf,
+    expected_package_file: RelativePathBuf,
     is_semantic_call_package: bool,
     optional_syntactic_call_package: Option<CallPackageArgumentInfo>,
     definition: String,
     location: Location,
-    relative_location_file: PathBuf,
+    relative_location_file: RelativePathBuf,
 ) -> validation::Validation<ratchet::RatchetState<ratchet::ManualDefinition>> {
     // At this point, we completed two different checks for whether it's a
     // `callPackage`
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 1cb5d1c0d08d..e2dc1e196141 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs
@@ -3,6 +3,7 @@
 use crate::utils::LineIndex;
 use anyhow::Context;
 use itertools::Either::{self, Left, Right};
+use relative_path::RelativePathBuf;
 use rnix::ast;
 use rnix::ast::Expr;
 use rnix::ast::HasEntry;
@@ -79,7 +80,7 @@ impl NixFile {
 #[derive(Debug, PartialEq)]
 pub struct CallPackageArgumentInfo {
     /// The relative path of the first argument, or `None` if it's not a path.
-    pub relative_path: Option<PathBuf>,
+    pub relative_path: Option<RelativePathBuf>,
     /// Whether the second argument is an empty attribute set
     pub empty_arg: bool,
 }
@@ -369,8 +370,8 @@ pub enum ResolvedPath {
     /// The path is outside the given absolute path
     Outside,
     /// The path is within the given absolute path.
-    /// The `PathBuf` is the relative path under the given absolute path.
-    Within(PathBuf),
+    /// The `RelativePathBuf` is the relative path under the given absolute path.
+    Within(RelativePathBuf),
 }
 
 impl NixFile {
@@ -402,7 +403,9 @@ impl NixFile {
                 // 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()),
+                    Ok(suffix) => ResolvedPath::Within(
+                        RelativePathBuf::from_path(suffix).expect("a relative path"),
+                    ),
                 }
             }
         }
@@ -506,14 +509,14 @@ mod tests {
             (
                 6,
                 Some(CallPackageArgumentInfo {
-                    relative_path: Some(PathBuf::from("file.nix")),
+                    relative_path: Some(RelativePathBuf::from("file.nix")),
                     empty_arg: true,
                 }),
             ),
             (
                 7,
                 Some(CallPackageArgumentInfo {
-                    relative_path: Some(PathBuf::from("file.nix")),
+                    relative_path: Some(RelativePathBuf::from("file.nix")),
                     empty_arg: true,
                 }),
             ),
@@ -527,7 +530,7 @@ mod tests {
             (
                 9,
                 Some(CallPackageArgumentInfo {
-                    relative_path: Some(PathBuf::from("file.nix")),
+                    relative_path: Some(RelativePathBuf::from("file.nix")),
                     empty_arg: false,
                 }),
             ),
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 498284ac6f24..f9e232b93933 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
@@ -2,139 +2,140 @@ use crate::structure;
 use crate::utils::PACKAGE_NIX_FILENAME;
 use indoc::writedoc;
 use relative_path::RelativePath;
+use relative_path::RelativePathBuf;
 use std::ffi::OsString;
 use std::fmt;
-use std::path::Path;
-use std::path::PathBuf;
 
 /// Any problem that can occur when checking Nixpkgs
+/// All paths are relative to Nixpkgs such that the error messages can't be influenced by Nixpkgs absolute
+/// location
 #[derive(Clone)]
 pub enum NixpkgsProblem {
     ShardNonDir {
-        relative_shard_path: PathBuf,
+        relative_shard_path: RelativePathBuf,
     },
     InvalidShardName {
-        relative_shard_path: PathBuf,
+        relative_shard_path: RelativePathBuf,
         shard_name: String,
     },
     PackageNonDir {
-        relative_package_dir: PathBuf,
+        relative_package_dir: RelativePathBuf,
     },
     CaseSensitiveDuplicate {
-        relative_shard_path: PathBuf,
+        relative_shard_path: RelativePathBuf,
         first: OsString,
         second: OsString,
     },
     InvalidPackageName {
-        relative_package_dir: PathBuf,
+        relative_package_dir: RelativePathBuf,
         package_name: String,
     },
     IncorrectShard {
-        relative_package_dir: PathBuf,
-        correct_relative_package_dir: PathBuf,
+        relative_package_dir: RelativePathBuf,
+        correct_relative_package_dir: RelativePathBuf,
     },
     PackageNixNonExistent {
-        relative_package_dir: PathBuf,
+        relative_package_dir: RelativePathBuf,
     },
     PackageNixDir {
-        relative_package_dir: PathBuf,
+        relative_package_dir: RelativePathBuf,
     },
     UndefinedAttr {
-        relative_package_file: PathBuf,
+        relative_package_file: RelativePathBuf,
         package_name: String,
     },
     EmptyArgument {
         package_name: String,
-        file: PathBuf,
+        file: RelativePathBuf,
         line: usize,
         column: usize,
         definition: String,
     },
     NonToplevelCallPackage {
         package_name: String,
-        file: PathBuf,
+        file: RelativePathBuf,
         line: usize,
         column: usize,
         definition: String,
     },
     NonPath {
         package_name: String,
-        file: PathBuf,
+        file: RelativePathBuf,
         line: usize,
         column: usize,
         definition: String,
     },
     WrongCallPackagePath {
         package_name: String,
-        file: PathBuf,
+        file: RelativePathBuf,
         line: usize,
-        actual_path: PathBuf,
-        expected_path: PathBuf,
+        actual_path: RelativePathBuf,
+        expected_path: RelativePathBuf,
     },
     NonSyntacticCallPackage {
         package_name: String,
-        file: PathBuf,
+        file: RelativePathBuf,
         line: usize,
         column: usize,
         definition: String,
     },
     NonDerivation {
-        relative_package_file: PathBuf,
+        relative_package_file: RelativePathBuf,
         package_name: String,
     },
     OutsideSymlink {
-        relative_package_dir: PathBuf,
-        subpath: PathBuf,
+        relative_package_dir: RelativePathBuf,
+        subpath: RelativePathBuf,
     },
     UnresolvableSymlink {
-        relative_package_dir: PathBuf,
-        subpath: PathBuf,
+        relative_package_dir: RelativePathBuf,
+        subpath: RelativePathBuf,
         io_error: String,
     },
     PathInterpolation {
-        relative_package_dir: PathBuf,
-        subpath: PathBuf,
+        relative_package_dir: RelativePathBuf,
+        subpath: RelativePathBuf,
         line: usize,
         text: String,
     },
     SearchPath {
-        relative_package_dir: PathBuf,
-        subpath: PathBuf,
+        relative_package_dir: RelativePathBuf,
+        subpath: RelativePathBuf,
         line: usize,
         text: String,
     },
     OutsidePathReference {
-        relative_package_dir: PathBuf,
-        subpath: PathBuf,
+        relative_package_dir: RelativePathBuf,
+        subpath: RelativePathBuf,
         line: usize,
         text: String,
     },
     UnresolvablePathReference {
-        relative_package_dir: PathBuf,
-        subpath: PathBuf,
+        relative_package_dir: RelativePathBuf,
+        subpath: RelativePathBuf,
         line: usize,
         text: String,
         io_error: String,
     },
     MovedOutOfByNameEmptyArg {
         package_name: String,
-        call_package_path: Option<PathBuf>,
-        file: PathBuf,
+        call_package_path: Option<RelativePathBuf>,
+        file: RelativePathBuf,
     },
     MovedOutOfByNameNonEmptyArg {
         package_name: String,
-        call_package_path: Option<PathBuf>,
-        file: PathBuf,
+        call_package_path: Option<RelativePathBuf>,
+        file: RelativePathBuf,
     },
     NewPackageNotUsingByNameEmptyArg {
         package_name: String,
-        call_package_path: Option<PathBuf>,
-        file: PathBuf,
+        call_package_path: Option<RelativePathBuf>,
+        file: RelativePathBuf,
     },
     NewPackageNotUsingByNameNonEmptyArg {
         package_name: String,
-        call_package_path: Option<PathBuf>,
-        file: PathBuf,
+        call_package_path: Option<RelativePathBuf>,
+        file: RelativePathBuf,
     },
     InternalCallPackageUsed {
         attr_name: String,
@@ -151,56 +152,56 @@ impl fmt::Display for NixpkgsProblem {
                 write!(
                     f,
                     "{}: This is a file, but it should be a directory.",
-                    relative_shard_path.display(),
+                    relative_shard_path,
                 ),
             NixpkgsProblem::InvalidShardName { relative_shard_path, shard_name } =>
                 write!(
                     f,
                     "{}: Invalid directory name \"{shard_name}\", must be at most 2 ASCII characters consisting of a-z, 0-9, \"-\" or \"_\".",
-                    relative_shard_path.display()
+                    relative_shard_path,
                 ),
             NixpkgsProblem::PackageNonDir { relative_package_dir } =>
                 write!(
                     f,
                     "{}: This path is a file, but it should be a directory.",
-                    relative_package_dir.display(),
+                    relative_package_dir,
                 ),
             NixpkgsProblem::CaseSensitiveDuplicate { relative_shard_path, first, second } =>
                 write!(
                     f,
                     "{}: Duplicate case-sensitive package directories {first:?} and {second:?}.",
-                    relative_shard_path.display(),
+                    relative_shard_path,
                 ),
             NixpkgsProblem::InvalidPackageName { relative_package_dir, package_name } =>
                 write!(
                     f,
                     "{}: Invalid package directory name \"{package_name}\", must be ASCII characters consisting of a-z, A-Z, 0-9, \"-\" or \"_\".",
-                    relative_package_dir.display(),
+                    relative_package_dir,
                 ),
             NixpkgsProblem::IncorrectShard { relative_package_dir, correct_relative_package_dir } =>
                 write!(
                     f,
                     "{}: Incorrect directory location, should be {} instead.",
-                    relative_package_dir.display(),
-                    correct_relative_package_dir.display(),
+                    relative_package_dir,
+                    correct_relative_package_dir,
                 ),
             NixpkgsProblem::PackageNixNonExistent { relative_package_dir } =>
                 write!(
                     f,
                     "{}: Missing required \"{PACKAGE_NIX_FILENAME}\" file.",
-                    relative_package_dir.display(),
+                    relative_package_dir,
                 ),
             NixpkgsProblem::PackageNixDir { relative_package_dir } =>
                 write!(
                     f,
                     "{}: \"{PACKAGE_NIX_FILENAME}\" must be a file.",
-                    relative_package_dir.display(),
+                    relative_package_dir,
                 ),
             NixpkgsProblem::UndefinedAttr { relative_package_file, package_name } =>
                 write!(
                     f,
                     "pkgs.{package_name}: This attribute is not defined but it should be defined automatically as {}",
-                    relative_package_file.display()
+                    relative_package_file,
                 ),
             NixpkgsProblem::EmptyArgument { package_name, file, line, column, definition } =>
                 writedoc!(
@@ -216,9 +217,9 @@ impl fmt::Display for NixpkgsProblem {
 
                       Such a definition is provided automatically and therefore not necessary. Please remove it.
                     ",
-                    structure::relative_dir_for_package(package_name).display(),
-                    structure::relative_file_for_package(package_name).display(),
-                    file.display(),
+                    structure::relative_dir_for_package(package_name),
+                    structure::relative_file_for_package(package_name),
+                    file,
                     line,
                     indent_definition(*column, definition.clone()),
                 ),
@@ -234,9 +235,9 @@ impl fmt::Display for NixpkgsProblem {
 
                     {}
                     ",
-                    structure::relative_dir_for_package(package_name).display(),
-                    structure::relative_file_for_package(package_name).display(),
-                    file.display(),
+                    structure::relative_dir_for_package(package_name),
+                    structure::relative_file_for_package(package_name),
+                    file,
                     line,
                     indent_definition(*column, definition.clone()),
                 ),
@@ -252,9 +253,9 @@ impl fmt::Display for NixpkgsProblem {
 
                     {}
                     ",
-                    structure::relative_dir_for_package(package_name).display(),
-                    structure::relative_file_for_package(package_name).display(),
-                    file.display(),
+                    structure::relative_dir_for_package(package_name),
+                    structure::relative_file_for_package(package_name),
+                    file,
                     line,
                     indent_definition(*column, definition.clone()),
                 ),
@@ -270,9 +271,9 @@ impl fmt::Display for NixpkgsProblem {
 
                         {package_name} = callPackage {} {{ /* ... */ }};
                     ",
-                    structure::relative_dir_for_package(package_name).display(),
+                    structure::relative_dir_for_package(package_name),
                     create_path_expr(file, expected_path),
-                    file.display(), line,
+                    file, line,
                     create_path_expr(file, actual_path),
                 },
             NixpkgsProblem::NonSyntacticCallPackage { package_name, file, line, column, definition } => {
@@ -287,9 +288,9 @@ impl fmt::Display for NixpkgsProblem {
 
                     {}
                     ",
-                    structure::relative_dir_for_package(package_name).display(),
-                    structure::relative_file_for_package(package_name).display(),
-                    file.display(),
+                    structure::relative_dir_for_package(package_name),
+                    structure::relative_file_for_package(package_name),
+                    file,
                     line,
                     indent_definition(*column, definition.clone()),
                 )
@@ -298,58 +299,58 @@ impl fmt::Display for NixpkgsProblem {
                 write!(
                     f,
                     "pkgs.{package_name}: This attribute defined by {} is not a derivation",
-                    relative_package_file.display()
+                    relative_package_file,
                 ),
             NixpkgsProblem::OutsideSymlink { relative_package_dir, subpath } =>
                 write!(
                     f,
                     "{}: Path {} is a symlink pointing to a path outside the directory of that package.",
-                    relative_package_dir.display(),
-                    subpath.display(),
+                    relative_package_dir,
+                    subpath,
                 ),
             NixpkgsProblem::UnresolvableSymlink { relative_package_dir, subpath, io_error } =>
                 write!(
                     f,
                     "{}: Path {} is a symlink which cannot be resolved: {io_error}.",
-                    relative_package_dir.display(),
-                    subpath.display(),
+                    relative_package_dir,
+                    subpath,
                 ),
             NixpkgsProblem::PathInterpolation { relative_package_dir, subpath, line, text } =>
                 write!(
                     f,
                     "{}: File {} at line {line} contains the path expression \"{}\", which is not yet supported and may point outside the directory of that package.",
-                    relative_package_dir.display(),
-                    subpath.display(),
+                    relative_package_dir,
+                    subpath,
                     text
                 ),
             NixpkgsProblem::SearchPath { relative_package_dir, subpath, line, text } =>
                 write!(
                     f,
                     "{}: File {} at line {line} contains the nix search path expression \"{}\" which may point outside the directory of that package.",
-                    relative_package_dir.display(),
-                    subpath.display(),
+                    relative_package_dir,
+                    subpath,
                     text
                 ),
             NixpkgsProblem::OutsidePathReference { relative_package_dir, subpath, line, text } =>
                 write!(
                     f,
                     "{}: File {} at line {line} contains the path expression \"{}\" which may point outside the directory of that package.",
-                    relative_package_dir.display(),
-                    subpath.display(),
+                    relative_package_dir,
+                    subpath,
                     text,
                 ),
             NixpkgsProblem::UnresolvablePathReference { relative_package_dir, subpath, line, text, io_error } =>
                 write!(
                     f,
                     "{}: File {} at line {line} contains the path expression \"{}\" which cannot be resolved: {io_error}.",
-                    relative_package_dir.display(),
-                    subpath.display(),
+                    relative_package_dir,
+                    subpath,
                     text,
                 ),
             NixpkgsProblem::MovedOutOfByNameEmptyArg { package_name, call_package_path, file } => {
                 let call_package_arg =
                     if let Some(path) = &call_package_path {
-                        format!("./{}", path.display())
+                        format!("./{}", path)
                     } else {
                         "...".into()
                     };
@@ -359,14 +360,14 @@ impl fmt::Display for NixpkgsProblem {
                     - Attribute `pkgs.{package_name}` was previously defined in {}, but is now manually defined as `callPackage {call_package_arg} {{ /* ... */ }}` in {}.
                       Please move the package back and remove the manual `callPackage`.
                     ",
-                    structure::relative_file_for_package(package_name).display(),
-                    file.display(),
+                    structure::relative_file_for_package(package_name),
+                    file,
                     )
             },
             NixpkgsProblem::MovedOutOfByNameNonEmptyArg { package_name, call_package_path, file } => {
                 let call_package_arg =
                     if let Some(path) = &call_package_path {
-                        format!("./{}", path.display())
+                        format!("./{}", path)
                     } else {
                         "...".into()
                     };
@@ -378,14 +379,14 @@ impl fmt::Display for NixpkgsProblem {
                     - Attribute `pkgs.{package_name}` was previously defined in {}, but is now manually defined as `callPackage {call_package_arg} {{ ... }}` in {}.
                       While the manual `callPackage` is still needed, it's not necessary to move the package files.
                     ",
-                    structure::relative_file_for_package(package_name).display(),
-                    file.display(),
+                    structure::relative_file_for_package(package_name),
+                    file,
                     )
             },
             NixpkgsProblem::NewPackageNotUsingByNameEmptyArg { package_name, call_package_path, file } => {
                 let call_package_arg =
                     if let Some(path) = &call_package_path {
-                        format!("./{}", path.display())
+                        format!("./{}", path)
                     } else {
                         "...".into()
                     };
@@ -397,14 +398,14 @@ impl fmt::Display for NixpkgsProblem {
                       See `pkgs/by-name/README.md` for more details.
                       Since the second `callPackage` argument is `{{ }}`, no manual `callPackage` in {} is needed anymore.
                     ",
-                    structure::relative_file_for_package(package_name).display(),
-                    file.display(),
+                    structure::relative_file_for_package(package_name),
+                    file,
                 )
             },
             NixpkgsProblem::NewPackageNotUsingByNameNonEmptyArg { package_name, call_package_path, file } => {
                 let call_package_arg =
                     if let Some(path) = &call_package_path {
-                        format!("./{}", path.display())
+                        format!("./{}", path)
                     } else {
                         "...".into()
                     };
@@ -416,8 +417,8 @@ impl fmt::Display for NixpkgsProblem {
                       See `pkgs/by-name/README.md` for more details.
                       Since the second `callPackage` argument is not `{{ }}`, the manual `callPackage` in {} is still needed.
                     ",
-                    structure::relative_file_for_package(package_name).display(),
-                    file.display(),
+                    structure::relative_file_for_package(package_name),
+                    file,
                 )
             },
             NixpkgsProblem::InternalCallPackageUsed { attr_name } =>
@@ -448,14 +449,13 @@ fn indent_definition(column: usize, definition: String) -> String {
 }
 
 /// 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 {
-    // These `expect` calls should never trigger because we only call this function with
-    // relative paths that have a parent. That's why we `expect` them!
-    let from = RelativePath::from_path(&from_file)
-        .expect("a relative path")
-        .parent()
-        .expect("a parent for this path");
-    let to = RelativePath::from_path(&to_file).expect("a path");
-    let rel = from.relative(to);
+fn create_path_expr(
+    from_file: impl AsRef<RelativePath>,
+    to_file: impl AsRef<RelativePath>,
+) -> String {
+    // This `expect` calls should never trigger because we only call this function with files.
+    // That's why we `expect` them!
+    let from = from_file.as_ref().parent().expect("a parent for this path");
+    let rel = from.relative(to_file);
     format!("./{rel}")
 }
diff --git a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
index eed5071a5a2a..8136d641c351 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
@@ -5,8 +5,8 @@
 use crate::nix_file::CallPackageArgumentInfo;
 use crate::nixpkgs_problem::NixpkgsProblem;
 use crate::validation::{self, Validation, Validation::Success};
+use relative_path::RelativePathBuf;
 use std::collections::HashMap;
-use std::path::PathBuf;
 
 /// The ratchet value for the entirety of Nixpkgs.
 #[derive(Default)]
@@ -146,7 +146,7 @@ impl ToNixpkgsProblem for ManualDefinition {
 pub enum UsesByName {}
 
 impl ToNixpkgsProblem for UsesByName {
-    type ToContext = (CallPackageArgumentInfo, PathBuf);
+    type ToContext = (CallPackageArgumentInfo, RelativePathBuf);
 
     fn to_nixpkgs_problem(
         name: &str,
diff --git a/pkgs/test/nixpkgs-check-by-name/src/references.rs b/pkgs/test/nixpkgs-check-by-name/src/references.rs
index b716b99099c4..e2319163ccc6 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/references.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/references.rs
@@ -2,6 +2,7 @@ use crate::nixpkgs_problem::NixpkgsProblem;
 use crate::utils;
 use crate::validation::{self, ResultIteratorExt, Validation::Success};
 use crate::NixFileStore;
+use relative_path::RelativePath;
 
 use anyhow::Context;
 use rowan::ast::AstNode;
@@ -12,14 +13,14 @@ use std::path::Path;
 /// Both symlinks and Nix path expressions are checked.
 pub fn check_references(
     nix_file_store: &mut NixFileStore,
-    relative_package_dir: &Path,
+    relative_package_dir: &RelativePath,
     absolute_package_dir: &Path,
 ) -> validation::Result<()> {
     // 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("");
+    let subpath = RelativePath::new("");
     check_path(
         nix_file_store,
         relative_package_dir,
@@ -29,7 +30,7 @@ pub fn check_references(
     .with_context(|| {
         format!(
             "While checking the references in package directory {}",
-            relative_package_dir.display()
+            relative_package_dir
         )
     })
 }
@@ -41,11 +42,11 @@ pub fn check_references(
 /// The absolute package directory gets prepended before doing anything with it though.
 fn check_path(
     nix_file_store: &mut NixFileStore,
-    relative_package_dir: &Path,
+    relative_package_dir: &RelativePath,
     absolute_package_dir: &Path,
-    subpath: &Path,
+    subpath: &RelativePath,
 ) -> validation::Result<()> {
-    let path = absolute_package_dir.join(subpath);
+    let path = subpath.to_path(absolute_package_dir);
 
     Ok(if path.is_symlink() {
         // Check whether the symlink resolves to outside the package directory
@@ -55,8 +56,8 @@ fn check_path(
                 // entire directory recursively anyways
                 if let Err(_prefix_error) = target.strip_prefix(absolute_package_dir) {
                     NixpkgsProblem::OutsideSymlink {
-                        relative_package_dir: relative_package_dir.to_path_buf(),
-                        subpath: subpath.to_path_buf(),
+                        relative_package_dir: relative_package_dir.to_owned(),
+                        subpath: subpath.to_owned(),
                     }
                     .into()
                 } else {
@@ -64,8 +65,8 @@ fn check_path(
                 }
             }
             Err(io_error) => NixpkgsProblem::UnresolvableSymlink {
-                relative_package_dir: relative_package_dir.to_path_buf(),
-                subpath: subpath.to_path_buf(),
+                relative_package_dir: relative_package_dir.to_owned(),
+                subpath: subpath.to_owned(),
                 io_error: io_error.to_string(),
             }
             .into(),
@@ -80,11 +81,12 @@ fn check_path(
                         nix_file_store,
                         relative_package_dir,
                         absolute_package_dir,
-                        &subpath.join(entry.file_name()),
+                        // TODO: The relative_path crate doesn't seem to support OsStr
+                        &subpath.join(entry.file_name().to_string_lossy().to_string()),
                     )
                 })
                 .collect_vec()
-                .with_context(|| format!("Error while recursing into {}", subpath.display()))?,
+                .with_context(|| format!("Error while recursing into {}", subpath))?,
         )
     } else if path.is_file() {
         // Only check Nix files
@@ -96,7 +98,7 @@ fn check_path(
                     absolute_package_dir,
                     subpath,
                 )
-                .with_context(|| format!("Error while checking Nix file {}", subpath.display()))?
+                .with_context(|| format!("Error while checking Nix file {}", subpath))?
             } else {
                 Success(())
             }
@@ -105,7 +107,7 @@ fn check_path(
         }
     } else {
         // This should never happen, git doesn't support other file types
-        anyhow::bail!("Unsupported file type for path {}", subpath.display());
+        anyhow::bail!("Unsupported file type for path {}", subpath);
     })
 }
 
@@ -113,11 +115,11 @@ fn check_path(
 /// directory
 fn check_nix_file(
     nix_file_store: &mut NixFileStore,
-    relative_package_dir: &Path,
+    relative_package_dir: &RelativePath,
     absolute_package_dir: &Path,
-    subpath: &Path,
+    subpath: &RelativePath,
 ) -> validation::Result<()> {
-    let path = absolute_package_dir.join(subpath);
+    let path = subpath.to_path(absolute_package_dir);
 
     let nix_file = nix_file_store.get(&path)?;
 
@@ -135,29 +137,29 @@ fn check_nix_file(
 
             match nix_file.static_resolve_path(path, absolute_package_dir) {
                 ResolvedPath::Interpolated => NixpkgsProblem::PathInterpolation {
-                    relative_package_dir: relative_package_dir.to_path_buf(),
-                    subpath: subpath.to_path_buf(),
+                    relative_package_dir: relative_package_dir.to_owned(),
+                    subpath: subpath.to_owned(),
                     line,
                     text,
                 }
                 .into(),
                 ResolvedPath::SearchPath => NixpkgsProblem::SearchPath {
-                    relative_package_dir: relative_package_dir.to_path_buf(),
-                    subpath: subpath.to_path_buf(),
+                    relative_package_dir: relative_package_dir.to_owned(),
+                    subpath: subpath.to_owned(),
                     line,
                     text,
                 }
                 .into(),
                 ResolvedPath::Outside => NixpkgsProblem::OutsidePathReference {
-                    relative_package_dir: relative_package_dir.to_path_buf(),
-                    subpath: subpath.to_path_buf(),
+                    relative_package_dir: relative_package_dir.to_owned(),
+                    subpath: subpath.to_owned(),
                     line,
                     text,
                 }
                 .into(),
                 ResolvedPath::Unresolvable(e) => NixpkgsProblem::UnresolvablePathReference {
-                    relative_package_dir: relative_package_dir.to_path_buf(),
-                    subpath: subpath.to_path_buf(),
+                    relative_package_dir: relative_package_dir.to_owned(),
+                    subpath: subpath.to_owned(),
                     line,
                     text,
                     io_error: e.to_string(),
diff --git a/pkgs/test/nixpkgs-check-by-name/src/structure.rs b/pkgs/test/nixpkgs-check-by-name/src/structure.rs
index 9b615dd9969a..09806bc3d4dc 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/structure.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/structure.rs
@@ -7,8 +7,9 @@ use crate::NixFileStore;
 use itertools::concat;
 use lazy_static::lazy_static;
 use regex::Regex;
+use relative_path::RelativePathBuf;
 use std::fs::DirEntry;
-use std::path::{Path, PathBuf};
+use std::path::Path;
 
 lazy_static! {
     static ref SHARD_NAME_REGEX: Regex = Regex::new(r"^[a-z0-9_-]{1,2}$").unwrap();
@@ -21,15 +22,15 @@ pub fn shard_for_package(package_name: &str) -> String {
     package_name.to_lowercase().chars().take(2).collect()
 }
 
-pub fn relative_dir_for_shard(shard_name: &str) -> PathBuf {
-    PathBuf::from(format!("{BASE_SUBPATH}/{shard_name}"))
+pub fn relative_dir_for_shard(shard_name: &str) -> RelativePathBuf {
+    RelativePathBuf::from(format!("{BASE_SUBPATH}/{shard_name}"))
 }
 
-pub fn relative_dir_for_package(package_name: &str) -> PathBuf {
+pub fn relative_dir_for_package(package_name: &str) -> RelativePathBuf {
     relative_dir_for_shard(&shard_for_package(package_name)).join(package_name)
 }
 
-pub fn relative_file_for_package(package_name: &str) -> PathBuf {
+pub fn relative_file_for_package(package_name: &str) -> RelativePathBuf {
     relative_dir_for_package(package_name).join(PACKAGE_NIX_FILENAME)
 }
 
@@ -120,7 +121,8 @@ fn check_package(
 ) -> validation::Result<String> {
     let package_path = package_entry.path();
     let package_name = package_entry.file_name().to_string_lossy().into_owned();
-    let relative_package_dir = PathBuf::from(format!("{BASE_SUBPATH}/{shard_name}/{package_name}"));
+    let relative_package_dir =
+        RelativePathBuf::from(format!("{BASE_SUBPATH}/{shard_name}/{package_name}"));
 
     Ok(if !package_path.is_dir() {
         NixpkgsProblem::PackageNonDir {
@@ -174,7 +176,7 @@ fn check_package(
         let result = result.and(references::check_references(
             nix_file_store,
             &relative_package_dir,
-            &path.join(&relative_package_dir),
+            &relative_package_dir.to_path(path),
         )?);
 
         result.map(|_| package_name.clone())

From fb0a07229f81417b90cbb05d6522dbc4c654fb76 Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Fri, 1 Mar 2024 02:41:02 +0100
Subject: [PATCH 18/18] tests.nixpkgs-check-by-name: More inline format!
 arguments

Now that the previous commit removed all the .display()'s that were
previously necessary for PathBuf's, but now aren't for RelativePathBuf,
we can also inline the format! arguments
---
 .../src/nixpkgs_problem.rs                    | 188 +++++++-----------
 1 file changed, 76 insertions(+), 112 deletions(-)

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 f9e232b93933..7e257c0ed5d8 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
@@ -151,218 +151,185 @@ impl fmt::Display for NixpkgsProblem {
             NixpkgsProblem::ShardNonDir { relative_shard_path } =>
                 write!(
                     f,
-                    "{}: This is a file, but it should be a directory.",
-                    relative_shard_path,
+                    "{relative_shard_path}: This is a file, but it should be a directory.",
                 ),
             NixpkgsProblem::InvalidShardName { relative_shard_path, shard_name } =>
                 write!(
                     f,
-                    "{}: Invalid directory name \"{shard_name}\", must be at most 2 ASCII characters consisting of a-z, 0-9, \"-\" or \"_\".",
-                    relative_shard_path,
+                    "{relative_shard_path}: Invalid directory name \"{shard_name}\", must be at most 2 ASCII characters consisting of a-z, 0-9, \"-\" or \"_\".",
                 ),
             NixpkgsProblem::PackageNonDir { relative_package_dir } =>
                 write!(
                     f,
-                    "{}: This path is a file, but it should be a directory.",
-                    relative_package_dir,
+                    "{relative_package_dir}: This path is a file, but it should be a directory.",
                 ),
             NixpkgsProblem::CaseSensitiveDuplicate { relative_shard_path, first, second } =>
                 write!(
                     f,
-                    "{}: Duplicate case-sensitive package directories {first:?} and {second:?}.",
-                    relative_shard_path,
+                    "{relative_shard_path}: Duplicate case-sensitive package directories {first:?} and {second:?}.",
                 ),
             NixpkgsProblem::InvalidPackageName { relative_package_dir, package_name } =>
                 write!(
                     f,
-                    "{}: Invalid package directory name \"{package_name}\", must be ASCII characters consisting of a-z, A-Z, 0-9, \"-\" or \"_\".",
-                    relative_package_dir,
+                    "{relative_package_dir}: Invalid package directory name \"{package_name}\", must be ASCII characters consisting of a-z, A-Z, 0-9, \"-\" or \"_\".",
                 ),
             NixpkgsProblem::IncorrectShard { relative_package_dir, correct_relative_package_dir } =>
                 write!(
                     f,
-                    "{}: Incorrect directory location, should be {} instead.",
-                    relative_package_dir,
-                    correct_relative_package_dir,
+                    "{relative_package_dir}: Incorrect directory location, should be {correct_relative_package_dir} instead.",
                 ),
             NixpkgsProblem::PackageNixNonExistent { relative_package_dir } =>
                 write!(
                     f,
-                    "{}: Missing required \"{PACKAGE_NIX_FILENAME}\" file.",
-                    relative_package_dir,
+                    "{relative_package_dir}: Missing required \"{PACKAGE_NIX_FILENAME}\" file.",
                 ),
             NixpkgsProblem::PackageNixDir { relative_package_dir } =>
                 write!(
                     f,
-                    "{}: \"{PACKAGE_NIX_FILENAME}\" must be a file.",
-                    relative_package_dir,
+                    "{relative_package_dir}: \"{PACKAGE_NIX_FILENAME}\" must be a file.",
                 ),
             NixpkgsProblem::UndefinedAttr { relative_package_file, package_name } =>
                 write!(
                     f,
-                    "pkgs.{package_name}: This attribute is not defined but it should be defined automatically as {}",
-                    relative_package_file,
+                    "pkgs.{package_name}: This attribute is not defined but it should be defined automatically as {relative_package_file}",
                 ),
-            NixpkgsProblem::EmptyArgument { package_name, file, line, column, definition } =>
+            NixpkgsProblem::EmptyArgument { package_name, file, line, column, definition } => {
+                let relative_package_dir = structure::relative_dir_for_package(package_name);
+                let relative_package_file = structure::relative_file_for_package(package_name);
+                let indented_definition = indent_definition(*column, definition.clone());
                 writedoc!(
                     f,
                     "
-                    - Because {} exists, the attribute `pkgs.{package_name}` must be defined like
+                    - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like
 
-                        {package_name} = callPackage ./{} {{ /* ... */ }};
+                        {package_name} = callPackage ./{relative_package_file} {{ /* ... */ }};
 
-                      However, in this PR, the second argument is empty. See the definition in {}:{}:
+                      However, in this PR, the second argument is empty. See the definition in {file}:{line}:
 
-                    {}
+                    {indented_definition}
 
                       Such a definition is provided automatically and therefore not necessary. Please remove it.
                     ",
-                    structure::relative_dir_for_package(package_name),
-                    structure::relative_file_for_package(package_name),
-                    file,
-                    line,
-                    indent_definition(*column, definition.clone()),
-                ),
-            NixpkgsProblem::NonToplevelCallPackage { package_name, file, line, column, definition } =>
+                )
+            }
+            NixpkgsProblem::NonToplevelCallPackage { package_name, file, line, column, definition } => {
+                let relative_package_dir = structure::relative_dir_for_package(package_name);
+                let relative_package_file = structure::relative_file_for_package(package_name);
+                let indented_definition = indent_definition(*column, definition.clone());
                 writedoc!(
                     f,
                     "
-                    - Because {} exists, the attribute `pkgs.{package_name}` must be defined like
+                    - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like
 
-                        {package_name} = callPackage ./{} {{ /* ... */ }};
+                        {package_name} = callPackage ./{relative_package_file} {{ /* ... */ }};
 
-                      However, in this PR, a different `callPackage` is used. See the definition in {}:{}:
+                      However, in this PR, a different `callPackage` is used. See the definition in {file}:{line}:
 
-                    {}
+                    {indented_definition}
                     ",
-                    structure::relative_dir_for_package(package_name),
-                    structure::relative_file_for_package(package_name),
-                    file,
-                    line,
-                    indent_definition(*column, definition.clone()),
-                ),
-            NixpkgsProblem::NonPath { package_name, file, line, column, definition } =>
+                )
+            }
+            NixpkgsProblem::NonPath { package_name, file, line, column, definition } => {
+                let relative_package_dir = structure::relative_dir_for_package(package_name);
+                let relative_package_file = structure::relative_file_for_package(package_name);
+                let indented_definition = indent_definition(*column, definition.clone());
                 writedoc!(
                     f,
                     "
-                    - Because {} exists, the attribute `pkgs.{package_name}` must be defined like
+                    - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like
 
-                        {package_name} = callPackage ./{} {{ /* ... */ }};
+                        {package_name} = callPackage ./{relative_package_file} {{ /* ... */ }};
 
-                      However, in this PR, the first `callPackage` argument is not a path. See the definition in {}:{}:
+                      However, in this PR, the first `callPackage` argument is not a path. See the definition in {file}:{line}:
 
-                    {}
+                    {indented_definition}
                     ",
-                    structure::relative_dir_for_package(package_name),
-                    structure::relative_file_for_package(package_name),
-                    file,
-                    line,
-                    indent_definition(*column, definition.clone()),
-                ),
-            NixpkgsProblem::WrongCallPackagePath { package_name, file, line, actual_path, expected_path } =>
+                )
+            }
+            NixpkgsProblem::WrongCallPackagePath { package_name, file, line, actual_path, expected_path } => {
+                let relative_package_dir = structure::relative_dir_for_package(package_name);
+                let expected_path_expr = create_path_expr(file, expected_path);
+                let actual_path_expr = create_path_expr(file, actual_path);
                 writedoc! {
                     f,
                     "
-                    - Because {} exists, the attribute `pkgs.{package_name}` must be defined like
+                    - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like
 
-                        {package_name} = callPackage {} {{ /* ... */ }};
+                        {package_name} = callPackage {expected_path_expr} {{ /* ... */ }};
 
-                      However, in this PR, the first `callPackage` argument is the wrong path. See the definition in {}:{}:
+                      However, in this PR, the first `callPackage` argument is the wrong path. See the definition in {file}:{line}:
 
-                        {package_name} = callPackage {} {{ /* ... */ }};
+                        {package_name} = callPackage {actual_path_expr} {{ /* ... */ }};
                     ",
-                    structure::relative_dir_for_package(package_name),
-                    create_path_expr(file, expected_path),
-                    file, line,
-                    create_path_expr(file, actual_path),
-                },
+                }
+            }
             NixpkgsProblem::NonSyntacticCallPackage { package_name, file, line, column, definition } => {
+                let relative_package_dir = structure::relative_dir_for_package(package_name);
+                let relative_package_file = structure::relative_file_for_package(package_name);
+                let indented_definition = indent_definition(*column, definition.clone());
                 writedoc!(
                     f,
                     "
-                    - Because {} exists, the attribute `pkgs.{package_name}` must be defined like
+                    - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like
 
-                        {package_name} = callPackage ./{} {{ /* ... */ }};
+                        {package_name} = callPackage ./{relative_package_file} {{ /* ... */ }};
 
-                      However, in this PR, it isn't defined that way. See the definition in {}:{}
+                      However, in this PR, it isn't defined that way. See the definition in {file}:{line}
 
-                    {}
+                    {indented_definition}
                     ",
-                    structure::relative_dir_for_package(package_name),
-                    structure::relative_file_for_package(package_name),
-                    file,
-                    line,
-                    indent_definition(*column, definition.clone()),
                 )
             }
             NixpkgsProblem::NonDerivation { relative_package_file, package_name } =>
                 write!(
                     f,
-                    "pkgs.{package_name}: This attribute defined by {} is not a derivation",
-                    relative_package_file,
+                    "pkgs.{package_name}: This attribute defined by {relative_package_file} is not a derivation",
                 ),
             NixpkgsProblem::OutsideSymlink { relative_package_dir, subpath } =>
                 write!(
                     f,
-                    "{}: Path {} is a symlink pointing to a path outside the directory of that package.",
-                    relative_package_dir,
-                    subpath,
+                    "{relative_package_dir}: Path {subpath} is a symlink pointing to a path outside the directory of that package.",
                 ),
             NixpkgsProblem::UnresolvableSymlink { relative_package_dir, subpath, io_error } =>
                 write!(
                     f,
-                    "{}: Path {} is a symlink which cannot be resolved: {io_error}.",
-                    relative_package_dir,
-                    subpath,
+                    "{relative_package_dir}: Path {subpath} is a symlink which cannot be resolved: {io_error}.",
                 ),
             NixpkgsProblem::PathInterpolation { relative_package_dir, subpath, line, text } =>
                 write!(
                     f,
-                    "{}: File {} at line {line} contains the path expression \"{}\", which is not yet supported and may point outside the directory of that package.",
-                    relative_package_dir,
-                    subpath,
-                    text
+                    "{relative_package_dir}: File {subpath} at line {line} contains the path expression \"{text}\", which is not yet supported and may point outside the directory of that package.",
                 ),
             NixpkgsProblem::SearchPath { relative_package_dir, subpath, line, text } =>
                 write!(
                     f,
-                    "{}: File {} at line {line} contains the nix search path expression \"{}\" which may point outside the directory of that package.",
-                    relative_package_dir,
-                    subpath,
-                    text
+                    "{relative_package_dir}: File {subpath} at line {line} contains the nix search path expression \"{text}\" which may point outside the directory of that package.",
                 ),
             NixpkgsProblem::OutsidePathReference { relative_package_dir, subpath, line, text } =>
                 write!(
                     f,
-                    "{}: File {} at line {line} contains the path expression \"{}\" which may point outside the directory of that package.",
-                    relative_package_dir,
-                    subpath,
-                    text,
+                    "{relative_package_dir}: File {subpath} at line {line} contains the path expression \"{text}\" which may point outside the directory of that package.",
                 ),
             NixpkgsProblem::UnresolvablePathReference { relative_package_dir, subpath, line, text, io_error } =>
                 write!(
                     f,
-                    "{}: File {} at line {line} contains the path expression \"{}\" which cannot be resolved: {io_error}.",
-                    relative_package_dir,
-                    subpath,
-                    text,
+                    "{relative_package_dir}: File {subpath} at line {line} contains the path expression \"{text}\" which cannot be resolved: {io_error}.",
                 ),
             NixpkgsProblem::MovedOutOfByNameEmptyArg { package_name, call_package_path, file } => {
                 let call_package_arg =
                     if let Some(path) = &call_package_path {
-                        format!("./{}", path)
+                        format!("./{path}")
                     } else {
                         "...".into()
                     };
+                let relative_package_file = structure::relative_file_for_package(package_name);
                 writedoc!(
                     f,
                     "
-                    - Attribute `pkgs.{package_name}` was previously defined in {}, but is now manually defined as `callPackage {call_package_arg} {{ /* ... */ }}` in {}.
+                    - Attribute `pkgs.{package_name}` was previously defined in {relative_package_file}, but is now manually defined as `callPackage {call_package_arg} {{ /* ... */ }}` in {file}.
                       Please move the package back and remove the manual `callPackage`.
                     ",
-                    structure::relative_file_for_package(package_name),
-                    file,
-                    )
+                )
             },
             NixpkgsProblem::MovedOutOfByNameNonEmptyArg { package_name, call_package_path, file } => {
                 let call_package_arg =
@@ -371,17 +338,16 @@ impl fmt::Display for NixpkgsProblem {
                     } else {
                         "...".into()
                     };
+                let relative_package_file = structure::relative_file_for_package(package_name);
                 // This can happen if users mistakenly assume that for custom arguments,
                 // pkgs/by-name can't be used.
                 writedoc!(
                     f,
                     "
-                    - Attribute `pkgs.{package_name}` was previously defined in {}, but is now manually defined as `callPackage {call_package_arg} {{ ... }}` in {}.
+                    - Attribute `pkgs.{package_name}` was previously defined in {relative_package_file}, but is now manually defined as `callPackage {call_package_arg} {{ ... }}` in {file}.
                       While the manual `callPackage` is still needed, it's not necessary to move the package files.
                     ",
-                    structure::relative_file_for_package(package_name),
-                    file,
-                    )
+                )
             },
             NixpkgsProblem::NewPackageNotUsingByNameEmptyArg { package_name, call_package_path, file } => {
                 let call_package_arg =
@@ -390,16 +356,15 @@ impl fmt::Display for NixpkgsProblem {
                     } else {
                         "...".into()
                     };
+                let relative_package_file = structure::relative_file_for_package(package_name);
                 writedoc!(
                     f,
                     "
                     - Attribute `pkgs.{package_name}` is a new top-level package using `pkgs.callPackage {call_package_arg} {{ /* ... */ }}`.
-                      Please define it in {} instead.
+                      Please define it in {relative_package_file} instead.
                       See `pkgs/by-name/README.md` for more details.
-                      Since the second `callPackage` argument is `{{ }}`, no manual `callPackage` in {} is needed anymore.
+                      Since the second `callPackage` argument is `{{ }}`, no manual `callPackage` in {file} is needed anymore.
                     ",
-                    structure::relative_file_for_package(package_name),
-                    file,
                 )
             },
             NixpkgsProblem::NewPackageNotUsingByNameNonEmptyArg { package_name, call_package_path, file } => {
@@ -409,16 +374,15 @@ impl fmt::Display for NixpkgsProblem {
                     } else {
                         "...".into()
                     };
+                let relative_package_file = structure::relative_file_for_package(package_name);
                 writedoc!(
                     f,
                     "
                     - Attribute `pkgs.{package_name}` is a new top-level package using `pkgs.callPackage {call_package_arg} {{ /* ... */ }}`.
-                      Please define it in {} instead.
+                      Please define it in {relative_package_file} instead.
                       See `pkgs/by-name/README.md` for more details.
-                      Since the second `callPackage` argument is not `{{ }}`, the manual `callPackage` in {} is still needed.
+                      Since the second `callPackage` argument is not `{{ }}`, the manual `callPackage` in {file} is still needed.
                     ",
-                    structure::relative_file_for_package(package_name),
-                    file,
                 )
             },
             NixpkgsProblem::InternalCallPackageUsed { attr_name } =>