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

docs: fix description of how NIX_PATH / nix-path interact #10998

Open
wants to merge 1 commit into
base: 2.23-maintenance
Choose a base branch
from

Conversation

CyberShadow
Copy link

@CyberShadow CyberShadow commented Jun 30, 2024

If nix-path is set in nix.conf, the environment variable is always ignored.

Motivation

I was confused by this incorrect fact in the documentation, and I'm submitting a correction.

Context

I was trying to set up NixOS without channels. Setting nix.channel.enable = false; causes nix.settings.nix-path to be set to "", which I thought wouldn't change much, but it actually effectively globally configures Nix to always ignore NIX_PATH in the environment.

Some related issues I've found:

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@fricklerhandwerk
Copy link
Contributor

fricklerhandwerk commented Jul 5, 2024

The described behavior is essentially a bug. The desired behavior is documented in #7772 and #9066. We can of course merge the change, but we probably should at least note that it's a bug and not indented to stay that way.

#7871 is probably correct, except for the merge conflicts and being very hard to read and follow. I reviewed it yesterday and considered updating and untangling it, but currently we can't even properly test the default behavior of search paths on a fresh vanilla Linux because the infrastructure simply isn't mature enough to just add the test cases. I took a stab at it after inspecting all the related code, and it will need a few focused sessions to get it done. I don't have that sort of time, but maybe someone else wants to help out?

The only place where we even assume a regular Linux is in the exactly one installer test there is. I think we should check what we can leverage from https://github.com/numtide/nix-vm-test to make the setup more legible and flexible, but I'd want to study the complete code before adopting anything. Given we will need to cover the configuration combinatorics programmatically, we'll also have to figure out how to still run the test suite quickly enough. (We may want to get rid of GitHub Actions entirely, eventually, see a brief note on a recent related discussion.)

There is always the option to make a quick fix and cross fingers, but I'm really tired of inadvertently introducing more regressions because the testing situation leaves so much to be desired. I'd rather fix the root causes first, and for me that's testing, code readability, and documentation (interfaces, self-contained usage examples, and design decisions).

@roberth @Ericson2314 what do you think? Note the bug in documentation? So far we had an unwritten and not explicitly agreed-upon policy to keep bugs in the issue tracker.

@roberth
Copy link
Member

roberth commented Jul 6, 2024

@fricklerhandwerk I appreciate your thoroughness, but do we really a "vanilla Linux" test setup for this? Wouldn't a NixOS VM test with like three configurations cover this? The testScript can tweak the environment and arguments easily.

Given we will need to cover the configuration combinatorics programmatically, we'll also have to figure out how to still run the test suite quickly enough.

This is the trouble with black box testing. The solution is to not treat it as a black box, because it isn't. It's ok to use some knowledge about the system to select a manageable set of combinations of parameters for a given set of test cases, and only test those.
Brute forcing it doesn't help anyone, because you need to be able to test in your development cycle without relying on a build farm to perform a whole bunch of redundant tests. It might catch low single digit % more bugs, but would slow us down by a larger amount, leaving us with less time to actually solve problems.

what do you think? Note the bug in documentation?

We could make an exception to the unwritten rule. It's a long standing bug. We already make an exception for some long standing design bugs that users can opt in to. Maybe it's not the same, but one instance is just a precedent, not a pattern. We can judge this on a case by case basis.
Do make clear that it's a bug.

CyberShadow added a commit to CyberShadow/nixpkgs that referenced this pull request 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 <oxalicc@pm.me>
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2024-07-03-nix-team-meeting-minutes-158/49097/1

github-actions bot pushed a commit to NixOS/nixpkgs that referenced this pull request 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 <oxalicc@pm.me>
(cherry picked from commit 1e6acab)
@roberth
Copy link
Member

roberth commented Aug 16, 2024

This has been fixed in Nix 2.24, but it is risky to backport #11079 because it's a fairly significant change in behavior. Perhaps this documentation could be backported instead?

@roberth roberth changed the base branch from master to 2.23-maintenance August 16, 2024 15:29
@github-actions github-actions bot added documentation new-cli Relating to the "nix" command contributor-experience Developer experience for Nix contributors with-tests Issues related to testing. PRs with tests have some priority store Issues and pull requests concerning the Nix store repl The Read Eval Print Loop, "nix repl" command and debugger fetching Networking with the outside (non-Nix) world, input locking c api Nix as a C library with a stable interface labels Aug 16, 2024
It can be overridden with the [`NIX_PATH` environment variable](@docroot@/command-ref/env-common.md#env-NIX_PATH) or the [`-I` command line option](@docroot@/command-ref/opt-common.md#opt-I).
It can be overridden with the [`-I` command line option](@docroot@/command-ref/opt-common.md#opt-I).

Due to a bug, it overrides the [`NIX_PATH` environment variable](@docroot@/command-ref/env-common.md#env-NIX_PATH) when set.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Due to a bug, it overrides the [`NIX_PATH` environment variable](@docroot@/command-ref/env-common.md#env-NIX_PATH) when set.
Due to a bug in Nix versions ≥2.13 and <2.24, `nix-path` overrides the [`NIX_PATH` environment variable](@docroot@/command-ref/env-common.md#env-NIX_PATH) when set.

Copy link
Author

Choose a reason for hiding this comment

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

Done, thanks!

If nix-path is set in nix.conf, the environment variable is always
ignored.
@fricklerhandwerk
Copy link
Contributor

Yeah I guess backporting the docs is easier than backporting the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c api Nix as a C library with a stable interface contributor-experience Developer experience for Nix contributors documentation fetching Networking with the outside (non-Nix) world, input locking new-cli Relating to the "nix" command repl The Read Eval Print Loop, "nix repl" command and debugger store Issues and pull requests concerning the Nix store with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants