Better `pkgs/by-name` errors, minor fixes and improvements by infinisil · Pull Request #290743 · NixOS/nixpkgs

infinisil · 2024-02-23T01:29:02Z

Description of changes

This is a bit of a random assortment of pkgs/by-name improvements:

Better errors in many cases. In particular errors when a package in pkgs/by-name is overridden with a manual definition in all-packages.nix, but it's not of the expected form.
A more spaced-out error formatting for some errors (could do all in the future)
A minor false negative fix, for when a package in pkgs/by-name is overridden with a callPackage whose first argument is not a path. This also makes the code easier to understand.
A low-hanging performance improvement by running the check on two cores, which should shave around 25 seconds from CI.
Indications about whether the PR would break master, fixes master, or doesn't affect it when merged. This should make sure that we don't run into cases like Investigate failing PR check (check-by-name) #289649 again. This essentially re-implements workflows/check-by-name: Better error when base branch also fails #257735, but now in Rust

I fully admit that the code isn't the best, but I've been getting side-tracked trying to improve it for like 2 days 🙃

This relates to NixOS/nixpkgs-vet#6, more is needed to address that, probably just more docs, but that can be done separately

This work is sponsored by Antithesis ✨

Things done

Updated/added tests

Add a 👍 reaction to pull requests you find important.

Detect inherit's better, such that a future commit can use this information in an error message

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.

…more

Or rather, make it more consistent

philiptaron

The tests not being reproducible outside your enlistment (/home/tweagysil/src) looks like a blocker. There's also just a lot of English verbiage suggestions that in total add up to "request changes." I'm not tied to any particular suggestion, so if you see a better way of saying it, please do so immediately.

philiptaron · 2024-02-26T17:31:10Z

pkgs/test/nixpkgs-check-by-name/src/eval.rs


+impl Location {
+    // Returns the [file] field, but relative to Nixpkgs
+    fn relative_file(&self, nixpkgs_path: &Path) -> anyhow::Result<PathBuf> {


Future: This makes me think that Nixpkgs object ought to be passed in here, then use its .path

philiptaron · 2024-02-26T17:35:37Z

pkgs/test/nixpkgs-check-by-name/src/eval.rs

+                        let relative_file =
+                            location.relative_file(nixpkgs_path).with_context(|| {
+                                format!(
+                                    "Failed to resolve location file of attribute {attribute_name}"


Suggested change

"Failed to resolve location file of attribute {attribute_name}"

"Failed to resolve the file where attribute {attribute_name} is declared"

Not in love with "declared" (possibly: "defined"?) but this sentence doesn't read nicely to me with "location file".

I like "defined", will change :)

philiptaron · 2024-02-26T17:41:53Z

pkgs/test/nixpkgs-check-by-name/src/eval.rs

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


This case isn't clear to me (and the indentation is starting to get out of control 😬 )

Might be helped by

Changing the sense of path == relative_package_file to path != relative_package_file and commenting the Tight bound as to why it's Tight.

Putting the if syntactic_call_package.empty_arg check into the else case so it goes down an indentation level.

Finally putting the remaining case into its own block and commenting as to why it's Tight.

philiptaron · 2024-02-26T20:00:34Z

pkgs/test/nixpkgs-check-by-name/Cargo.toml


 [dev-dependencies]
 temp-env = "0.3.5"
-indoc = "2.0.4"


Note for myself: this is no longer a dev dependency because it's being used to write some error messages in NixpkgsProblem.

philiptaron · 2024-02-26T20:03:20Z

pkgs/test/nixpkgs-check-by-name/src/main.rs

+    let base_thread = thread::spawn(move || check_nixpkgs(&base_nixpkgs, keep_nix_path));
+    let main_result = check_nixpkgs(&main_nixpkgs, keep_nix_path)?;


pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected

philiptaron · 2024-02-26T20:55:07Z

pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/all-packages.nix


  bar = (x: x) self.callPackage ./pkgs/by-name/fo/foo/package.nix { someFlag = true; };
  foo = self.bar;
-


This isn't a meaningful or load-bearing change, right?

philiptaron · 2024-02-26T20:58:50Z

pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs

+                      Please define it in {} instead.
+                      See `pkgs/by-name/README.md` for more details.
+                      {extra}


Reading the tests, it looks like the *empty_arg case should be split out into its own NixpkgsProblem case. This formatting would get better if it was just two writedoc! calls rather than jammed together as they are today.

Agreed, doing that!

philiptaron · 2024-02-26T20:59:56Z

pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected

+  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 { };
+
+- 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 { /* ... */ };
+
+  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 { };
+
+- 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.


These are the tests that made me think that the wording in NewPackageNotUsingByName should be improved; they're both wordy and not as helpful as the others you added in this PR; plus, the vertical spacing is not as generous, and I think that also hurts them.

See #290743 (comment)

Co-Authored-By: Philip Taron <philip.taron@gmail.com>

infinisil · 2024-03-01T01:49:49Z

@philiptaron Thanks for the extensive review! I either addressed or implemented all of them now. I was also able to do a nice refactoring using RelativePath[Buf]

philiptaron

Tests and messages look good.

philiptaron · 2024-03-01T05:35:03Z

pkgs/test/nixpkgs-check-by-name/src/nix_file.rs

comment now out of date

Makes the code easier to understand and less error-prone

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

Includes #290743

Includes NixOS/nixpkgs#290743

infinisil added 12 commits February 19, 2024 22:02

tests.nixpkgs-check-by-name: Improve inherit detection

712ac79

Detect inherit's better, such that a future commit can use this information in an error message

tests.nixpkgs-check-by-name: Better error for non-syntactic callPackage

24069b9

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

a53b07e

tests.nixpkgs-check-by-name: Improve non-syntactic callPackage error …

7258d47

…more

tests.nixpkgs-check-by-name: Improve more errors

75cdccd

tests.nixpkgs-check-by-name: Improve errors relating to the base branch

db7562e

tests.nixpkgs-check-by-name: Evaluate base and main branch in parallel

f5e9b88

tests.nixpkgs-check-by-name: More consistent errors

eb1f014

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

1c4308c

tests.nixpkgs-check-by-name: More spaces in errors

ce27178

tests.nixpkgs-check-by-name: Improve errors for new packages

d2fa5ba

Or rather, make it more consistent

infinisil requested a review from philiptaron February 23, 2024 01:29

infinisil added this to the RFC 140 milestone Feb 23, 2024

tests.nixpkgs-check-by-name: Fix internal docs

64da617

philiptaron requested changes Feb 26, 2024

View reviewed changes

This was referenced Feb 29, 2024

Investigate failing PR check (check-by-name) #289649

Closed

zfs: rename zfsStable -> zfs_2_2; zfsUnstable -> zfs_unstable; remove enableUnstable option in favor of package #291951

Merged

Error messages should be better NixOS/nixpkgs-vet#6

Open

infinisil and others added 3 commits March 1, 2024 01:12

tests.nixpkgs-check-by-name: Various minor improvements

2e8d778

Co-Authored-By: Philip Taron <philip.taron@gmail.com>

tests.nixpkgs-check-by-name: Fix absolute path in errors

f056449

tests.nixpkgs-check-by-name: Better empty argument error

7858f06

infinisil requested a review from philiptaron March 1, 2024 01:50

philiptaron approved these changes Mar 1, 2024

View reviewed changes

pkgs/test/nixpkgs-check-by-name/src/nix_file.rs Outdated

Copy link

Contributor

philiptaron Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment now out of date

infinisil reacted with thumbs up emoji

tests.nixpkgs-check-by-name: Use RelativePath for relative paths

5981aff

Makes the code easier to understand and less error-prone

tests.nixpkgs-check-by-name: More inline format! arguments

fb0a072

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

infinisil force-pushed the by-name-better-errors branch from 14cc4ed to fb0a072 Compare March 1, 2024 22:07

philiptaron approved these changes Mar 1, 2024

View reviewed changes

delroth added the 12.approvals: 1 This PR was reviewed and approved by one person. label Mar 1, 2024

infinisil merged commit 4b8265a into NixOS:master Mar 2, 2024

infinisil deleted the by-name-better-errors branch March 2, 2024 01:28

infinisil mentioned this pull request Mar 6, 2024

check-by-name: Update pinned tooling #293664

Merged

1 task

infinisil added a commit that referenced this pull request Mar 11, 2024

check-by-name: Update pinned tooling

ec758e1

Includes #290743

infinisil mentioned this pull request Mar 16, 2024

Errors are inconsistent NixOS/nixpkgs-vet#2

Closed

infinisil added a commit to NixOS/nixpkgs-vet that referenced this pull request Mar 18, 2024

check-by-name: Update pinned tooling

64794fb

Includes NixOS/nixpkgs#290743

	"Failed to resolve location file of attribute {attribute_name}"
	"Failed to resolve the file where attribute {attribute_name} is declared"

		let base_thread = thread::spawn(move \|\| check_nixpkgs(&base_nixpkgs, keep_nix_path));
		let main_result = check_nixpkgs(&main_nixpkgs, keep_nix_path)?;


		bar = (x: x) self.callPackage ./pkgs/by-name/fo/foo/package.nix { someFlag = true; };
		foo = self.bar;

Uh oh!

Conversation

infinisil commented Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Things done

Uh oh!

philiptaron left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

🚀

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

infinisil commented Mar 1, 2024

Uh oh!

philiptaron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

infinisil commented Feb 23, 2024 •

edited

Loading

philiptaron left a comment •

edited

Loading