Allow nixpkgs-check-by-name to work with recent Nix versions #79

philiptaron · 2024-07-08T20:38:04Z

Fixes #78. I didn't root cause what the new behavior in the most recent Nix versions was that made the output of nix-instantiate different.

infinisil · 2024-07-12T04:05:02Z

Nice, this looks like the right fix! Could you also extend the test suite I mentioned in #78 (comment) with a case for this? Furthermore it would be great to have some comments giving the extra context

Edit: Also, let's increase the version number so we get a new release from this :)

test-nixpkgs-check-by-name.nix

default.nix

philiptaron · 2024-07-14T01:00:03Z

Could you also extend the test suite I mentioned in #78 (comment) with a case for this?

I'm trying, but I have no idea how you generated that test case. 😬 All the test says is "these are what builtins.unsafeGetAttrPos returns", not how to get unsafeGetAttrPos to return that information about the above attrset.

Could you help me out with reproduction instructions? My best attempt is:

toInherit: {
  testA = 1;
  "testB" = 2;
  ${"testC"} = 3;
  "${"testD"}" = 4;
  # A
  testE
  # B
  =
  # C
  5
  # D
  ;
  # E

  /**/testF/**/=/**/5/**/;/*E*/

  testG = {
    valueG = 6;
  };

  inherit toInherit;
  inherit (toInherit) testH;
}

nix run nixpkgs#nixVersions.latest -- eval --expr 'let f = import ./test-nix-file.nix; value = f { testH = 8; }; in map (n: { inherit (builtins.unsafeGetAttrPos n value) line column; }) (builtins.attrNames value)' --impure --json | jq -r .

It's not helped out by unsafeGetAttrPos having no documentation whatsoever.

infinisil · 2024-07-15T15:00:40Z

Oh yeah I can't quite remember how I got those numbers, probably just some trial-and-error, but obviously without actually running the final version through Nix eval, because the file in main is invalid :P

Your approach is pretty good though! I'm suggesting some changes:

testG: {
  testA = 1;
  "testB" = 2;
  ${"testC"} = 3;
  "${"testD"}" = 4;

  # A
  testE
  # B
  =
  # C
  5
  # D
  ;
  # E

  /**/testF/**/=/**/5/**/;/*E*/

  inherit testG;
  inherit ({}) testH;

  inherit ({})
    testI testJ
    testK testL;
}

nix run nixpkgs#nixVersions.latest -- eval --expr 'let value = import ./test.nix null; in builtins.mapAttrs (n: _: { inherit (builtins.unsafeGetAttrPos n value) line column; }) value' --impure --json | jq -r .

philiptaron · 2024-07-17T16:50:54Z

I'll do the requested version bump (and hence release) in a separate PR.

infinisil

Out of time for now, but ended up spending a bunch of time to improve the testing with #83

infinisil · 2024-07-19T20:57:56Z

default.nix

+    # Tests the tool on the pinned Nixpkgs tree with various Nix and Lix versions.
+    # This allows exposure to changes in behavior from Nix and Nix-alikes.
+    nixpkgsCheckWithLatestNix = packages.nixpkgsCheck.nixVersions.latest;
+    nixpkgsCheckWithGitNix = packages.nixpkgsCheck.nixVersions.git;
+    nixpkgsCheckWithMinimumNix = packages.nixpkgsCheck.nixVersions.minimum;
+    nixpkgsCheckWithStableLix = packages.nixpkgsCheck.lixVersions.stable;
+    nixpkgsCheckWithLatestLix = packages.nixpkgsCheck.lixVersions.latest;


This is slowing down CI by about 3.5 minutes. I opened #83 instead to allow running the test suite with different Nix versions, which this PR can make use of.

This is way less important now that #83 is merged and the tool doesn't depend on the ambient version of Nix. I'm fine cutting them all, or going down to {minimum,stable,latest}.

That said, given that reviews are usually a matter of a week or more, 3.5 minutes more isn't an actual issue. Amdahl's law!

I decided to keep these in the CI run. The number of PRs that aren't you or I is quite low.

src/nix_file.rs

infinisil · 2024-07-19T21:02:05Z

src/nix_file.rs

@@ -190,9 +190,7 @@ impl NixFile {
            // Something like `foo`, `"foo"` or `${"foo"}`


While testing, I found out that this is the branch also used for the foo in inherit (bar) foo in Nix > 2.20.

Suggested change

// Something like `foo`, `"foo"` or `${"foo"}`

// Something like `foo`, `"foo"` or `${"foo"}`

// For Nix versions > 2.20, this is also `foo` in `inherit (bar) foo`

infinisil · 2024-07-19T21:03:10Z

src/nix_file.rs

@@ -190,9 +190,7 @@ impl NixFile {
            // Something like `foo`, `"foo"` or `${"foo"}`
        } else if ast::Inherit::can_cast(node.kind()) {
            // Something like `inherit <attr>` or `inherit (<source>) <attr>`.


Suggested change

// Something like `inherit <attr>` or `inherit (<source>) <attr>`.

// Only for Nix versions <= 2.20: Something like `inherit <attr>` or `inherit (<source>) <attr>`.

src/nix_file.rs

philiptaron · 2024-07-19T23:18:42Z

I'll take the various suggestions about comments and fix it up to run the cargo tests under a plethora of Nix versions.

philiptaron · 2024-07-22T18:36:07Z

Nix eval using 2.3.18 (as performed by nixpkgs-check-by-name) is currently broken in nixpkgs master. NixOS/nixpkgs#329212 fixes it.

philiptaron · 2024-07-22T18:42:12Z

This is drafted until an update for nixpkgs causes eval on 2.3.18 to pass.

Edit on 2024-07-26: #88 unblocked this.

…x version To run against a specific Nix or Lix version, run this: ``` nix-build -A nixpkgsCheck.nixVersions.nix_2_20 nix-build -A nixpkgsCheck.lixVersions.lix_2_90 ``` To run against all Nix or Lix versions, run this: ``` nix-build -A nixpkgsCheck.nixVersions nix-build -A nixpkgsCheck.lixVersions ``` CI will build the following versions: * `nixpkgsCheck` (using `nixVersions.stable`) * `nixpkgsCheckWithMinimumNix` (using `nixVersions.minimum`) * `nixpkgsCheckWithLatestNix` (using `nixVersions.latest`) * `nixpkgsCheckWithGitNix` (using `nixVersions.git`) * `nixpkgsCheckWithStableLix` (using `lixVersions.stable`) * `nixpkgsCheckWithLatestLix` (using `lixVersions.latest`) These new tests exposes that Nix versions greater than 2.20 fail. We don't currently have a derivation for unwrapped `nixpkgs-check-by-name`. Maybe soon.

…note This exposes that Nix latest and all Lix versions fail.

This causes the following tests to pass: - `nix-build -A nixpkgsCheck.nixVersions.nix_2_21` - `nix-build -A nixpkgsCheck.nixVersions.nix_2_22` - `nix-build -A nixpkgsCheck.nixVersions.nix_2_23` - `nix-build -A nixpkgsCheck.nixVersions.git` - `nix-build -A nixpkgsCheck.lixVersions.stable` Add a regression test for this multiline inherit situation. In that test, use `pretty_assertions` to get a nice diff expression for the assertion, and make it do one assertion instead of asserting in a loop.

infinisil

Let's go! Thanks for this :D

philiptaron requested a review from a team as a code owner July 8, 2024 20:38

infinisil requested changes Jul 12, 2024

View reviewed changes

test-nixpkgs-check-by-name.nix Outdated Show resolved Hide resolved

test-nixpkgs-check-by-name.nix Outdated Show resolved Hide resolved

default.nix Outdated Show resolved Hide resolved

philiptaron force-pushed the issue-78/fix-inherit-io-issue branch from 52fd4a8 to d128f5e Compare July 15, 2024 16:56

philiptaron requested a review from infinisil July 16, 2024 18:24

infinisil mentioned this pull request Jul 19, 2024

Fixed Nix version and testing of multiple versions (and fix them for nixVersions.minimum) #83

Merged

infinisil requested changes Jul 19, 2024

View reviewed changes

This was referenced Jul 20, 2024

eval: use a work directory instead of an environment variable to control the Nix expression #84

Merged

{gradle.fetchDeps,shattered-pixel-dungeon}: fix eval on Nix 2.3.18 NixOS/nixpkgs#329212

Merged

philiptaron force-pushed the issue-78/fix-inherit-io-issue branch from d128f5e to ba8bc85 Compare July 22, 2024 18:37

philiptaron marked this pull request as draft July 22, 2024 18:41

philiptaron added 3 commits July 26, 2024 14:59

package.nix: run the unit tests against every Nix and Lix version of …

a5856f0

…note This exposes that Nix latest and all Lix versions fail.

philiptaron force-pushed the issue-78/fix-inherit-io-issue branch from ba8bc85 to bb1b9e1 Compare July 26, 2024 22:03

philiptaron marked this pull request as ready for review July 26, 2024 22:10

philiptaron requested a review from infinisil July 26, 2024 22:10

infinisil approved these changes Jul 26, 2024

View reviewed changes

infinisil merged commit 725b75b into NixOS:main Jul 26, 2024
3 checks passed

philiptaron deleted the issue-78/fix-inherit-io-issue branch August 29, 2024 22:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow nixpkgs-check-by-name to work with recent Nix versions #79

Allow nixpkgs-check-by-name to work with recent Nix versions #79

philiptaron commented Jul 8, 2024

infinisil commented Jul 12, 2024 •

edited

Loading

philiptaron commented Jul 14, 2024 •

edited

Loading

infinisil commented Jul 15, 2024

philiptaron commented Jul 17, 2024

infinisil left a comment

infinisil Jul 19, 2024

philiptaron Jul 19, 2024

philiptaron Jul 26, 2024

infinisil Jul 19, 2024

infinisil Jul 19, 2024

philiptaron commented Jul 19, 2024

philiptaron commented Jul 22, 2024 •

edited

Loading

philiptaron commented Jul 22, 2024 •

edited

Loading

infinisil left a comment

		@@ -190,9 +190,7 @@ impl NixFile {
		// Something like `foo`, `"foo"` or `${"foo"}`

	// Something like `foo`, `"foo"` or `${"foo"}`
	// Something like `foo`, `"foo"` or `${"foo"}`
	// For Nix versions > 2.20, this is also `foo` in `inherit (bar) foo`

	// Something like `inherit <attr>` or `inherit (<source>) <attr>`.
	// Only for Nix versions <= 2.20: Something like `inherit <attr>` or `inherit (<source>) <attr>`.

Allow nixpkgs-check-by-name to work with recent Nix versions #79

Allow nixpkgs-check-by-name to work with recent Nix versions #79

Conversation

philiptaron commented Jul 8, 2024

infinisil commented Jul 12, 2024 • edited Loading

philiptaron commented Jul 14, 2024 • edited Loading

infinisil commented Jul 15, 2024

philiptaron commented Jul 17, 2024

infinisil left a comment

Choose a reason for hiding this comment

infinisil Jul 19, 2024

Choose a reason for hiding this comment

philiptaron Jul 19, 2024

Choose a reason for hiding this comment

philiptaron Jul 26, 2024

Choose a reason for hiding this comment

infinisil Jul 19, 2024

Choose a reason for hiding this comment

infinisil Jul 19, 2024

Choose a reason for hiding this comment

philiptaron commented Jul 19, 2024

philiptaron commented Jul 22, 2024 • edited Loading

philiptaron commented Jul 22, 2024 • edited Loading

infinisil left a comment

Choose a reason for hiding this comment

infinisil commented Jul 12, 2024 •

edited

Loading

philiptaron commented Jul 14, 2024 •

edited

Loading

philiptaron commented Jul 22, 2024 •

edited

Loading

philiptaron commented Jul 22, 2024 •

edited

Loading