Mic92 / nixpkgs-review

Review pull-requests on https://github.com/NixOS/nixpkgs
MIT License
353 stars 59 forks source link

`os.environ["NIX_PATH"] = self.nixpkgs_path()` silently get ignored when `nix-path` is set in `nix.conf` #343

Closed NickCao closed 11 months ago

NickCao commented 11 months ago
$ grep nix-path /etc/nix/nix.conf
nix-path =
$ echo $NIX_PATH

$ nix eval --expr 'builtins.nixPath' --impure
[ ]
$ export NIX_PATH=foo=bar
$ nix eval --expr 'builtins.nixPath' --impure
[ ]
$ nix eval --expr 'builtins.nixPath' --impure --option nix-path foo=baz
[ { path = "baz"; prefix = "foo"; } ]

This may or may not be considered a bug of nix. But it breaks nixpkgs-review when nix.channel.enable = false; is set.

Reference: https://github.com/NixOS/nixpkgs/pull/242098 https://github.com/NixOS/nixpkgs/blob/b56ed968c4840efdc871f7408997b1d9c07bf76a/nixos/modules/config/nix-channel.nix#L98

Mic92 commented 11 months ago

We can pass --option nix-path NIXPKGS=$foo to the nix commands that nixpkgs-review runs. However it will be difficult for user provided nix commands that may run inside the review shell.

Mic92 commented 11 months ago

We are actually not depending on NIX_PATH to build packages and for our shell already: https://github.com/Mic92/nixpkgs-review/blob/720dea663bad688d251736f7322e955b9c846c73/nixpkgs_review/nix.py#L291 Can you point out under what circumstances you see issues?

NickCao commented 11 months ago

The callsite is https://github.com/Mic92/nixpkgs-review/blob/720dea663bad688d251736f7322e955b9c846c73/nixpkgs_review/nix/evalAttrs.nix#L5

figsoda commented 11 months ago

I opened #349 as a workaround for now

colonelpanic8 commented 10 months ago

@NickCao Is this behavior of nix expected? Were you ever able to figure out exactly what codepath is taken that makes it so that this:

ends up being empty:

https://github.com/NixOS/nix/blob/4a8c9bb9aa872d0f20c5aeb62357f832b4f9c0b4/src/libexpr/eval.cc#L531

as far as I can tell, this:

https://github.com/NixOS/nix/blob/4a8c9bb9aa872d0f20c5aeb62357f832b4f9c0b4/src/libexpr/eval-settings.cc#L49

is where that value gets set, and it doesn't get set anywhere else unless I'm missing something obvious.

I get that maybe it could be argued that this is desired behavior, but it feels very strange to me that setting nix-path in

nix.conf makes it so that the environment variable is just completely ignored with no warning.

figsoda commented 10 months ago

https://github.com/NixOS/nix/blob/4a8c9bb9aa872d0f20c5aeb62357f832b4f9c0b4/src/libexpr/eval-settings.hh#L19 is probably where the nix-path option comes into play here. I don't know much about C++, but this is my guess on the order of how evalSettings.nixPath is set

  1. set to getDefaultNixPath() when the nixPath Setting is constructed
  2. when the EvalSettings is constructed, if $NIX_PATH exists, parse and use that
  3. parse the config file and then command line flags and set nixPath accordingly

Since 3 runs last, the config file takes precedence over the environment variable

colonelpanic8 commented 10 months ago
  • set to getDefaultNixPath() when the nixPath Setting is constructed
  • when the EvalSettings is constructed, if $NIX_PATH exists, parse and use that
  • parse the config file and then command line flags and set nixPath accordingly

right, this was my guess as well, but its really hard to work out where this is actually happening. Also, In practice, when I compiled it and ran it, it seemed that actually what was happening is that somehow the section that sets the environment variable just wasn't being hit.

But also, I will just say that its pretty unconventional to have a config file take precedence over an environment variable.

I feel like the typical hierarchy is, because generally speaking the precedence given to these things is:

command line argument > environment variable > config file

that feels very logical to me, because it orders them by how "dynamic"/ likely it is that the user explicitly set them that way.

colonelpanic8 commented 10 months ago

Its very likely that the call that results in all of this happens here:

https://github.com/NixOS/nix/blob/4a8c9bb9aa872d0f20c5aeb62357f832b4f9c0b4/src/libstore/globals.cc#L111

but its just very unclear to me whats going on overall.

figsoda commented 10 months ago

Also, In practice, when I compiled it and ran it, it seemed that actually what was happening is that somehow the section that sets the environment variable just wasn't being hit.

I added a print statement and it seems to be working as expected. The $NIX_PATH code gets executed and builtins.nixPath still evaluates to my nix-path config

$ NIX_CONF_DIR=/etc/nix nix repl
/home/figsoda/.nix-defexpr/channels:nixpkgs=/nix/var/nix/profiles/per-user/root/channels/nixos:nixos-config=/etc/nixos/configuration.nix:/nix/var/nix/profiles/per-user/root/c
hannels
Welcome to Nix 2.18.0. Type :? for help.

nix-repl> :p builtins.nixPath
[ { path = "/nix/store/9rajnwbn6zh0w4ww7mw74y71dy23ssn8-source"; prefix = "nixpkgs"; } ]

Here is the patch I applied on top of HEAD:

--- a/src/libexpr/eval-settings.cc
+++ b/src/libexpr/eval-settings.cc
@@ -46,7 +46,10 @@ static Strings parseNixPath(const std::string & s)
 EvalSettings::EvalSettings()
 {
     auto var = getEnv("NIX_PATH");
-    if (var) nixPath = parseNixPath(*var);
+    if (var) {
+        nixPath = parseNixPath(*var);
+        printError(*var);
+    }
 }

 Strings EvalSettings::getDefaultNixPath()

The precedence is a bit unintuitive, but that might be better brought up to upstream.

colonelpanic8 commented 10 months ago

I added a print statement and it seems to be working as expected. The $NIX_PATH code gets executed and builtins.nixPath still evaluates to my nix-path config

yeah I just worked out what was going on. If you try to use debug here, you wont get output at that point because the configuration for log verbosity has not been parsed yet.