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

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

Closed
NickCao opened this issue Jul 27, 2023 · 10 comments · Fixed by #349

Comments

@NickCao
Copy link
Contributor

NickCao commented Jul 27, 2023

$ 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: NixOS/nixpkgs#242098 https://github.com/NixOS/nixpkgs/blob/b56ed968c4840efdc871f7408997b1d9c07bf76a/nixos/modules/config/nix-channel.nix#L98

@Mic92
Copy link
Owner

Mic92 commented Jul 27, 2023

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
Copy link
Owner

Mic92 commented Jul 27, 2023

We are actually not depending on NIX_PATH to build packages and for our shell already:

f"""{{ pkgs ? import ./nixpkgs {{ system = \"{system}\"; config = import {nixpkgs_config}; }} }}:

Can you point out under what circumstances you see issues?

@NickCao
Copy link
Contributor Author

NickCao commented Jul 27, 2023

The callsite is

pkgs = import <nixpkgs> {

figsoda added a commit to figsoda/nixpkgs-review that referenced this issue Aug 11, 2023
This is not a proper fix, but removing NIX_PATH altogether removes
involves changing too many things that I am not confident in changing,
so this is what I went for as a temporary fix.
@figsoda
Copy link
Collaborator

figsoda commented Aug 11, 2023

I opened #349 as a workaround for now

figsoda added a commit to figsoda/nixpkgs-review that referenced this issue Aug 12, 2023
This is not a proper fix, but removing NIX_PATH altogether removes
involves changing too many things that I am not confident in changing,
so this is what I went for as a temporary fix.
@mergify mergify bot closed this as completed in #349 Aug 13, 2023
@colonelpanic8
Copy link

colonelpanic8 commented Sep 3, 2023

@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
Copy link
Collaborator

figsoda commented Sep 3, 2023

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
Copy link

  • 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
Copy link

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
Copy link
Collaborator

figsoda commented Sep 3, 2023

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
Copy link

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.

oxalica added a commit to oxalica/nixpkgs that referenced this issue Dec 9, 2023
An empty nix-path in nix.conf will disable NIX_PATH environment variable
entirely, which is not necessarily implied by users who want to disable
nix channels. NIX_PATH also has some usages in tools like nixos-rebuild
or just as user aliases.

That change is surprising and debatable, and also caused breakages in
nixpkgs-review and user configs.

See:
- https://github.com/NixOS/nixpkgs/pull/242098/files#r1269891427
- Mic92/nixpkgs-review#343
CyberShadow added a commit to CyberShadow/nixpkgs that referenced this issue Jul 14, 2024
An empty nix-path in nix.conf will disable NIX_PATH environment variable
entirely, which is not necessarily implied by users who want to disable
nix channels. NIX_PATH also has some usages in tools like nixos-rebuild
or just as user aliases.

That change is surprising and debatable, and also caused breakages in
nixpkgs-review and user configs.

See:
- https://github.com/NixOS/nixpkgs/pull/242098/files#r1269891427
- Mic92/nixpkgs-review#343
- NixOS/nix#10998

Co-authored-by: oxalica <[email protected]>
SuperSandro2000 pushed a commit to SuperSandro2000/nixpkgs that referenced this issue Jul 26, 2024
An empty nix-path in nix.conf will disable NIX_PATH environment variable
entirely, which is not necessarily implied by users who want to disable
nix channels. NIX_PATH also has some usages in tools like nixos-rebuild
or just as user aliases.

That change is surprising and debatable, and also caused breakages in
nixpkgs-review and user configs.

See:
- https://github.com/NixOS/nixpkgs/pull/242098/files#r1269891427
- Mic92/nixpkgs-review#343
github-actions bot pushed a commit to NixOS/nixpkgs that referenced this issue Jul 28, 2024
An empty nix-path in nix.conf will disable NIX_PATH environment variable
entirely, which is not necessarily implied by users who want to disable
nix channels. NIX_PATH also has some usages in tools like nixos-rebuild
or just as user aliases.

That change is surprising and debatable, and also caused breakages in
nixpkgs-review and user configs.

See:
- https://github.com/NixOS/nixpkgs/pull/242098/files#r1269891427
- Mic92/nixpkgs-review#343
- NixOS/nix#10998

Co-authored-by: oxalica <[email protected]>
(cherry picked from commit 1e6acab)
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 a pull request may close this issue.

4 participants