Be mindful about the default arguments passed to nixpkgs in nix-shell #5543
Conversation
andir
force-pushed
the
fix-2.4-nix-shell-breakage
branch
3 times, most recently
from
bbb31fe to
28c9dd5
Compare
|
Note a #!nix-shell script with -I nixpkgs= pinned to nixos-19.09 breaks without this patch. |
|
@edolstra any chance you could have a look at this fix? |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/nix-2-4-and-what-s-next/16257/1 |
|
I'm getting a test failure: |
Starting with 6eeb6f9 Nix started injecting `inNixShell` unconditonally into all nix-shell calls. This caused to a minor breakage with old Nix expressions that had a thin wrapper around nixpkgs. While we can fix those projects we should try to be backwards compatible in this regard. The error encountered in those cases looked like this: error: anonymous function at /home/andi/dev/nixos/nix/tests/shell-empty-attrs-args.nix:1:1 called with unexpected argument 'inNixShell' at «string»:1:18: 1| {...}@Args: with import <nixpkgs> args; (pkgs.runCommandCC or pkgs.runCommand) "shell" { buildInputs = [ (foo) (run) (echo "$(foo)") ]; } "" | ^ … while evaluating anonymous lambda at «string»:1:1: 1| {...}@Args: with import <nixpkgs> args; (pkgs.runCommandCC or pkgs.runCommand) "shell" { buildInputs = [ (foo) (run) (echo "$(foo)") ]; } "" | ^ … from call site
andir
force-pushed
the
fix-2.4-nix-shell-breakage
branch
from
28c9dd5 to
e1b9bb7
Compare
I am not able to reproduce that locally with Edit: It looks like |
…ix-shell" This reverts commit e1b9bb7.
… looks like Other nixpkgs aren't supported and thus our tests should reflect the nixpkgs state. [1] [1]: NixOS#5543 (comment)
|
I wonder how CI ever passed with my patch. 🤔 Anyway. The implementation is wrong and the fix doesn't appear to be as simple as I did hope. First of, the current implementation (proposed here in e1b9bb7 and earlier) removes all arguments that are not part of the formal function arguments. Less than ideal and probably breaking someone's code. Then there is also the whole story that is covered in #4003. While that issue was forward fixed in Nixpkgs we never thought about backwards compatibility in that PR (or related conversations). The flaw in my implementation is that it might work in the tests but wouldn't work with actual Nixpkgs. In nixpkgs the attribute is never part of any function signature but is just removed from the arguments if passed: If we want to retain backwards compatibility, we can't inject the I've reverted my changes in this PR and added test cases that should pass if we are backwards compatible instead. @edolstra any pointers on what we should do here? Isn't the |
|
@andir See #3168. The problem with |
When I referred to the environment variable I meant during the shells runtime. Having it available during eval is already impure and that's bad. I don't get why the Nix argument is required at all. It sounds like a hack. We already have shell.nix and equivalent flake attributes for shell expressions. In the simplest case these are the same expression as for a nix build. And if someone wants to reuse the code they should pass that flag via one of the shell expressions. Injecting |
More recent versions of Nix make backwards incompatible changes such as NixOS/nix#5543 and apparently no interest in fixing this - So we'll stick to a 2.3 version of Nix for now.
|
Since nobody cares about backwards compat I'll close this. |
Complained about here: NixOS/nix#5543
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`.
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`.
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/nix-shell-error-called-with-unexpected-argument-innixshell/20356/2 |
More recent versions of Nix make backwards incompatible changes such as NixOS/nix#5543 and apparently no interest in fixing this - So we'll stick to a 2.3 version of Nix for now.
Starting with 6eeb6f9 Nix started injecting
inNixShellunconditonally into all nix-shell calls. This caused to a minor breakage
with old Nix expressions that had a thin wrapper around nixpkgs. While
we can fix those projects we should try to be backwards compatible in
this regard.
The error encountered in those cases looked like this:
This should be backported to 2.4.