Skip to content

nix-channel: do not set empty nix-path when disabling channels#323613

Merged
roberth merged 7 commits intoNixOS:masterfrom
CyberShadow:fix-nix-path-without-channels-v2
Jul 28, 2024
Merged

nix-channel: do not set empty nix-path when disabling channels#323613
roberth merged 7 commits intoNixOS:masterfrom
CyberShadow:fix-nix-path-without-channels-v2

Conversation

@CyberShadow
Copy link
Member

@CyberShadow CyberShadow commented Jun 30, 2024 •
edited
Loading

Description of changes

Continuation of #273170.

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:

Changes since #273170: implements @roberth's suggestion to warn when channel data still exists, as Nix (in the absence of a NIX_PATH) will still use it.

In this circumstance, Nix will now complain:

warning: Nix search path entry '/root/.nix-defexpr/channels' does not exist, ignoring
warning: Nix search path entry '/nix/var/nix/profiles/per-user/root/channels' does not exist, ignoring

It is a little annoying, but also in a way reassuring.

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.11 Release Notes (or backporting 23.11 and 24.05 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.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jun 30, 2024
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jun 30, 2024
@CyberShadow CyberShadow force-pushed the fix-nix-path-without-channels-v2 branch from db18f9c to 55d50b6 Compare June 30, 2024 16:04
@CyberShadow CyberShadow requested a review from roberth June 30, 2024 16:06
@CyberShadow CyberShadow marked this pull request as ready for review June 30, 2024 16:12
@CyberShadow CyberShadow mentioned this pull request Jun 30, 2024
Closed
13 tasks
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Jun 30, 2024
@roberth
Copy link
Member

roberth commented Jul 14, 2024

@fricklerhandwerk and I are making progress on a fix in Nix proper, and while I wish we could do just that, not this, and keep it simple, realistically NixOS has to deal with the buggy behavior for some time to come.
We can - later - make the warning conditional or tone it down depending on config.nix.package.version.

All that context aside, one small question.

In this circumstance, Nix will now complain:

warning: Nix search path entry '/root/.nix-defexpr/channels' does not exist, ignoring
warning: Nix search path entry '/nix/var/nix/profiles/per-user/root/channels' does not exist, ignoring

It is a little annoying, but also in a way reassuring.

Do I understand correctly that NIX_PATH is set to "" by the configuration, and it would take for example unset NIX_PATH in a shellHook to cause these warnings to be printed?
That seems beneficial, because the intent of unset NIX_PATH is probably to ignore external NIX_PATH configuration such as system channels, and probably not "I don't care; use the system stuff", which is quite the opposite of what they probably want!

roberth
roberth reviewed Jul 14, 2024
View reviewed changes
nixos/modules/config/nix-channel.nix Outdated
Copy link
Member

@roberth roberth Jul 14, 2024

Choose a reason for hiding this comment

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

Shouldn't we also check for user channels?

Copy link
Member Author

@CyberShadow CyberShadow Jul 14, 2024

Choose a reason for hiding this comment

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

Yes, good idea. Even though we can't enumerate all possible values of HOME that Nix could be invoked with, we could at least check the home directories in the user catalog. Added.

Copy link
Member

@roberth roberth Jul 14, 2024

Choose a reason for hiding this comment

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

That easily catches 99% of users in my estimate, and that's a big win. 👍

@CyberShadow
Copy link
Member Author

CyberShadow commented Jul 14, 2024

Do I understand correctly that NIX_PATH is set to "" by the configuration

Yes.

and it would take for example unset NIX_PATH in a shellHook to cause these warnings to be printed?

The warnings are printed regardless of whether NIX_PATH is set. They're printed by the NixOS activation script.

The circumstance that I ran into where Nix was using an unintended default was running things with env -i, but I imagine the situation you described applies as well.

That seems beneficial, because the intent of unset NIX_PATH is probably to ignore external NIX_PATH configuration such as system channels, and probably not "I don't care; use the system stuff", which is quite the opposite of what they probably want!

Agreed! In my case, I expected the script to fail with 'nixpkgs' was not found in the Nix search path, instead of using the channels from when I installed NixOS :)

@CyberShadow @oxalica
nix-channel: do not set empty nix-path when disabling channels
1e6acab 
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]>
@CyberShadow CyberShadow force-pushed the fix-nix-path-without-channels-v2 branch from 55d50b6 to 1e6acab Compare July 14, 2024 18:17
@github-actions github-actions bot added the 8.has: documentation This PR adds or changes documentation label Jul 16, 2024
@roberth
nixosTests.installer.switchToFlake: It is probably really stupid
3f76dce 
We may want to clear NIX_PATH when channels are disabled, or maybe
it has to be a separate option.
This is just very frustrating to me.
roberth
roberth approved these changes Jul 16, 2024
View reviewed changes
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've adjusted the warnings a bit (plus added the infra to make it nice; sorry for the scope creep), and I've done some testing, with the NixOS test as well as manually with a VM.

I think we may have an issue, which is that since recently NixOS sets a NIX_PATH=nixpkgs=flake:nixpkgs anyway, which I think is really bad, but that's a separate issue.

This PR is good now! 🚀

CyberShadow
CyberShadow commented Jul 16, 2024
View reviewed changes
Copy link
Member Author

@CyberShadow CyberShadow left a comment

Choose a reason for hiding this comment

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

Good stuff!

@roberth roberth force-pushed the fix-nix-path-without-channels-v2 branch from d326855 to d376ea6 Compare July 16, 2024 20:04
@roberth
Copy link
Member

roberth commented Jul 16, 2024 •
edited
Loading

@ofborg build nixosTests.installer.switchToFlake
@ofborg build nixosTests.nix-channel
@ofborg build nixosTests.activation-lib

@roberth roberth force-pushed the fix-nix-path-without-channels-v2 branch from d376ea6 to 2d9a686 Compare July 16, 2024 20:09
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Jul 16, 2024
@SuperSandro2000 SuperSandro2000 mentioned this pull request Jul 17, 2024
Merged
@CyberShadow
Copy link
Member Author

CyberShadow commented Jul 17, 2024

and it would take for example unset NIX_PATH in a shellHook to cause these warnings to be printed?

Apologies, I've only just now realized that you meant the warnings printed by Nix, not the ones printed by the activation script added here.

And... I can no longer recall how I got Nix to print those warnings. 🤦

@jian-lin jian-lin mentioned this pull request Jul 22, 2024
Merged
13 tasks
@r-vdp r-vdp mentioned this pull request Jul 28, 2024
Merged
6 tasks
@roberth roberth merged commit 4ca52fd into NixOS:master Jul 28, 2024
github-actions bot pushed a commit that referenced this pull request Jul 28, 2024
@roberth @github-actions
nixosTests.installer.switchToFlake: Adjust for workaround in #323613
9d6195d 
(cherry picked from commit 46df92b)
@github-actions github-actions bot mentioned this pull request Jul 28, 2024
Merged
1 task
@github-actions
Copy link
Contributor

github-actions bot commented Jul 28, 2024

Successfully created backport PR for release-24.05:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@roberth roberth roberth approved these changes

@SuperSandro2000 SuperSandro2000 SuperSandro2000 approved these changes

Assignees

No one assigned

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@CyberShadow @roberth @SuperSandro2000