check-by-name: Refactor to prepare for enforcing `pkgs/by-name`, make `--base` required #278805

infinisil · 2024-01-05T00:51:11Z

Description of changes

This is a preparation PR for #275539, including only internal improvements to the code and comments, without changing any functionality. This makes the diff for the above PR much smaller.

The only somewhat interesting change is that --base is now required for nixpkgs-check-by-name, which was already prepared for in #274591 by always passing the flag in CI.

This PR is best reviewed commit-by-commit.

Things done

Built and tested on x86_64-linux

Add a 👍 reaction to pull requests you find important.

Does a bunch of cleanups to the eval.{rs,nix} code to make future changes easier, no functionality is changed.

This was previously a checking impurity that could produce different results when run on different systems.

CI now passes the flag, so it doesn't have to be optional anymore

This makes the attribute ratchet check logic more re-usable, which will be used in a future commit. It also renames the ratchet states to something more intuitive

Makes errors for attributes deterministic so it's easier to test (also, reproducibility is always nice)

Strips the Nixpkgs prefix from the callPackage paths, makes future error messages using this path be deterministic.

philiptaron · 2024-01-05T02:27:29Z

pkgs/test/nixpkgs-check-by-name/tests/empty-base/pkgs/by-name/README.md

I'm assuming this is a sentinel file.

Yeah, just so that git can track that the directory exists. Marginally easier than changing the code to handle a missing directory

philiptaron

OK, I took a look at the Rust code here, and skimmed the test-only Nix code.

I have three non-blocking refactoring suggestions, in descending order of "want to see":

Simplify the traits in ratchet.rs and use them more broadly.
Use anyhow::Context::with_context to avoid allocating error strings in the non-error case.
Minor typo and comment fixes as noted.

There's a slightly larger refactoring that I see that's not directly related to the changes in this PR. In main, everywhere the error_writer is used could instead use a ratchet::Nixpkgs that's cased on its trivial success.

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

philiptaron · 2024-01-05T02:33:55Z

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

+    /// What error to produce in case the ratchet went in the wrong direction
+    fn to_error(name: &str, context: &Self, existed_before: bool) -> NixpkgsProblem;


This looks like a IntoNixpkgsProblem trait. I think it can consume Self, and have context be self.

It looks like making it consume Self is a bit trickier, since the values come from a HashMap originally. And not sure what you mean with context being self!

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

philiptaron · 2024-01-08T16:23:41Z

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

+                Missing => NixpkgsProblem::UndefinedAttr {
+                    relative_package_file: relative_package_file.clone(),
+                    package_name: attribute_name.clone(),
+                }
+                .into(),


This looks like it's implementing IntoNixpkgsProblem on ByNameAttribute.

The same thing holds for the cases below on their specific types.

Not entirely because in some cases there won't be an error.

Not entirely because in some cases there won't be an error.

Agreed. I took a swing at this (PR not sent out) and ran into the same thing. There's some fine tuning that could be done for sure but I think the trait-based approach I suggested isn't right.

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

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

Ericson2314

LGTM!

- Typo - Rename AttributeRatchet to ToNixpkgsProblem - Make the compare trait method into a RatchetState method Co-Authored-By: Philip Taron <[email protected]>

Avoids allocation in the non-error case

infinisil · 2024-01-09T18:44:58Z

Feedback addressed, I'll merge once CI passes since the feedback isn't blocking :)

nixos-discourse · 2024-01-09T18:49:28Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-01-09-nixpkgs-architecture-team-meeting-47/38037/1

philiptaron

Ship it.

philiptaron · 2024-01-09T21:39:19Z

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

+                Missing => NixpkgsProblem::UndefinedAttr {
+                    relative_package_file: relative_package_file.clone(),
+                    package_name: attribute_name.clone(),
+                }
+                .into(),


Not entirely because in some cases there won't be an error.

Agreed. I took a swing at this (PR not sent out) and ran into the same thing. There's some fine tuning that could be done for sure but I think the trait-based approach I suggested isn't right.

infinisil added 3 commits January 3, 2024 19:56

tests.nixpkgs-check-by-name: Refactor eval code and improve comments

5b7ae79

Does a bunch of cleanups to the eval.{rs,nix} code to make future changes easier, no functionality is changed.

tests.nixpkgs-check-by-name: Set evaluation system to x86_64-linux

66f2959

This was previously a checking impurity that could produce different results when run on different systems.

tests.nixpkgs-check-by-name: Make --base required

ba6faf4

CI now passes the flag, so it doesn't have to be optional anymore

infinisil added this to the RFC 140 milestone Jan 5, 2024

infinisil requested review from philiptaron and 9999years January 5, 2024 00:51

infinisil mentioned this pull request Jan 5, 2024

[RFC 140 2a] pkgs/by-name: Enforce for new packages #275539

Merged

8 tasks

infinisil changed the title ~~check-by-name: Refactore to prepare for enforcing pkgs/by-name, make --base required~~ check-by-name: Refactor to prepare for enforcing pkgs/by-name, make --base required Jan 5, 2024

infinisil added 3 commits January 5, 2024 02:01

tests.nixpkgs-check-by-name: Re-usable ratchet logic

2a8f469

This makes the attribute ratchet check logic more re-usable, which will be used in a future commit. It also renames the ratchet states to something more intuitive

tests.nixpkgs-check-by-name: Deterministic ordering

27c873a

Makes errors for attributes deterministic so it's easier to test (also, reproducibility is always nice)

tests.nixpkgs-check-by-name: Internal strip nixpkgs prefix

54b0532

Strips the Nixpkgs prefix from the callPackage paths, makes future error messages using this path be deterministic.

infinisil force-pushed the by-name-enforce-preparation branch from 07eee89 to 54b0532 Compare January 5, 2024 01:02

infinisil mentioned this pull request Jan 5, 2024

RFC 140 migration plan #258650

Open

ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Jan 5, 2024

philiptaron reviewed Jan 5, 2024

View reviewed changes

philiptaron approved these changes Jan 8, 2024

View reviewed changes

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

Ericson2314 approved these changes Jan 8, 2024

View reviewed changes

delroth added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Jan 8, 2024

infinisil and others added 2 commits January 9, 2024 19:35

tests.nixpkgs-check-by-name: Minor improvements from feedback

4cd2e64

- Typo - Rename AttributeRatchet to ToNixpkgsProblem - Make the compare trait method into a RatchetState method Co-Authored-By: Philip Taron <[email protected]>

tests.nixpkgs-check-by-name: .context -> .with_context

a1db0cd

Avoids allocation in the non-error case

delroth removed the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Jan 9, 2024

philiptaron approved these changes Jan 9, 2024

View reviewed changes

infinisil merged commit da3e72b into NixOS:master Jan 9, 2024
21 of 22 checks passed

infinisil deleted the by-name-enforce-preparation branch January 9, 2024 21:59

infinisil mentioned this pull request Feb 5, 2024

doc: add clarification on logical unit for each commit #285193

Open

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

check-by-name: Refactor to prepare for enforcing `pkgs/by-name`, make `--base` required #278805

check-by-name: Refactor to prepare for enforcing `pkgs/by-name`, make `--base` required #278805

infinisil commented Jan 5, 2024

philiptaron Jan 5, 2024

infinisil Jan 5, 2024

philiptaron left a comment

philiptaron Jan 5, 2024

infinisil Jan 9, 2024

philiptaron Jan 8, 2024

infinisil Jan 9, 2024

philiptaron Jan 9, 2024

Ericson2314 left a comment

infinisil commented Jan 9, 2024

nixos-discourse commented Jan 9, 2024

philiptaron left a comment

philiptaron Jan 9, 2024

		/// What error to produce in case the ratchet went in the wrong direction
		fn to_error(name: &str, context: &Self, existed_before: bool) -> NixpkgsProblem;

check-by-name: Refactor to prepare for enforcing pkgs/by-name, make --base required #278805

check-by-name: Refactor to prepare for enforcing pkgs/by-name, make --base required #278805

Conversation

infinisil commented Jan 5, 2024

Description of changes

Things done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philiptaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 left a comment

Choose a reason for hiding this comment

infinisil commented Jan 9, 2024

nixos-discourse commented Jan 9, 2024

philiptaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

check-by-name: Refactor to prepare for enforcing `pkgs/by-name`, make `--base` required #278805

check-by-name: Refactor to prepare for enforcing `pkgs/by-name`, make `--base` required #278805