tests.nixpkgs-check-by-name: Better error for empty second arg

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

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 {
+    EmptyArgument {
-        relative_package_file: PathBuf,
         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 } =>
+            NixpkgsProblem::EmptyArgument { package_name, file, line, column, definition } =>
-                write!(
+                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!(

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 {
+        (*to).clone()
-            relative_package_file: structure::relative_file_for_package(name),
-            package_name: name.to_owned(),
-        }
     }
 }
 

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(..) => {

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.
+- Because pkgs/by-name/no/noEval exists, the attribute `pkgs.noEval` must be defined like
-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.
+
+    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

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

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