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

@@ -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 =
+                        let relative_location_file = location.relative_file(nixpkgs_path).with_context(|| {
-                            location.relative_file(nixpkgs_path).with_context(|| {
+                            format!("Failed to resolve the file where attribute {attribute_name} is defined")
-                                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, definition) = nix_file.call_package_argument_info_at(
+                        let (optional_syntactic_call_package, definition) = nix_file
-                            location.line,
+                            .call_package_argument_info_at(location.line, location.column, nixpkgs_path)
-                            location.column,
+                            .with_context(|| {
-                            nixpkgs_path,
+                                format!("Failed to get the definition info for attribute {attribute_name}")
-                        ).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
+                        by_name_override(
-                        // `callPackage`
+                            attribute_name,
-                        match (is_semantic_call_package, optional_syntactic_call_package) {
+                            relative_package_file,
-                            // Something like `<attr> = foo`
+                            is_semantic_call_package,
-                            (_, None) => NixpkgsProblem::NonSyntacticCallPackage {
+                            optional_syntactic_call_package,
-                                package_name: attribute_name.to_owned(),
-                                file: relative_file,
-                                line: location.line,
-                                column: location.column,
                             definition,
-                            }
+                            location,
-                            .into(),
+                            relative_location_file,
-                            // 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()
-                                }
-                            }
-                        }
                     } 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(

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(())

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
+    /// If the definition at the given location is not of the form `<attr> = callPackage <arg1> <arg2>;`,
-    /// returned, with String being how the definition looks like.
+    /// `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),
+            Left(definition) => (None, definition),
-            Ok(attrpath_value) => {
+            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;")),
+            (2, 3, Right("foo = 1;")),
-            (3, 3, Ok(r#""bar" = 2;"#)),
+            (3, 3, Right(r#""bar" = 2;"#)),
-            (4, 3, Ok(r#"${"baz"} = 3;"#)),
+            (4, 3, Right(r#"${"baz"} = 3;"#)),
-            (5, 3, Ok(r#""${"qux"}" = 4;"#)),
+            (5, 3, Right(r#""${"qux"}" = 4;"#)),
-            (8, 3, Ok("quux\n  # B\n  =\n  # C\n  5\n  # D\n  ;")),
+            (8, 3, Right("quux\n  # B\n  =\n  # C\n  5\n  # D\n  ;")),
-            (17, 7, Ok("quuux/**/=/**/5/**/;")),
+            (17, 7, Right("quuux/**/=/**/5/**/;")),
-            (19, 10, Err("inherit toInherit;")),
+            (19, 10, Left("inherit toInherit;")),
-            (20, 22, Err("inherit (toInherit) 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}"

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.
+                      However, in this PR, the second argument is empty. See the definition in {}:{}:
-                      It is defined in {}:{} as
 
                     {}
                     ",
@@ -222,8 +228,7 @@ impl fmt::Display for NixpkgsProblem {
 
                         {package_name} = callPackage ./{} {{ /* ... */ }};
 
-                      This is however not the case: A different `callPackage` is used.
+                      However, in this PR, a different `callPackage` is used. See the definition in {}:{}:
-                      It is defined in {}:{} as
 
                     {}
                     ",
@@ -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:
+                      However, in this PR, the first `callPackage` argument is not a path. See the definition in {}:{}:
-                      It is defined in {}:{} as
 
                     {}
                     ",
@@ -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.
+                      However, in this PR, the first `callPackage` argument is the wrong path. See the definition in {}:{}:
-                      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,
+                    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.
+                      However, in this PR, it isn't defined that way. See the definition in {}:{}
-                      It is defined in {}:{} as
 
                     {}
                     ",
@@ -342,24 +344,30 @@ 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 {}.
+                    - 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::MovedOutOfByNameNonEmptyArg { package_name, call_package_path, file } => {
+                let call_package_arg =
+                    if let Some(path) = &call_package_path {
+                        format!("./{}", path.display())
                     } else {
+                        "...".into()
+                    };
                 // This can happen if users mistakenly assume that for custom arguments,
                 // pkgs/by-name can't be used.
                 writedoc!(
@@ -371,30 +379,43 @@ impl fmt::Display for NixpkgsProblem {
                     structure::relative_file_for_package(package_name).display(),
                     file.display(),
                     )
-                }
             },
-            NixpkgsProblem::NewPackageNotUsingByName { package_name, call_package_path, empty_arg, file } => {
+            NixpkgsProblem::NewPackageNotUsingByNameEmptyArg { 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())
-                    } else {
-                        format!("Since the second `callPackage` argument is not `{{ }}`, the manual `callPackage` in {} is still needed.", file.display())
-                    };
                 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.
-                      {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
+    // These `expect` calls should never trigger because we only call this function with
-    // But these should never trigger because we only call this function with relative paths, and only
+    // relative paths that have a parent. That's why we `expect` them!
-    // with `from_file` being an actual file (so there's always a parent)
     let from = RelativePath::from_path(&from_file)
-        .unwrap()
+        .expect("a relative path")
         .parent()
-        .unwrap();
+        .expect("a parent for this path");
-    let to = RelativePath::from_path(&to_file).unwrap();
+    let to = RelativePath::from_path(&to_file).expect("a path");
     let rel = from.relative(to);
     format!("./{rel}")
 }

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(),
-                empty_arg: to.empty_arg,
                     file: file.to_owned(),
                 }
             } else {
-            NixpkgsProblem::NewPackageNotUsingByName {
+                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(),
+                file: file.to_owned(),
+            }
+        } else {
+            NixpkgsProblem::NewPackageNotUsingByNameNonEmptyArg {
                 package_name: name.to_owned(),
                 call_package_path: to.relative_path.clone(),
-                empty_arg: to.empty_arg,
                 file: file.to_owned(),
             }
         }

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.
+  However, in this PR, a different `callPackage` is used. See the definition in all-packages.nix: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
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.

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!

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.

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.

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.

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.

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.

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.

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.
+  However, in this PR, the second argument is empty. See the definition in all-packages.nix:9:
-  It is defined in all-packages.nix:9 as
 
     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.
+  However, in this PR, the second argument is empty. See the definition in all-packages.nix:7:
-  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
+This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.

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.

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.

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.

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.

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.

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.

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;
+
 }

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.
+  However, in this PR, it isn't defined that way. See the definition in all-packages.nix: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
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.

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.
+  However, in this PR, the first `callPackage` argument is the wrong path. See the definition in all-packages.nix:2:
-  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
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.

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.
+  However, in this PR, the second argument is empty. See the definition in all-packages.nix:2:
-  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
+This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.

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.
+  However, in this PR, it isn't defined that way. See the definition in all-packages.nix:2
-  It is defined in all-packages.nix:2 as
 
     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.

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:
+  However, in this PR, the first `callPackage` argument is not a path. See the definition in all-packages.nix:2:
-  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
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.

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:
+  However, in this PR, the first `callPackage` argument is not a path. See the definition in all-packages.nix:3:
-  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
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.

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.

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.

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.

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.

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.

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.

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.

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.
+  However, in this PR, the second argument is empty. See the definition in all-packages.nix:2:
-  It is defined in all-packages.nix:2 as
 
     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.
+  However, in this PR, the second argument is empty. See the definition in all-packages.nix:4:
-  It is defined in all-packages.nix:4 as
 
     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.

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.

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.

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.