tests.nixpkgs-check-by-name: Syntactic `callPackage` detection and allow new package variants #285089

infinisil · 2024-01-30T20:48:05Z

Description of changes

This fixes a false positive with the pkgs/by-name CI check, where it thinks that a new package defined using callPackage is introduced, but it's really just something that happens to use callPackage underneath.

This caused the check to mistakenly fail for at least these PRs:

The fix is very involved, because the new code parses the file where each attribute is defined (usually all-packages.nix) and syntactically checks whether its definition location is of the form <attr> = callPackage <file> <arg>.

The nice thing about this is that this pretty much this code has to be written anyways in order to do the automated migration.

Furthermore, the last commit fixes another small problem that causes the check to fail for #284964, since the same code is touched for these fixes.

Best reviewed commit-by-commit

This work is sponsored by Antithesis and Tweag ✨

Things done

Added tests for the changed/added code
Added comments and documentation to the changed/added code

Add a 👍 reaction to pull requests you find important.

- `fromlinecolumn` is added to be almost the reverse of `line`. This is needed in the future to get from `builtins.unsafeGetAttrPos` locations to the index for rnix - Previously the semantics for newline indices were not really defined, now they are, and make more sense. - Now there's a unit test for these functions

This makes the callPackage detection stronger by syntactically detecting whether an attribute is using callPackage See the added test case for why this is needed.

Makes the reference check use the nixFileCache instead of separately parsing and resolving paths

With `cargo fmt` Explicitly didn't do that for previous commits in order to keep the diff more easily reviewable

adamcstephens

I’m approving even though I will admit I haven’t read all the code. Broken checks are bad and I trust your diligence and responsiveness.

philiptaron

Whew! Looks mostly great. I think I want:

Not to throw away the parse error.
Move indoc to dev-dependencies.
Clarify a couple bail-vs. Ok(None) choices

... and I'm super sold.

Thanks for this work.

philiptaron · 2024-02-03T17:54:54Z

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

+                            Ok(nix_file) =>
+                                match nix_file.call_package_argument_info_at(
+                                    location.line,
+                                    location.column,
+                                    nixpkgs_path,
+                                )? {
+                                    None => Success(NonApplicable),
+                                    Some(call_package_argument_info) => {
+                                        if let Some(ref rel_path) = call_package_argument_info.relative_path {
+                                            if rel_path.starts_with(utils::BASE_SUBPATH) {
+                                                // Package variants of by-name packages are explicitly allowed according to RFC 140
+                                                // https://github.com/NixOS/rfcs/blob/master/rfcs/0140-simple-package-paths.md#package-variants:
+                                                //
+                                                // foo-variant = callPackage ../by-name/fo/foo/package.nix {
+                                                //   someFlag = true;
+                                                // }
+                                                //
+                                                // While such definitions could be moved to `pkgs/by-name` by using
+                                                // `.override { someFlag = true; }` instead, this changes the semantics in
+                                                // relation with overlays.
+                                                Success(NonApplicable)
+                                            } else {
+                                                Success(Loose(call_package_argument_info))
+                                            }
+                                        } else {
+                                            Success(Loose(call_package_argument_info))
+                                        }
+                                    },
+                                },
+                        },


The indentation levels and complexity of checks in this function are starting to get quite hard to understand.

Now refactored this function by adding two new functions to handle the by-name and non-by-name cases separately :)

philiptaron · 2024-02-03T17:55:57Z

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

@@ -14,6 +14,8 @@ anyhow = "1.0"
 lazy_static = "1.4.0"
 colored = "2.0.4"
 itertools = "0.11.0"
+rowan = "0.15.11"
+indoc = "2.0.4"


For my notes: indoc is a procedural macro for indented string literals. The indoc!() macro takes a multiline string literal and un-indents it at compile time so the leftmost non-space character is in the first column.

I think this goes in dev-dependencies because it's a test-only dependency.

philiptaron · 2024-02-03T17:56:38Z

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

@@ -14,6 +14,8 @@ anyhow = "1.0"
 lazy_static = "1.4.0"
 colored = "2.0.4"
 itertools = "0.11.0"
+rowan = "0.15.11"


For my notes: rowan is a library for lossless syntax trees, inspired in part by Swift's libsyntax.

philiptaron · 2024-02-03T17:57:30Z

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

@@ -76,6 +76,7 @@ let
        CallPackage = {
          call_package_variant = value._callPackageVariant;
          is_derivation = pkgs.lib.isDerivation value;
+          location = builtins.unsafeGetAttrPos name pkgs;


Neat. I like this little bridge to find what defines things.

philiptaron · 2024-02-03T17:58:14Z

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

@@ -1,7 +1,10 @@
 use crate::nixpkgs_problem::NixpkgsProblem;
 use crate::ratchet;
 use crate::structure;
+use crate::utils;
+use crate::validation::ResultIteratorExt;


Suggested change

use crate::validation::ResultIteratorExt;

use crate::validation::ResultIteratorExt as _;

Because I think you don't actually use the name ResultIteratorExt.

philiptaron · 2024-02-03T18:51:44Z

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

+        nix_file.syntax_root.syntax().descendants().map(|node| {
            let text = node.text().to_string();
-            let line = line_index.line(node.text_range().start().into());
+            let line = nix_file.line_index.line(node.text_range().start().into());

-            if node.kind() != NODE_PATH {
-                // We're only interested in Path expressions
-                Success(())
-            } else if node.children().count() != 0 {
+            // We're only interested in Path expressions
+            let Some(path) = rnix::ast::Path::cast(node) else {
+                return Success(());
+            };


Future: I'd appreciate a method on NixFile that accepted a Fn closure called with text, line, and path. I think that would let you make syntax_root not pub

philiptaron · 2024-02-03T18:55:27Z

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

                // Filters out ./foo/${bar}/baz
                // TODO: We can just check ./foo


I think this comment isn't that useful anymore.

philiptaron · 2024-02-03T18:56:09Z

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

-                    }
-                    .into(),
+                .into(),
+                Within(_p) =>


Suggested change

Within(_p) =>

Within(..) =>

philiptaron · 2024-02-03T18:56:44Z

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

@@ -35,17 +35,61 @@ impl LineIndex {
        // the vec
        for split in s.split_inclusive('\n') {
            index += split.len();
-            newlines.push(index);
+            newlines.push(index - 1);


Is this a bug fix? Based on the tests, I think it is. 💪🏻

Kind of. Only the behavior for indices of newline characters themselves is changed. Previously they were thought to be on the succeeding line, but I thought it makes more sense to have them on the preceding line. I don't think this is even used anywhere, but it made more sense when writing the tests and was previously unspecified :)

philiptaron · 2024-02-03T19:00:32Z

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

+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn line_index() {
+        let line_index = LineIndex::new("a\nbc\n\ndef\n");
+
+        let pairs = [
+            (0, 1, 1),
+            (1, 1, 2),
+            (2, 2, 1),
+            (3, 2, 2),
+            (4, 2, 3),
+            (5, 3, 1),
+            (6, 4, 1),
+            (7, 4, 2),
+            (8, 4, 3),
+            (9, 4, 4),
+        ];
+
+        for (index, line, column) in pairs {
+            assert_eq!(line_index.line(index), line);
+            assert_eq!(line_index.fromlinecolumn(line, column), index);
+        }
+    }


Nice, thanks for testing this.

NixOS#285089 (review) Many thanks, Philip Taron!

There was too much indentation!

infinisil · 2024-02-05T00:38:28Z

@philiptaron Thanks a lot! Addressed all your comments in two new commits :)

philiptaron

Ship it.

tomberek · 2024-02-06T17:21:19Z

My understanding was that this check would only apply if the "extra" parameter was empty {}. Thus exempting cases like:

new-package = callPackage ./pkgs/somewhere.nix { some-arg = true;};

infinisil · 2024-02-06T18:58:25Z

@tomberek It applies to all new packages using callPackage (even before this PR, this is just a bug fix). It's not a problem to require this for a non-empty second argument because we support overriding it for packages in pkgs/by-name, see here.

This reverts commit 0a3dab4 After doing some improvements to the `pkgs/by-name` check I discovered that sbcl shouldn't have been allowed in `pkgs/by-name` after all as is. Specifically, the requirement is that if `pkgs/by-name/sb/sbcl` exists, the definition of the `sbcl` attribute must look like sbcl = callPackage ../by-name/sb/sbcl/package.nix { ... }; However it currently is an alias like sbcl = sbcl_2_4_1; This wasn't detected before because `sbcl_2_4_1` was semantically defined using `callPackage`: sbcl_2_4_1 = wrapLisp { pkg = callPackage ../development/compilers/sbcl { version = "2.4.1"; }; faslExt = "fasl"; flags = [ "--dynamic-space-size" "3000" ]; }; However this doesn't syntactically match what is required. In NixOS#285089 I introduced syntactic checks for exactly this, but they were only used for packages not already in `pkgs/by-name`. Only now that I'm doing the refactoring to also use this check for `pkgs/by-name` packages this problem is noticed. While introducing this new check is technically an increase in strictness, and therefore would justify adding a new ratchet, I consider this case to be rare enough that we don't need to do that. This commit also introduces a test to prevent such regressions in the future

This adds a test to check that a commit like 0a3dab4 would fail CI After doing some improvements to the `pkgs/by-name` check I discovered that sbcl shouldn't have been allowed in `pkgs/by-name` after all as is. Specifically, the requirement is that if `pkgs/by-name/sb/sbcl` exists, the definition of the `sbcl` attribute must look like sbcl = callPackage ../by-name/sb/sbcl/package.nix { ... }; However it currently is an alias like sbcl = sbcl_2_4_1; This wasn't detected before because `sbcl_2_4_1` was semantically defined using `callPackage`: sbcl_2_4_1 = wrapLisp { pkg = callPackage ../development/compilers/sbcl { version = "2.4.1"; }; faslExt = "fasl"; flags = [ "--dynamic-space-size" "3000" ]; }; However this doesn't syntactically match what is required. In NixOS#285089 I introduced syntactic checks for exactly this, but they were only used for packages not already in `pkgs/by-name`. Only now that I'm doing the refactoring to also use this check for `pkgs/by-name` packages this problem is noticed. While introducing this new check is technically an increase in strictness, and therefore would justify adding a new ratchet, I consider this case to be rare enough that we don't need to do that. This commit introduces a test to prevent such regressions in the future Moving sbcl back out of `pkgs/by-name` will be done when the pinned CI is updated

Includes #285089

nixos-discourse · 2024-02-19T10:25:31Z

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

https://discourse.nixos.org/t/tweag-nix-dev-update-54/39990/1

NixOS/nixpkgs#285089 (review) Many thanks, Philip Taron!

This adds a test to check that a commit like 0a3dab4af34e4d086931d82827bfc8760c3e3150 would fail CI After doing some improvements to the `pkgs/by-name` check I discovered that sbcl shouldn't have been allowed in `pkgs/by-name` after all as is. Specifically, the requirement is that if `pkgs/by-name/sb/sbcl` exists, the definition of the `sbcl` attribute must look like sbcl = callPackage ../by-name/sb/sbcl/package.nix { ... }; However it currently is an alias like sbcl = sbcl_2_4_1; This wasn't detected before because `sbcl_2_4_1` was semantically defined using `callPackage`: sbcl_2_4_1 = wrapLisp { pkg = callPackage ../development/compilers/sbcl { version = "2.4.1"; }; faslExt = "fasl"; flags = [ "--dynamic-space-size" "3000" ]; }; However this doesn't syntactically match what is required. In NixOS/nixpkgs#285089 I introduced syntactic checks for exactly this, but they were only used for packages not already in `pkgs/by-name`. Only now that I'm doing the refactoring to also use this check for `pkgs/by-name` packages this problem is noticed. While introducing this new check is technically an increase in strictness, and therefore would justify adding a new ratchet, I consider this case to be rare enough that we don't need to do that. This commit introduces a test to prevent such regressions in the future Moving sbcl back out of `pkgs/by-name` will be done when the pinned CI is updated

Includes NixOS/nixpkgs#285089

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

infinisil requested a review from philiptaron January 30, 2024 20:48

infinisil added 3 commits January 30, 2024 21:53

tests.nixpkgs-check-by-name: Syntactic callPackage detection

0bcb052

This makes the callPackage detection stronger by syntactically detecting whether an attribute is using callPackage See the added test case for why this is needed.

tests.nixpkgs-check-by-name: Re-use nixFileCache more

db435ae

Makes the reference check use the nixFileCache instead of separately parsing and resolving paths

tests.nixpkgs-check-by-name: Format

46da6c5

With `cargo fmt` Explicitly didn't do that for previous commits in order to keep the diff more easily reviewable

infinisil force-pushed the by-name-syntactic-callPackage branch from 2603e33 to 46da6c5 Compare January 30, 2024 20:54

This was referenced Jan 30, 2024

sbcl: 2.4.0 -> 2.4.1 #284591

Merged

Linux Hardened Kernels 2024-01-27 #284602

Merged

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

infinisil changed the title ~~tests.nixpkgs-check-by-name: Syntactic callPackage detection~~ tests.nixpkgs-check-by-name: Syntactic callPackage detection and allow package variants Jan 30, 2024

infinisil changed the title ~~tests.nixpkgs-check-by-name: Syntactic callPackage detection and allow package variants~~ tests.nixpkgs-check-by-name: Syntactic callPackage detection and allow new package variants Jan 30, 2024

infinisil mentioned this pull request Jan 30, 2024

libcamera-qcam: init #284964

Merged

13 tasks

tests.nixpkgs-check-by-name: Allow new package variants

7d1f0a4

infinisil force-pushed the by-name-syntactic-callPackage branch from 41a2849 to 7d1f0a4 Compare January 30, 2024 21:45

This was referenced Jan 31, 2024

Netbox 3.7.1 #282929

Merged

docker_25: init at 25.0.3 #278607

Merged

RFC 140 migration: misleading CI error for aliases #285368

Closed

infinisil linked an issue Jan 31, 2024 that may be closed by this pull request

RFC 140 migration: misleading CI error for aliases #285368

Closed

This was referenced Jan 31, 2024

alice-vision: init at 3.0.0, add 3.1.0 #256115

Open

onlyoffice-bin_latest: 7.5.1 -> 8.0.0 #285888

Merged

adamcstephens approved these changes Feb 3, 2024

View reviewed changes

delroth added the 12.approvals: 1 label Feb 3, 2024

philiptaron suggested changes Feb 3, 2024

View reviewed changes

This was referenced Feb 4, 2024

Bump all* JDK packages #286267

Open

teleport_15: init at 15.2.2, teleport_12: remove #285922

Merged

tests.nixpkgs-check-by-name: Apply feedback from review

37a54e3

NixOS#285089 (review) Many thanks, Philip Taron!

tests.nixpkgs-check-by-name: Refactor eval.rs

474e7e7

There was too much indentation!

philiptaron approved these changes Feb 5, 2024

View reviewed changes

infinisil merged commit 70f9d31 into NixOS:master Feb 5, 2024
23 checks passed

infinisil deleted the by-name-syntactic-callPackage branch February 5, 2024 19:48

infinisil mentioned this pull request Feb 7, 2024

Improve pkgs/by-name code, minor fix #287083

Merged

1 task

infinisil mentioned this pull request Feb 8, 2024

Babeltrace updates and Python modules #285681

Open

13 tasks

infinisil mentioned this pull request Feb 9, 2024

check-by-name: Update pinned tool #287351

Merged

1 task

infinisil added a commit that referenced this pull request Feb 9, 2024

check-by-name: Update pinned tool

50fcc65

Includes #285089

infinisil mentioned this pull request Feb 20, 2024

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

Closed

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

tests.nixpkgs-check-by-name: Apply feedback from review

e73b9db

NixOS/nixpkgs#285089 (review) Many thanks, Philip Taron!

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

check-by-name: Update pinned tool

6a3bf80

Includes NixOS/nixpkgs#285089

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests.nixpkgs-check-by-name: Syntactic `callPackage` detection and allow new package variants #285089

tests.nixpkgs-check-by-name: Syntactic `callPackage` detection and allow new package variants #285089

infinisil commented Jan 30, 2024 •

edited

adamcstephens left a comment

philiptaron left a comment

philiptaron Feb 3, 2024

infinisil Feb 5, 2024

philiptaron Feb 3, 2024

philiptaron Feb 3, 2024

philiptaron Feb 3, 2024

philiptaron Feb 3, 2024

philiptaron Feb 3, 2024

philiptaron Feb 3, 2024

philiptaron Feb 3, 2024

philiptaron Feb 3, 2024

infinisil Feb 4, 2024

philiptaron Feb 3, 2024

infinisil commented Feb 5, 2024

philiptaron left a comment

tomberek commented Feb 6, 2024

infinisil commented Feb 6, 2024

nixos-discourse commented Feb 19, 2024

	use crate::validation::ResultIteratorExt;
	use crate::validation::ResultIteratorExt as _;

		// Filters out ./foo/${bar}/baz
		// TODO: We can just check ./foo

tests.nixpkgs-check-by-name: Syntactic callPackage detection and allow new package variants #285089

tests.nixpkgs-check-by-name: Syntactic callPackage detection and allow new package variants #285089

Conversation

infinisil commented Jan 30, 2024 • edited

Description of changes

Things done

adamcstephens left a 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

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

Choose a reason for hiding this comment

infinisil commented Feb 5, 2024

philiptaron left a comment

Choose a reason for hiding this comment

tomberek commented Feb 6, 2024

infinisil commented Feb 6, 2024

nixos-discourse commented Feb 19, 2024

tests.nixpkgs-check-by-name: Syntactic `callPackage` detection and allow new package variants #285089

tests.nixpkgs-check-by-name: Syntactic `callPackage` detection and allow new package variants #285089

infinisil commented Jan 30, 2024 •

edited