tests.nixpkgs-check-by-name: Implement gradual empty arg check migration by infinisil · Pull Request #272395 · NixOS/nixpkgs

infinisil · 2023-12-06T02:30:02Z

Context

This is part of the plan to migrate all packages to pkgs/by-name as detailed in #258650.

In #256792 I implemented support for the nixpkgs-check-by-name tool to complain when there's an unnecessary definition in all-packages.nix with an empty second argument like

foo = ../by-name/fo/foo/package.nix { };

However this was never enabled in CI with --version v1.
While at least in master there's no package where this applies, this is a perfect test case to make sure the tool can increase strictness over time without causing master breakages, as further detailed in #256788.

After thinking about it for too long, I realised that this is actually pretty easy to do after the changes in #261939 and #257735.

Changes

The --version flag is removed again, it was never used in CI, and it's not part of the public interface, so this is fine
The --base <NIXPKGS> flag is introduced, allowing the tool to check against granular regressions. For the empty argument check this means that existing empty arguments can stay, but new ones can't be introduced.

Example

If the base Nixpkgs has

# all-packages.nix
{
  foo = callPackage ../by-name/fo/foo/package.nix {
    withGui = true;
  };
}

And the main Nixpkgs has

# all-packages.nix
{
  foo = callPackage ../by-name/fo/foo/package.nix { };
}

Then the check fails because of the empty argument check.

But if you go the other way, the check succeeds!

Implications

While this isn't very useful for the empty argument check specifically, the same approach can be used to also ensure that all new packages using callPackage use pkgs/by-name.

This means that we can have incremental (and automated!) PRs to do the migration, without worrying about the base branch always introducing new violations!

Things done

Tidy code
Updated documentation
Added tests

Follow-up PR

Once the new tooling version propagates to the nixos-unstable channel, update CI to pass the --base flag.

Add a 👍 reaction to pull requests you find important.

blaggacao · 2023-12-13T22:25:22Z

Nice solution!

…problems This makes it such that these two errors can both be thrown for a single package: - The attribute value not being a derivation - The attribute not being a proper callPackage The tests had to be adjusted to only throw the error they were testing for

Convenience function to run another validation over a successful validation result. This will be usable in more locations in future commits, making the code nicer.

This prepares the code base for the removal of the `--version` flag, to be replaced with a flag that can specify a base version to compare the main Nixpkgs against, in order to have gradual transitions to stricter checks. This refactoring does: - Introduce the `version` module that can house the logic to increase strictness, with a `version::Nixpkgs` struct that contains the strictness conformity of a single Nixpkgs version - Make the check return `version::Nixpkgs` - Handle the behavior of the still-existing `--version` flag with `version::Nixpkgs` - Introduce an intermediate `process` function to handle the top-level logic, especially useful in the next commit

This implements the option for a gradual migration to stricter checks. For now this is only done for the check against empty non-auto-called callPackage arguments, but in the future this can be used to ensure all new packages make use of `pkgs/by-name`. This is implemented by adding a `--base <BASE_NIXPKGS>` flag, which then compares the base nixpkgs against the main nixpkgs version, making sure that there are no regressions. The `--version` flag is removed. While it was implemented, it was never used in CI, so this is fine.

This implements the ability to test gradual transitions in check strictness, and adds one such test for the empty non-auto-called arguments check.

philiptaron

I did end up taking a look through the PR. It all looks sane. I have various code organization suggestions, but the current organization isn't bad at all and I would definitely accept it.

I haven't had a chance to run the code or tests myself, which is the only reason I'm not signing off as-is.

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

philiptaron · 2023-12-14T17:04:33Z

pkgs/test/nixpkgs-check-by-name/README.md

- Command line: `nixpkgs-check-by-name <NIXPKGS>`
+- Command line: `nixpkgs-check-by-name [--base <BASE_NIXPKGS>] <NIXPKGS>`
 - Arguments:
+  - `<BASE_NIXPKGS>`: The path to the Nixpkgs to check against


If not specified, ...

Good point! Also had to fix the behavior in that case (though it doesn't matter much): 53b43ce

philiptaron · 2023-12-14T17:07:37Z

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

    package_names: Vec<String>,
-    eval_accessible_paths: Vec<&Path>,
-) -> validation::Result<()> {
+    eval_accessible_paths: &Vec<&Path>,


One "rusty" nit is to have the various things accepting &Vec<Something> accept instead the slice &[Something]. That's both lower power and more general. &Vec<T> automatically Derefs to &[T].

It could even be impl IntoIterator<Item = impl AsRef<Path>> to be really general, since all you do with this value is iterate over it.

impl feels a bit overkill. This doesn't expose any public interface, so it should be fine for it to not be polymorphic to make it easier to read.

The &[&Path] in this case sounds good to me!

philiptaron · 2023-12-14T17:09:24Z

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

+                let check_result = if !attribute_info.is_derivation {
+                    NixpkgsProblem::NonDerivation {
+                        relative_package_file: relative_package_file.clone(),
+                        package_name: package_name.clone(),
+                    }
+                    .into()
+                } else {
+                    Success(())
+                };
+
+                let check_result = check_result.and(match &attribute_info.variant {
+                    AttributeVariant::AutoCalled => Success(version::Attribute {
+                        empty_non_auto_called: version::EmptyNonAutoCalled::Valid,
+                    }),
                    AttributeVariant::CallPackage { path, empty_arg } => {
                        let correct_file = if let Some(call_package_path) = path {
                            absolute_package_file == *call_package_path
                        } else {
                            false
                        };
-                        // Only check for the argument to be non-empty if the version is V1 or
-                        // higher
-                        let non_empty = if version >= Version::V1 {
-                            !empty_arg
-                        } else {
-                            true
-                        };
-                        correct_file && non_empty
-                    }
-                    AttributeVariant::Other => false,
-                };

-                if !valid {
-                    NixpkgsProblem::WrongCallPackage {
-                        relative_package_file: relative_package_file.clone(),
-                        package_name: package_name.clone(),
+                        if correct_file {
+                            Success(version::Attribute {
+                                empty_non_auto_called: if *empty_arg {
+                                    version::EmptyNonAutoCalled::Invalid
+                                } else {
+                                    version::EmptyNonAutoCalled::Valid
+                                },
+                            })
+                        } else {
+                            NixpkgsProblem::WrongCallPackage {
+                                relative_package_file: relative_package_file.clone(),
+                                package_name: package_name.clone(),
+                            }
+                            .into()
+                        }
                    }
-                    .into()
-                } else if !attribute_info.is_derivation {
-                    NixpkgsProblem::NonDerivation {
+                    AttributeVariant::Other => NixpkgsProblem::WrongCallPackage {
                        relative_package_file: relative_package_file.clone(),
                        package_name: package_name.clone(),
                    }
-                    .into()
-                } else {
-                    Success(())
-                }
+                    .into(),


The current organization isn't bad at all and I would definitely accept it.

You could make a method on NixpkgsProblem for each condition that will be tested here, accepting whatever parameters are needed, and returning Option<Self> if the condition actually occured. That would make check_values look super sleek by sequencing together all the various checks, rather than bulky due to having the checks inlined.

Yeah I would also lean towards splitting this up.

Sounds like an interesting idea, though I'd like to defer that to a future PR, since it will touch a bunch more parts of the code.

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

philiptaron · 2023-12-14T17:12:53Z

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

 }

+/// Checks whether the pkgs/by-name structure in Nixpkgs is valid,
+/// and returns to which degree it's valid for checks with increased strictness.


I'm not sure what this sentence is trying to communicate.

Now updated, hope this helps: 413dd9c#diff-fc467c6e2889ff0305612175c09cd06401123d3d5e8b6cfd1e85bec22586197eL102-R104

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

Previously, not passing `--base` would enforce the most strict checks. While there's currently no actual violation of these stricter checks, this does not match the previous behavior. This won't matter once CI passes `--base`, the code handling the optionality can be removed then.

infinisil · 2023-12-15T00:12:17Z

Thanks for the review! Greatly appreciated as I don't really consider myself a Rust expert ❤️

I addressed all the comments, let me know if it looks good to you!

I haven't had a chance to run the code or tests myself, which is the only reason I'm not signing off as-is.

Can confirm that the tests pass, at least on x86_64-linux (CI will take care of the rest). Note that if you want to run them yourself you can either nix-build -A tests.nixpkgs-check-by-name, or see the development section.

philiptaron

I'll give this an actual run later tonight.

philiptaron · 2023-12-15T00:05:25Z

pkgs/test/nixpkgs-check-by-name/README.md

+    This allows the strictness of checks to increase over time by only preventing _new_ violations from being introduced,
+    while allowing violations that already existed.


At work, we call this a ratchet linter.

"Ratchet" is a great term for this idea, I updated the PR to use it! 79618ff

This would be duplicated otherwise

infinisil · 2023-12-15T01:30:45Z

Finally, I moved all interface description into the --help output, so it's not duplicated: 74e8b38

philiptaron

🎉

blaggacao

The description of the first ratchet could be clearer. I think it's important to prioritize intelligibility so that people intuitively understand what they should not do.

blaggacao · 2023-12-15T08:13:22Z

pkgs/test/nixpkgs-check-by-name/README.md

+
+The current ratchets are:
+
+- If `pkgs.${name}` is not auto-called from `pkgs/by-name`, the `args` in its `callPackage` must not be empty,


That's hardly intelligible.

In part, due to that it's hard to observe an auto-called package, it's automatic after all. So how does a non-auto called look like?

I don't know what exactly is meant, but maybe could be:

It is not allowed to introduce new callPackage calls in top-level.nix, if those would have empty args.

While it's not strictly necessary for users to understand these checks (that's what CI is for!), it's still good for future developers. Pushed fc2d269 to improve this!

infinisil added this to the RFC 140 milestone Dec 6, 2023

This was referenced Dec 6, 2023

A strategy for evolving the pkgs/by-name CI checks #256788

Closed

RFC 140 migration plan #258650

Open

infinisil added 5 commits December 14, 2023 03:13

tests.nixpkgs-check-by-name: Introduce result_map

e98d228

Convenience function to run another validation over a successful validation result. This will be usable in more locations in future commits, making the code nicer.

tests.nixpkgs-check-by-name: Test for gradual transition

bb08bfc

This implements the ability to test gradual transitions in check strictness, and adds one such test for the empty non-auto-called arguments check.

infinisil force-pushed the by-name-migrate-empty-arg branch from 3aadb9f to bb08bfc Compare December 14, 2023 03:07

infinisil marked this pull request as ready for review December 14, 2023 03:08

infinisil requested a review from philiptaron December 14, 2023 03:28

philiptaron reviewed Dec 14, 2023

View reviewed changes

infinisil added 2 commits December 15, 2023 01:01

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

413dd9c

philiptaron approved these changes Dec 15, 2023

View reviewed changes

infinisil added 2 commits December 15, 2023 02:14

tests.nixpkgs-check-by-name: Improve docs, introduce "ratchet" term

79618ff

tests.nixpkgs-check-by-name: Move interface description into code

74e8b38

This would be duplicated otherwise

philiptaron approved these changes Dec 15, 2023

View reviewed changes

blaggacao reviewed Dec 15, 2023

View reviewed changes

tests.nixpkgs-check-by-name: Improve check clarity

fc2d269

philiptaron approved these changes Dec 15, 2023

View reviewed changes

infinisil merged commit 2a107bc into NixOS:master Dec 15, 2023

infinisil deleted the by-name-migrate-empty-arg branch December 15, 2023 18:08

This was referenced Dec 16, 2023

pkgs/by-name: Enable gradual migration checks and add run-local.sh #274591

Merged

Introduce empty arg in a PR tweag/nixpkgs#81

Closed

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

Merged

infinisil mentioned this pull request Feb 20, 2024

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

Closed

		This allows the strictness of checks to increase over time by only preventing _new_ violations from being introduced,
		while allowing violations that already existed.


		The current ratchets are:

		- If `pkgs.${name}` is not auto-called from `pkgs/by-name`, the `args` in its `callPackage` must not be empty,

Uh oh!

Comments

Conversation

infinisil commented Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Changes

Example

Implications

Things done

Follow-up PR

Uh oh!

blaggacao commented Dec 13, 2023

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!

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!

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!

Uh oh!

infinisil commented Dec 15, 2023

Uh oh!

philiptaron left a comment

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 Dec 15, 2023

Uh oh!

philiptaron left a comment

Choose a reason for hiding this comment

🎉

Uh oh!

blaggacao 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

infinisil commented Dec 6, 2023 •

edited

Loading

philiptaron left a comment •

edited

Loading

blaggacao left a comment •

edited

Loading