Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nix-shell: restore backwards-compat with old nixpkgs #6664

Merged
merged 2 commits into from
Jul 5, 2022

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Jun 13, 2022

Basically an attempt to resume fixing #5543 for a breakage introduced
earlier[1]. Basically, when evaluating an older nixpkgs with
nix-shell the following error occurs:

λ ma27 [~] → nix-shell -I nixpkgs=channel:nixos-18.03 -p nix
error: anonymous function at /nix/store/zakqwc529rb6xcj8pwixjsxscvlx9fbi-source/pkgs/top-level/default.nix:20:1 called with unexpected argument 'inNixShell'

       at /nix/store/zakqwc529rb6xcj8pwixjsxscvlx9fbi-source/pkgs/top-level/impure.nix:82:1:

           81|
           82| import ./. (builtins.removeAttrs args [ "system" "platform" ] // {
             | ^
           83|   inherit config overlays crossSystem;

This is a problem because one of the main selling points of Nix is that
you can evaluate any old Nix expression and still get the same result
(which also means that it still evaluates). In fact we're deprecating,
but not removing a lot of stuff for that reason such as unquoted URLs[2]
or builtins.toPath. However this property was essentially thrown away
here.

The change is rather simple: check if inNixShell is specified in the
formals of an auto-called function. This means that

{ inNixShell ? false }:
builtins.trace inNixShell
  (with import <nixpkgs> { }; makeShell { name = "foo"; })

will show trace: true while

args@{ ... }:
builtins.trace args.inNixShell
  (with import <nixpkgs> { }; makeShell { name = "foo"; })

will throw the following error:

error: attribute 'inNixShell' missing

This is explicitly needed because the function in
pkgs/top-level/impure.nix of e.g. NixOS 18.03 has an ellipsis[3], but
passes the attribute-set on to another lambda with formals that doesn't
have an ellipsis anymore (hence the error from above). This was perhaps
a mistake, but we can't fix it anymore. This also means that there's
AFAICS no proper way to check if the attr-set that's passed to the Nix
code via EvalState::autoCallFunction is eventually passed to a lambda
with formals where inNixShell is missing.

However, this fix comes with a certain price. Essentially every
shell.nix that assumes inNixShell to be passed to the formals even
without explicitly specifying it would break with this[4]. However I think
that this is ugly, but preferable:

  • Nix 2.3 was declared stable by NixOS up until recently (well, it still
    is as long as 21.11 is alive), so most people might not have even
    noticed that feature.

  • We're talking about a way shorter time-span with this change being
    in the wild, so the fallout should be smaller IMHO.

[1] 9d612c3
[2] NixOS/rfcs#45 (comment)
[3] https://github.com/NixOS/nixpkgs/blob/release-18.03/pkgs/top-level/impure.nix#L75
[4] See e.g. the second expression in this commit-message or the changes
for tests/ca/nix-shell.sh.

cc @andir @edolstra @thufschmitt @Ericson2314 @roberth

@Ma27 Ma27 changed the title /doesnix-shell: restore backwards-compat with old nixpkgs nix-shell: restore backwards-compat with old nixpkgs Jun 13, 2022
Basically an attempt to resume fixing NixOS#5543 for a breakage introduced
earlier[1]. Basically, when evaluating an older `nixpkgs` with
`nix-shell` the following error occurs:

    λ ma27 [~] → nix-shell -I nixpkgs=channel:nixos-18.03 -p nix
    error: anonymous function at /nix/store/zakqwc529rb6xcj8pwixjsxscvlx9fbi-source/pkgs/top-level/default.nix:20:1 called with unexpected argument 'inNixShell'

           at /nix/store/zakqwc529rb6xcj8pwixjsxscvlx9fbi-source/pkgs/top-level/impure.nix:82:1:

               81|
               82| import ./. (builtins.removeAttrs args [ "system" "platform" ] // {
                 | ^
               83|   inherit config overlays crossSystem;

This is a problem because one of the main selling points of Nix is that
you can evaluate any old Nix expression and still get the same result
(which also means that it *still evaluates*). In fact we're deprecating,
but not removing a lot of stuff for that reason such as unquoted URLs[2]
or `builtins.toPath`. However this property was essentially thrown away
here.

The change is rather simple: check if `inNixShell` is specified in the
formals of an auto-called function. This means that

    { inNixShell ? false }:
    builtins.trace inNixShell
      (with import <nixpkgs> { }; makeShell { name = "foo"; })

will show `trace: true` while

    args@{ ... }:
    builtins.trace args.inNixShell
      (with import <nixpkgs> { }; makeShell { name = "foo"; })

will throw the following error:

    error: attribute 'inNixShell' missing

This is explicitly needed because the function in
`pkgs/top-level/impure.nix` of e.g. NixOS 18.03 has an ellipsis[3], but
passes the attribute-set on to another lambda with formals that doesn't
have an ellipsis anymore (hence the error from above). This was perhaps
a mistake, but we can't fix it anymore. This also means that there's
AFAICS no proper way to check if the attr-set that's passed to the Nix
code via `EvalState::autoCallFunction` is eventually passed to a lambda
with formals where `inNixShell` is missing.

However, this fix comes with a certain price. Essentially every
`shell.nix` that assumes `inNixShell` to be passed to the formals even
without explicitly specifying it would break with this[4]. However I think
that this is ugly, but preferable:

* Nix 2.3 was declared stable by NixOS up until recently (well, it still
  is as long as 21.11 is alive), so most people might not have even
  noticed that feature.

* We're talking about a way shorter time-span with this change being
  in the wild, so the fallout should be smaller IMHO.

[1] NixOS@9d612c3
[2] NixOS/rfcs#45 (comment)
[3] https://github.com/NixOS/nixpkgs/blob/release-18.03/pkgs/top-level/impure.nix#L75
[4] See e.g. the second expression in this commit-message or the changes
    for `tests/ca/nix-shell.sh`.
@Ma27 Ma27 force-pushed the innixshell-backwards-compat branch from a41f22d to 98946e2 Compare June 13, 2022 21:29
@roberth
Copy link
Member

roberth commented Jun 22, 2022

We're still recovering from the 2.4 shock, aren't we.

Tbh, I don't think anyone cares about inNixShell. It was supposed to make the default.nix = shell.nix pattern reliable, but it sat in limbo for over a year and then flakes came along as a more complicated solution to this 'problem'.

@roberth
Copy link
Member

roberth commented Jun 22, 2022

I don't think anyone cares about inNixShell.

In the sense that they're using it. Not in the sense that this broke old evals.

And do take it with a grain of salt. It's out in the wild now...

@Ma27
Copy link
Member Author

Ma27 commented Jun 22, 2022

Yep, I think that this is the least problematic solution to the problem IMHO.

What do @edolstra and @thufschmitt think? :)

Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do @edolstra and @thufschmitt think? :)

That it's a very dirty hack 🙃 But it's probably the best we can do.

It's a shame that this fix is itself breaking backwards-compat (although in a more limited fashion), but again, I don't think we can do better.

tests/nix-shell.sh Outdated Show resolved Hide resolved
@Ma27
Copy link
Member Author

Ma27 commented Jun 22, 2022

@thufschmitt fixed :)

@Ma27 Ma27 requested a review from thufschmitt June 22, 2022 20:36
Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool thanks :)

@Ma27
Copy link
Member Author

Ma27 commented Jun 26, 2022

@edolstra wdyt? :)

@thufschmitt thufschmitt merged commit d63cd77 into NixOS:master Jul 5, 2022
@Ma27 Ma27 deleted the innixshell-backwards-compat branch July 5, 2022 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants