Revert #6204 to fix regression, add nixpkgs/lib/tests as regression test by roberth · Pull Request #7621 · NixOS/nix

roberth · 2023-01-17T23:22:51Z

Motivation

The latest release causes an undue infinite recursion error and another undue error in a select expression (but possibly not caused by ExprSelect)

This reverts the PR that causes the problem and adds the nixpkgs/lib test suite to help make sure we don't break the evaluator or nixpkgs/lib as easily.

Troubleshooting, bisect, etc

TODO

~~fix~~ "fix" the issue
- bisect details below
- Appears to be introduced by Add context to better locate runtime coercions #6204
  - hypothesis: Nix appears to have become stricter?
    - no extra force* calls in the diff
- Reverted everything

Nixpkgs diff

diff --git a/lib/tests/release.nix b/lib/tests/release.nix
index f67892ab962..568e82b3b02 100644
--- a/lib/tests/release.nix
+++ b/lib/tests/release.nix
@@ -1,11 +1,11 @@
 { # The pkgs used for dependencies for the testing itself
   # Don't test properties of pkgs.lib, but rather the lib in the parent directory
-  pkgs ? import ../.. {} // { lib = throw "pkgs.lib accessed, but the lib tests should use nixpkgs' lib path directly!"; }
+  pkgs ? import ../.. {} // { lib = throw "pkgs.lib accessed, but the lib tests should use nixpkgs' lib path directly!"; },
+  nix ? pkgs.nix,
 }:
 
 pkgs.runCommand "nixpkgs-lib-tests" {
   buildInputs = [
-    pkgs.nix
     (import ./check-eval.nix)
     (import ./maintainers.nix {
       inherit pkgs;
@@ -19,8 +19,11 @@ pkgs.runCommand "nixpkgs-lib-tests" {
       inherit pkgs;
     })
   ];
+  nativeBuildInputs = [
+    nix
+  ];
 } ''
-    datadir="${pkgs.nix}/share"
+    datadir="${nix}/share"
     export TEST_ROOT=$(pwd)/test-tmp
     export NIX_BUILD_HOOK=
     export NIX_CONF_DIR=$TEST_ROOT/etc

$ git bisect good 2.12.0
$ git bisect bad upstream/master
$ cat >../bisecter
#!/usr/bin/env bash
nix-build ../nixpkgs/lib/tests/release.nix --arg nix '(builtins.getFlake "git+file://${toString ./.}").packages.x86_64-linux.default'
$ chmod a+x ../bisecter
$ git bisect run ../bisecter

git bisect start
# status: waiting for both good and bad commits
# good: [ef800f1e73602c0f10951dd789b97e750f37afc0] Mark official release
git bisect good ef800f1e73602c0f10951dd789b97e750f37afc0
# status: waiting for bad commit, 1 good commit known
# bad: [98f57f44bbeca3b555bd732771eac4c07d54576b] Merge pull request #7620 from NixOS/bump-2.14.0
git bisect bad 98f57f44bbeca3b555bd732771eac4c07d54576b
# good: [eece14dce42bcadb3137b27197f4807f0d696c01] Merge pull request #7410 from edolstra/release-notes
git bisect good eece14dce42bcadb3137b27197f4807f0d696c01
# good: [fb8fc6fda61b44fec161958531ab69934c5f2f4d] Merge pull request #7478 from hercules-ci/make-sure-initNix-called
git bisect good fb8fc6fda61b44fec161958531ab69934c5f2f4d
# bad: [1a4a02cff9f5d474e7085c80a0c6ef58cf50a335] Merge pull request #7559 from ncfavier/no-check-modules
git bisect bad 1a4a02cff9f5d474e7085c80a0c6ef58cf50a335
# bad: [d6f5734c630f893e8695bd37ca2b0dbf2bb24daa] Complete genericClosure tests
git bisect bad d6f5734c630f893e8695bd37ca2b0dbf2bb24daa
# bad: [5ef88457b8bdef957fcbb3cd0e7740595fad1949] Better document error location indent
git bisect bad 5ef88457b8bdef957fcbb3cd0e7740595fad1949
# bad: [e6d07e0d89d964cde22894fca57a95177c085c8d] Refactor to use more traces and less string manipulations
git bisect bad e6d07e0d89d964cde22894fca57a95177c085c8d
# bad: [57684d62475c9ec37e2abc2b7df2ec3581d0f011] fixup! s/forceValue/forceFunction/ where applicable
git bisect bad 57684d62475c9ec37e2abc2b7df2ec3581d0f011
# bad: [be1f0697468bd6c0f2be4f7e058270c161098e9f] Add error context for most basic coercions
git bisect bad be1f0697468bd6c0f2be4f7e058270c161098e9f
# bad: [00e242feed5ac848f5948dd2731bfabe603999ce] Add some context to coercion error strings
git bisect bad 00e242feed5ac848f5948dd2731bfabe603999ce
# first bad commit: [00e242feed5ac848f5948dd2731bfabe603999ce] Add some context to coercion error strings

Context

This test breaks when upgrading to the 2.13 release. nixVersions.nix_2_13: init at 2.13.1 nixpkgs#211290 (comment)
Log: https://gist.github.com/GrahamcOfBorg/fc8358d3707cca3d7aa55f38d6b9da4d

Checklist for maintainers

~~agreed on idea~~
- hotfix. easy enough to revert the revert and fix properly. Ideally we shouldn't have to agree and release is automated so I can complete the hotfix myself.
agreed on implementation strategy
- nixpkgsFor includes our nix by overlay
~~unit~~ tests
- regression test added
functional tests (tests/**.sh)
- tbd when root cause is found
documentation in the manual
- n/a
code and comments are self-explanatory
commit message explains why the change was made
- could be improved slightly, but I'm going to favor speed and definitely not merge without CI to regain that speed, because I do make mistakes.
new feature or bug fix: updated release notes

This reverts commit a75b7ba, reversing changes made to 9af16c5.

github-actions · 2023-01-18T01:27:15Z

Successfully created backport PR #7622 for 2.13-maintenance.

thufschmitt · 2023-01-18T08:05:22Z

Probably worth pinging @layus since he's the author of #6204

On top of your head, do you have any idea what might have gone wrong?

thufschmitt · 2023-01-18T08:08:01Z

Other than that, Do we really want to revert this PR on master? It's obviously important on the release branch, but keeping it for a couple days on master and try to directly fix the bug might have been simpler given the size of the change (and the likelyness of merge conflicts once we want to re-merge it). Maybe we should un-revert it if there's a reasonable guaranty that the bug can be fixed quickly?

edolstra · 2023-01-18T10:32:27Z

I'm not happy about reverting this on master, instead of actually fixing the bug. This applies the code churn of the original PR all over again. For instance, I spent a non-trivial amount of time last week syncing master into lazy-trees after the original PR got merged. The revert probably will cause a similar amount of merge conflicts to be resolved, and presumably again once we re-apply the PR.

edolstra · 2023-01-18T10:35:16Z

flake.nix

+            import (nixpkgs + "/lib/tests/release.nix")
+              { pkgs = nixpkgsFor.${system}; }
+          );
+


I don't think this test works, because it's evaluated by the Nix of the CI, not the Nix we're building.

Maybe this should just extend tests.evalNixpkgs?

nixpkgsFor includes our nix by overlay

Just confirmed that it catches the regression. I've also merged a PR upstream so that we can pass it directly, which is more explicit.

Maybe this should just extend tests.evalNixpkgs?

Reusing the whole upstream expression helps with making sure that the release can make its way into Nixpkgs.

the Nix of the CI

The real testing happens in the sandbox. The Nix of the CI only builds the new Nix and provides the environment, but the new Nix is used for the actual testing.

roberth · 2023-01-18T10:58:58Z

Do we really want to revert this PR on master?

That is a good question to ask. I think it's important to keep master in a regression free state.

but keeping it for a couple days on master

I'd prefer for this to happen on a feature branch. In the end it's the commits that matter, not really the name of the branch.

the likelyness of merge conflicts once we want to re-merge it

Reverting the PR only took only two or three conflicts. I don't expect many conflicts reverting the revert.

I'm not happy about reverting this on master, instead of actually fixing the bug.

Neither am I. I've spent about an hour trying to find the problem after bisecting.

I spent a non-trivial amount of time last week syncing master into lazy-trees after the original PR got merged.

It should be in a mergeable state as soon as the revert is reverted, shouldn't it?

tomberek · 2023-01-18T20:15:05Z

@roberth is this a regression in behavior or in the error message?

withNix(){
    tag=$1 ;
    shift ;
    nix shell github:nixos/nix/$tag --command \
        nix eval --impure --expr 'with builtins;
            let pkgs = (getFlake "github:NixOS/nixpkgs");
            in (pkgs.lib.evalModules {
                modules = [
                    ({lib,custom,...}: { config.enable = custom; })
                    ({lib,custom,...}: {
                        imports = [] ++ lib.optional custom ({lib,...}: {enable = lib.mkForce false;});
                    })
                ];
            }).config' ;
}
withNix 2.10.3
withNix 2.12.0
withNix 2.13.0
withNix 2.13.1

It's just that the nixpkgs test is a bit convoluted to unwrap.

lf- · 2023-01-19T02:22:00Z

@tomberek while it is true that it is a regression in the error message, it seems to be a regression that causes the error context to not be displayed, which is definitely a notable regression since it would break custom errors in nixpkgs modules, regressing UX.

tomberek · 2023-01-19T08:33:30Z

@tomberek while it is true that it is a regression in the error message, it seems to be a regression that causes the error context to not be displayed, which is definitely a notable regression since it would break custom errors in nixpkgs modules, regressing UX.

@lf- Please confirm. Is the resulting error worse? The relevant PR was specifically improvements to error messages.

layus · 2023-01-19T08:42:39Z

@roberth Thanks for catching this, and sorry for the fuss.
The nixos modules traces not being printed is definitely an issue, but thankfully a very easy one to fix, see #6204 (comment).

I could not reproduce the infinite execution / strictness issue you mentioned. Does it happen in the same tests ?

lf- · 2023-01-19T08:57:57Z

I am just going off the output posted in the ofborg failure, which should say which module was being evaluated due to the added error context. I've not done any tests myself.

tomberek · 2023-01-19T13:30:23Z

The difference in error output re-ordering and the following line with --show-trace. This is not a difference in evaluation output.

 … while evaluating the module argument `custom' in "/home/tom/nixpkgs/lib/tests/modules/import-custom-arg.nix":

2.12.0

error: infinite recursion encountered

       at /nix/store/1y58r47hvvn2dbwm4vrsj95krdnmfdl5-source/lib/modules.nix:483:28:

          482|         builtins.addErrorContext (context name)
          483|           (args.${name} or config._module.args.${name})
             |                            ^
          484|       ) (lib.functionArgs f);

       … while evaluating the module argument `custom' in "/home/tom/nixpkgs/lib/tests/modules/import-custom-arg.nix":

       … while calling anonymous lambda

       at /nix/store/1y58r47hvvn2dbwm4vrsj95krdnmfdl5-source/lib/modules.nix:481:44:

          480|       context = name: ''while evaluating the module argument `${name}' in "${key}":'';
          481|       extraArgs = builtins.mapAttrs (name: _:
             |                                            ^
          482|         builtins.addErrorContext (context name)

       … from call site

       … while calling 'optional'

       at /nix/store/1y58r47hvvn2dbwm4vrsj95krdnmfdl5-source/lib/lists.nix:255:20:

          254|   */
          255|   optional = cond: elem: if cond then [elem] else [];
             |                    ^
          256|

       … from call site

and

2.13.0

         at /nix/store/1y58r47hvvn2dbwm4vrsj95krdnmfdl5-source/lib/lists.nix:255:20:

          254|   */
          255|   optional = cond: elem: if cond then [elem] else [];
             |                    ^
          256|

       … while calling anonymous lambda

         at /nix/store/1y58r47hvvn2dbwm4vrsj95krdnmfdl5-source/lib/modules.nix:481:44:

          480|       context = name: ''while evaluating the module argument `${name}' in "${key}":'';
          481|       extraArgs = builtins.mapAttrs (name: _:
             |                                            ^
          482|         builtins.addErrorContext (context name)

       error: infinite recursion encountered

       at /nix/store/1y58r47hvvn2dbwm4vrsj95krdnmfdl5-source/lib/modules.nix:483:28:

          482|         builtins.addErrorContext (context name)
          483|           (args.${name} or config._module.args.${name})
             |                            ^
          484|       ) (lib.functionArgs f);

layus · 2023-01-19T14:21:59Z

#7641 is my best fix right now. And passes the old and the new tests.

roberth requested review from edolstra and thufschmitt as code owners January 17, 2023 23:22

Revert "Merge pull request NixOS#6204 from layus/coerce-string"

9b33ef3

This reverts commit a75b7ba, reversing changes made to 9af16c5.

roberth changed the title ~~flake.nix: Add nixpkgs/lib/tests as regression test~~ Revert to fix regression, add nixpkgs/lib/tests as regression test Jan 18, 2023

roberth force-pushed the nixpkgs-lib-regression-test branch from 7ba89ab to 4941ee0 Compare January 18, 2023 00:45

roberth requested a review from fricklerhandwerk as a code owner January 18, 2023 00:45

flake.nix: Add nixpkgs/lib/tests as regression test

620e4fb

roberth force-pushed the nixpkgs-lib-regression-test branch from 4941ee0 to 620e4fb Compare January 18, 2023 00:55

roberth changed the title ~~Revert to fix regression, add nixpkgs/lib/tests as regression test~~ Revert #6204 to fix regression, add nixpkgs/lib/tests as regression test Jan 18, 2023

roberth merged commit d385c13 into NixOS:master Jan 18, 2023

roberth added the backport 2.13-maintenance label Jan 18, 2023

github-actions bot mentioned this pull request Jan 18, 2023

[Backport 2.13-maintenance] Revert #6204 to fix regression, add nixpkgs/lib/tests as regression test #7622

Merged

edolstra reviewed Jan 18, 2023

View reviewed changes

tomberek mentioned this pull request Jan 19, 2023

Add context to better locate runtime coercions #6204

Merged

3 tasks

layus mentioned this pull request Jan 19, 2023

Coerce strings fixups #7641

Merged

3 tasks

lf- mentioned this pull request Nov 13, 2023

Remove impractical statement about backports #9188

Closed

Uh oh!

Comments

Conversation

roberth commented Jan 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Context

Checklist for maintainers

Uh oh!

github-actions bot commented Jan 18, 2023

Uh oh!

thufschmitt commented Jan 18, 2023

Uh oh!

thufschmitt commented Jan 18, 2023

Uh oh!

edolstra commented Jan 18, 2023

Uh oh!

edolstra Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

roberth Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

roberth Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

roberth commented Jan 18, 2023

Uh oh!

tomberek commented Jan 18, 2023

Uh oh!

lf- commented Jan 19, 2023

Uh oh!

tomberek commented Jan 19, 2023

Uh oh!

layus commented Jan 19, 2023

Uh oh!

lf- commented Jan 19, 2023

Uh oh!

tomberek commented Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

2.12.0

2.13.0

Uh oh!

layus commented Jan 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

roberth commented Jan 17, 2023 •

edited

Loading

tomberek commented Jan 19, 2023 •

edited

Loading