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-channel: do not set empty nix-path when disabling channels #273170

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

oxalica
Copy link
Contributor

@oxalica oxalica commented Dec 9, 2023

Description of changes

This reverts the line introduced by #242098.

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.

EDIT: It seems that empty nix-path also makes config nix.nixPath have no effect, because NIX_PATH is always ignored.

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

See:

Not sure what to test.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

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
@oxalica oxalica force-pushed the fix/nix-path-without-channel branch from b576318 to 0f2cd55 Compare December 9, 2023 17:13
@roberth
Copy link
Member

roberth commented Dec 9, 2023

IIUC this change will Nix to set its hardcoded default value.
See https://github.com/search?q=repo%3ANixOS%2Fnix%20getDefaultNixPath&type=code

If a system has not been manually cleaned after disabling nix.channel.enable, this will cause the old channel data to be used again.
For this reason I don't think this is a good workaround, unless you've removed the data.
Maybe it's good enough if the activation script put a big fat warning if it finds that the data still exists and tells the user that their option is ineffective until they remove it?

The actual root cause is in Nix. Unfortunately it's non-trivial because of implementation design, and legacy.

cc @fricklerhandwerk

Copy link
Contributor

@r-vdp r-vdp left a comment

Choose a reason for hiding this comment

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

Been running with this for a bit now and seems to work well. Thanks!

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

I see two ways to move this forward without breaking users' expectations.

  • Either print a clear warning in the activation script if the channel data still exists in /nix/var/... and can therefore still be found by Nix,
  • or make this an explicit opt-in: add an option such as nix.dontUnsetNixPath.

In the latter case, we can deprecate the option when the root cause has been solved in Nix.

@nbdd0121
Copy link
Contributor

IIUC this change will Nix to set its hardcoded default value.
See github.com/search?q=repo%3ANixOS%2Fnix%20getDefaultNixPath&type=code

From my testing, NIX_PATH takes precedence over that hardcoded default. If I unset NIX_PATH, then indeed channels from the default config are picked up, but if I set NIX_PATH to empty then no channels can be used.

IIUC when channels.enable = false, NIX_PATH is set to empty, and therefore I'd say it's the expected behaviour.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/disabling-channels-breaks-nix-path-resolution/34825/3

@oxalica
Copy link
Contributor Author

oxalica commented Jan 8, 2024

@roberth

the channel data still exists in /nix/var/... and can therefore still be found by Nix,

If a system has not been manually cleaned after disabling nix.channel.enable, this will cause the old channel data to be used again.

As stated by @nbdd0121, this is not true. When NIX_PATH is set, which is the current behavior no matter if channel is enabled, it is used instead of the builtin default value that you mentioned. Thus the old channel data is still ignored.

It's already covered by the test nixosTests.installer.switchToFlake, which still succeeds on this PR.

@roberth
Copy link
Member

roberth commented Jan 8, 2024

Ok, I suppose we could apply this change while NixOS/nix#9574 isn't resolved then.
Instead of deleting the line, could you comment it out, and put a comment that references the issue?

@ofborg test installer.switchToFlake

@roberth
Copy link
Member

roberth commented Jan 8, 2024

Ok, I suppose we could apply this change

Or maybe not, because of nix-shell --pure, which unsets NIX_PATH, among almost everything.
Ironic.

$ export NIX_PATH=test=$PWD

$ nix-shell --pure -I nixpkgs=. -p hello --run 'env | grep NIX_PATH'

$

@oxalica
Copy link
Contributor Author

oxalica commented Jan 8, 2024

Or maybe not, because of nix-shell --pure, which unsets NIX_PATH, among almost everything. Ironic.

$ export NIX_PATH=test=$PWD

$ nix-shell --pure -I nixpkgs=. -p hello --run 'env | grep NIX_PATH'

I don't think this matter. --pure is an explicit way to mimic Nix's sandbox-ed building environment, which of course unset as much as possible from your shell environment. As a module option, it's configuring the default shell environment, and it cannot forbid user or program from changing it to another way.
Also that, if you are really running nix in a nix-shell, I'd assume you are debugging a builder which calls nix, it's problematic anyway due to not-so-good isolation. It's the last thing to consider that whether outer configuration takes effect on the inner Nix.

@infinisil
Copy link
Member

@oxalica The problem is that an empty NIX_PATH makes Nix use the default, which then includes the channels unless you have them deleted..

@oxalica
Copy link
Contributor Author

oxalica commented Jan 13, 2024

@oxalica The problem is that an empty NIX_PATH makes Nix use the default, which then includes the channels unless you have them deleted..

My argument is that, NIX_PATH is set by the module already. It is NOT empty without intentional setting/unsetting it (like nix-shell or env). It is not possible and unnecessary to prohibit user from reverting it at will.

For example, EDITOR= sudoedit /file or nix-shell --pure --run '/path/to/sudoedit' will call nano as fallback, even through programs.nano.enable = false; programs.vim.defaultEditor = true;. This is not considered a problem. Because the user deliberately unsetting the environment from the system configuration, and want to use the "true default" of sudoedit.

@roberth
Copy link
Member

roberth commented Jan 13, 2024

The problem is that the "true default" behavior breaks the intended effect of the option. It's a system option, so it will be perceived to have authority over the system.

Arguably that wouldn't be as big of a deal if the activation script were to print a big fat warning that commands such as nix-shell --pure will still be able to find the old channel data (and suggest how to remove that).
I think that would be an ok solution.

@uninsane
Copy link
Contributor

a warning would go a long way to helping users out here. previously, i was setting:

nix.nixPath = [ "nixpkgs=/path/to/my/dev/fork/of/nixpkgs" ];

this was working for quite a while. eventually though i set nix.channels.enable = false; because i don't use channels, and this seems to have had the unintended side-effect of breaking all my #!nix-shell scripts (though, i'm actually not sure if it was that change, or something else?)

anyway, there are people out there setting nix.nixPath for reasons entirely unrelated to channels. i ran into another person in the Nix Matrix room who hit this yesterday. nothing in the name of that option suggests it's about channels. so if there's no obvious "correct" behavior to choose, can we at least raise a warning if nix.nixPath is non-default while nix.channels.enable = false?

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/how-to-get-a-derivation-path-for-nixpkgs/41231/4

@winterqt
Copy link
Member

IIUC when channels.enable = false, NIX_PATH is set to empty, and therefore I'd say it's the expected behaviour.

@nbdd0121 nix.settings.nix-path is setting the nix-path setting in nix.conf, which then causes the behavior wrt the NIX_PATH environment variable being discussed.


That being said, is there a path forward for this? It's been sitting for quite a while 😕

Have we considered just nuking (or at least moving/automatically backing up/restoring a la nix-darwin) the channel data?

@zendo
Copy link
Contributor

zendo commented May 7, 2024

Fixed by #254405

@SebTM
Copy link
Contributor

SebTM commented May 12, 2024

Fixed by #254405

Can you explain, please? For me it looks like it was partially reverted by https://github.com/ryan4yin/nix-config/pull/123/files but as another comment stated (don't know why it's gone/not showing up for me?):

Nix path is set to: nixpkgs=flake:nixpkgs then but but action e.g. "nix-shell -p jq" fail with:

error: file 'nixpkgs' was not found in the Nix search path (add it using $NIX_PATH or -I)

       at «none»:0: (source not available)

@zendo
Copy link
Contributor

zendo commented May 13, 2024

Fixed by #254405

Can you explain, please? For me it looks like it was partially reverted by https://github.com/ryan4yin/nix-config/pull/123/files but as another comment stated (don't know why it's gone/not showing up for me?):

Nix path is set to: nixpkgs=flake:nixpkgs then but but action e.g. "nix-shell -p jq" fail with:

error: file 'nixpkgs' was not found in the Nix search path (add it using $NIX_PATH or -I)

       at «none»:0: (source not available)

Sorry, I made a mistake, this issue has not been resolved yet.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/24-05-add-flake-to-nix-path/46310/11

@CyberShadow
Copy link
Contributor

I see two ways to move this forward without breaking users' expectations.

  • Either print a clear warning in the activation script if the channel data still exists in /nix/var/... and can therefore still be found by Nix,

@roberth I agree. I implemented this suggestion in #323613, if you'd like to have a look.

@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
@antoineco
Copy link

antoineco commented Sep 10, 2024

Note that since NixOS/nix#11079 made its way into Nix 2.24, the current behavior of exporting the NIX_PATH environment variable even when nix.nixPath is empty seems to no longer be appropriate.

An empty value now effectively prevents extra-nix-path from having any effect, so the option set by the installer is ignored since Nix 2.24, which I have found to be very confusing until I stumbled upon the aforementioned Nix PR.

This can detect and fix the problem in a Zsh RC file:

programs.zsh.shellInit = ''(( ''${+NIX_PATH} )) && [[ -z ''${NIX_PATH} ]] && unset NIX_PATH'';

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.