From d2fa5bafa9faff354c13c3117deb3025ca4a5795 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 23 Feb 2024 02:17:47 +0100 Subject: [PATCH] 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, empty_arg: bool, + file: PathBuf, }, NewPackageNotUsingByName { package_name: String, call_package_path: Option, 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