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

nixos/flake: set up NIX_PATH and system flake registry automatically #254405

Merged
merged 1 commit into from
Feb 25, 2024

Conversation

lf-
Copy link
Member

@lf- lf- commented Sep 10, 2023

Currently there are a bunch of really wacky hacks required to get nixpkgs path correctly set up under flake configs such that nix run nixpkgs#hello and nix run -f '<nixpkgs>' hello hit the nixpkgs that the system was built with. In particular you have to use specialArgs or an anonymous module, and everyone has to include this hack in their own configs.

We can do this for users automatically.

I have tested these manually with a basic config; I don't know if it is even possible to write a nixos test for it since you can't really get a string-with-context to yourself unless you are in a flake context.

cc @RaitoBezarius

Description of changes

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.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.

Copy link
Contributor

@IreneKnapp IreneKnapp left a comment

Choose a reason for hiding this comment

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

overall looks great, I want this!

default = false;

description = mdDoc ''
Whether to create /etc/nix/inputs/nixpkgs as a symlink and set
Copy link
Contributor

Choose a reason for hiding this comment

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

This could use more documentation about where the symlink points to. I mean, I assume it points to the path to the derivation that nixos is being built out of, but there must be an appropriately precise way to say that...

default = false;

description = mdDoc ''
Whether to set nixpkgs in the local flake registry and disable the
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, I assume this points to the derivation nixos is built out of, but find some precise way to say that. Also specify explicitly whether this option relies on the /etc/nix/inputs/nixpkgs symlink to exist or not.

Also, it's called the system registry not the local registry. Since that term is vague and confusing and doesn't help a user to understand why they'd want this option turned on, by all means keep the present wording about "local" as well, but do use it in conjunction with the official name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, not quite the one nixos is built out of, which means my docs are bad. I will write better ones than I wrote while originally writing this whilst tired at nixcon tomorrow :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently I misunderstood what you wrote because it was 2am. This option doesn't require /etc/nix/inputs/nixpkgs to exist, and is completely independent of the other option.

Copy link
Contributor

Choose a reason for hiding this comment

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

it sure was 2am <3

@lf- lf- force-pushed the jade/nix-path-flakes branch 3 times, most recently from e0c648f to bc3b186 Compare October 2, 2023 21:42
@lf-
Copy link
Member Author

lf- commented Oct 2, 2023

Alright, I have pushed a change rewriting the documentation to have examples and to use more clear wording. I have also turned it on by default.

The reasoning for enabling it by default on flake configurations is that we do things that are suboptimal for closure size by default, for example environment.noXlibs = false;, documentation.enable = true;, and it seems much more important to make the out of the box experience better for machines that might be used interactively.

This change only affects systems built with flakes.

@lf- lf- requested a review from IreneKnapp October 2, 2023 21:43
@lf- lf- changed the title nixos/flake: options to set up registry and nixPath for flake configs nixos/flake: setup NIX_PATH and system flake registry automatically Oct 2, 2023
Copy link
Contributor

@IreneKnapp IreneKnapp left a comment

Choose a reason for hiding this comment

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

looks solid to me! note that this is by way of peer review, I'm not a committer, somebody else will need to approve it

@@ -332,6 +332,16 @@ in
always allowed to connect.
'';
};

flake-registry = mkOption {
type = types.str;
Copy link
Contributor

Choose a reason for hiding this comment

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

don't you mean nullOr str?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I didn't realize null was coerced to empty string.


description = mdDoc ''
Whether to create `/etc/nix/inputs/nixpkgs` as a symlink to the version of nixpkgs that
the NixOS system was built with and set `NIX_PATH` to include `/etc/nix/inputs/nixpkgs`.
Copy link
Contributor

Choose a reason for hiding this comment

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

just to be absolutely clear: the symlink points to a path in the store, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. I will word this more carefully.


description = mdDoc ''
Whether to pin nixpkgs in the system-wide flake registry (`/etc/nix/registry.json`) to the
version of nixpkgs that the NixOS system has been built with.
Copy link
Contributor

Choose a reason for hiding this comment

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

so the understanding I get from this is that it replicates the protocol, URL, etc but pins based on rev. correct? I still feel like there could be a bit more wording to say so explicitly, but it's good as-is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope. It is a string-with-context that introduces a simple dependency:

nix-repl> nixosConfigurations.micro.options.nix.registry.value.nixpkgs.to
{ path = "/nix/store/c8cz4q0y6jw9qvjwk45nf6pvwysj7yn6-source"; type = "path"; }

nix-repl> :p builtins.getContext nixosConfigurations.micro.options.nix.registry.value.nixpkgs.to.path
{ "/nix/store/c8cz4q0y6jw9qvjwk45nf6pvwysj7yn6-source" = { path = true; }; }

Copy link
Contributor

Choose a reason for hiding this comment

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

ah! that's nice and simple. the clearest way to say that might be "by referencing it in the store"?

version of nixpkgs that the NixOS system has been built with.

This also sets a default to disable the global (online) flake registry due to
<https://github.com/NixOS/nix/issues/9087>.
Copy link
Contributor

Choose a reason for hiding this comment

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

ah good call

Copy link
Member

Choose a reason for hiding this comment

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

Bad call: unnecessary breaking change. Why not fix the bug instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't want to try getting anything into Nix after a one line change to fix nix-shell in shebangs by a former coworker continues to sit unreviewed for going on two years now, in spite of repeated prodding for reviews including by back channels.

I understand the Nix team has been improving things, and I think more prompt reviews generally happen now but I'm still not interested.

I can remove the workaround because the bug isn't that bad.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lf- i have an interest in the shebangs features, but I am not finding the referenced shebang PR. (my own effort in bringing shebangs to nix-command took more than two years with 5189, so I understand the patience can get worn out)

Copy link
Member Author

Choose a reason for hiding this comment

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

i had it written down somewhere; it's NixOS/nix#5088

@lf-
Copy link
Member Author

lf- commented Oct 3, 2023

I have done a final pass through the docs. I am not sure if they are too verbose, but even if they are, usually we have the opposite problem and I have rarely thought that of module docs.

@IreneKnapp
Copy link
Contributor

the latest revision of the docs looks great. thanks for indulging me! I know I tend to prefer more verbose docs than others do, but I feel that a lot of flake semantics are under-documented and I'd much rather not have to guess at these details.

@Artturin Artturin requested a review from roberth October 3, 2023 07:11
@Artturin
Copy link
Member

Artturin commented Oct 3, 2023

The diff looks very weird, are there formatting changes or something?

Related PR #197424

@lf-
Copy link
Member Author

lf- commented Oct 3, 2023

The diff looks very weird, are there formatting changes or something?

Related PR #197424

Yes. I moved the whole existing config= contents right one indent, put it in a mkMerge, and added my own things. When I rebased I did these changes manually because git had made such an absolute dog's breakfast of the diff and I couldn't make any sense of it.

I don't know what made git fail so hard on this PR's diff, but I am sorry it is annoying to review.

@Artturin
Copy link
Member

Artturin commented Oct 3, 2023

Hiding the whitespace changes makes it better

image

rightmost icon

Copy link
Contributor

@tomberek tomberek left a comment

Choose a reason for hiding this comment

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

  • blocking: /etc/nix/inputs/nixpkgs is awkward, please examine some other possibilities mentioned above
  • thought: having nixpkgs.flake.setNixPath interacting with nix.nixPath (same with nixpkgs.flake.setFlakeRegistry / nix.registry) might cause confusion. There are then two ways to do the same thing, which takes precedence, etc. Here i don't yet know of solid suggestion; there is merit to having it in both option sets.

nixos/modules/misc/nixpkgs.nix Outdated Show resolved Hide resolved
@IreneKnapp
Copy link
Contributor

This is the PR from ages ago that should be merged to fix nix flake init NixOS/templates#67

wait, really? this looks Haskell-specific

@lf-
Copy link
Member Author

lf- commented Feb 15, 2024

This is the PR from ages ago that should be merged to fix nix flake init NixOS/templates#67

wait, really? this looks Haskell-specific

Clicked wrong, meant NixOS/templates#53

Copy link
Member

@RaitoBezarius RaitoBezarius left a comment

Choose a reason for hiding this comment

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

This has been reviewed, scrutinized for a long time, this change makes a lot of sense and improves the statut-quo. Let's iterate from now on.

@RaitoBezarius RaitoBezarius dismissed stale reviews from infinisil and tomberek February 25, 2024 20:07

stale

@RaitoBezarius RaitoBezarius merged commit 5337ff6 into NixOS:master Feb 25, 2024
21 checks passed
Kidsan added a commit to Kidsan/nixos-config that referenced this pull request Feb 28, 2024
@totoroot
Copy link
Contributor

Just for reference, in case someone is looking for the change that caused their flake-based NixOS systems to no longer rebuild.

Since I previously had nix.registry.nixpkgs.flake set in my flake config, I got the following error when trying to rebuild my system:

       error: The option `nix.registry.nixpkgs.to.path' has conflicting definition values:
       - In `/nix/store/1gpgihsi21bni03hy6i5ccw9lfrhnbqi-source/nixos/modules/config/nix-flakes.nix': "/nix/store/84q33bh989jr3hadc8jj2rsz6ax5cqyj-source"
       - In `/nix/store/1gpgihsi21bni03hy6i5ccw9lfrhnbqi-source/nixos/modules/misc/nixpkgs-flake.nix': "/nix/store/1gpgihsi21bni03hy6i5ccw9lfrhnbqi-source"
       Use `lib.mkForce value` or `lib.mkDefault value` to change the priority on any of these definitions.

The suggested use of lib.mkForce or lib.mkDefault is not advisable.
Instead just remove the manual NIX_PATH override in your config, as this is now handled automatically.

If you do not wish to do so, you can also disable the newly added option, as described in the release notes (look for NIX_PATH).

To disable this, set nixpkgs.flake.setNixPath and nixpkgs.flake.setFlakeRegistry to false.

IMO the documentation of this change in the release notes is good, but users might still look for this error in the issues and PRs of the nixpkgs repository, so I thought I'd mention it.

totoroot added a commit to totoroot/dotfiles that referenced this pull request Feb 29, 2024
GaetanLepage added a commit to GaetanLepage/nix-config that referenced this pull request Mar 6, 2024
name-snrl added a commit to name-snrl/nixos-configuration that referenced this pull request Apr 6, 2024
Since <NixOS/nixpkgs#254405> was merged,
`nix.nixPath` is set when you use `lib.nixosSystem`
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/use-a-system-nixpkgs-version-in-a-flake/43784/3

Misterio77 added a commit to Misterio77/nix-starter-configs that referenced this pull request Apr 27, 2024
- Don't auto-optimise-store by default
    - We should avoid arbitrary "sane defaults" in this template, as nixpkgs already has pretty sane defaults nowadays
- Simplify registry+nixPath
    - Instead of using a file as indirection (to avoid staleness), indirect it through the registry using `flake:nixpkgs`
    - Add a comment about NixOS/nixpkgs#254405, which all this by default for nixpkgs
- Disable channels and flake registry by default
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/python-applications-set-pythonpath-how-is-that-not-a-nix-anti-pattern/45736/4

@arianvp
Copy link
Member

arianvp commented Jun 8, 2024

Hmm should we also enable flakes when using nixpkgs.lib.nixosSystem? Since this PR landed you get the following super confusing error message when you try to use nix-shell -p on a NixOS system built with lib.nixpkgs.nixosSystem.

nix-shell -p aws
error:
       … <borked>

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

       … while calling the 'import' builtin

         at «string»:1:18:

            1| {...}@args: with import <nixpkgs> args; (pkgs.runCommandCC or pkgs.runCommand) "shell" { buildInputs = [ (aws) ]; } ""
             |                  ^

       (stack trace truncated; use '--show-trace' to show the full trace)

       error: experimental Nix feature 'flakes' is disabled; use '--extra-experimental-features flakes' to override

[ssm-user@ip-172-31-1-45:/var/log]$ nix-shell -p aws --show-trace
error:
       … while calling anonymous lambda

         at «string»:1:1:

            1| {...}@args: with import <nixpkgs> args; (pkgs.runCommandCC or pkgs.runCommand) "shell" { buildInputs = [ (aws) ]; } ""
             | ^

       error: experimental Nix feature 'flakes' is disabled; use '--extra-experimental-features flakes' to override
       ```

@lf-
Copy link
Member Author

lf- commented Jun 8, 2024

@arianvp probably yes. I've been meaning to submit that exact change for like, months.

@name-snrl
Copy link
Contributor

name-snrl commented Jun 8, 2024

Maybe we just need to replace registry link to a path?

@lf-
Copy link
Member Author

lf- commented Jun 8, 2024

Maybe we just need to replace registry link to a path?

no, that is broken. the environment variable won't be updated in existing processes so you need at minimum a symlink indirection. i had one of those twice in the revision history of this pr if you want to see, and the current method is the simplest and most robust possible with the singular caveat that the resulting system needs flakes enabled.

@DavHau
Copy link
Member

DavHau commented Aug 25, 2024

I appreciate this effort, but as long as nix allows pointing inputs to the local registry, this change practically seems to do more harm than good.

I think we should consider at least turning the setFlakeRegistry off by default and add a big fat warning to the docs about the consequences this can have on flake.lock updates.

I ended up publishing broken flakes for some time now without noticing and I'm probably not the only one.

Implementing a performance improvement (less networking/fetching) with the cost of introducing breakages in the wild doesn't seem to be a tradeoff worthwhile to me.

I understand that there are opinions on how flake inputs should never point to the (local) registry, and I'm not opposed to that.
But still, many people don't know that and the tooling doesn't prevent or warn about it.
At the same time the breakages introduced here are not obvious at all, and only detected after the flake gets published and re-used by another person.

It would be much better if nix itself is fixed first, in order to not allow local registry flake inputs or warn about using them, before breaking many peoples setups by such change.

An alternative approach could be to point the registry override to github:nixos/nixpkgs/${inputs.nixpkgs.rev}.
This will lead to a permanently cached nixpkgs as well without breaking lock file updates.

@lf-
Copy link
Member Author

lf- commented Aug 25, 2024

Here's the lix bug number to do what you describe. https://git.lix.systems/lix-project/lix/issues/170

Don't write flakes that use registry indirect inputs. They were always broken regardless of this change because you have no idea which version of nixpkgs they will update to when someone else runs nix flake update. If you really are motivated, you can set the registry to point to be a github input type and pull the metadata out of flake.lock because I believe it never makes it into inputs or at least definitely doesn't make it into nixpkgs' self so we could never have implemented this here.

I think that people who work on broken flakes regularly should consider setting up the necessary custom code to allow them to do that safely and disable this feature. Or fix the flakes. You can absolutely see when you nix flake update and it did the wrong thing. My point here is that working on broken flakes that use the registry is the exceptional case and I've seen a lot of nix support questions in my time; I believe you to be the second one to complain about this, compared to probably a dozen or more times I've had to tell people how to set up NIX_PATH which this change eliminates completely.

I don't think we should regress the default user experience (which, especially NIX_PATH being correct by default has completely eliminated a whole class of support issues to the extent that I've forgotten this was ever a problem). I would especially not advise blocking these UX improvements on changes in the nix implementation because those do not get to users quickly (even with Lix, releases have a long latency to get fixes out).

@lf-
Copy link
Member Author

lf- commented Aug 25, 2024

As for your suggestion, it's broken if you have a local or private fork of nixpkgs. It's plausible it would cause redownloads as well if that commit is contained within a fork. Shipping it would be a regression, when the actual fix is making the nix implementations refuse to do registry indirect flake inputs.

This isn't just a performance improvement. It's a correctness improvement. It makes it so that if you bring stuff in with nix-shell today and open a nested shell tomorrow, the stuff will be compatible with each other.

@arianvp
Copy link
Member

arianvp commented Aug 25, 2024

FWIW personally I was very surprised that nix run nixpkgs#hello didn't give me the latest version of hello on my system. I do not think this is the greatest default and it's also very different from pre-flakes world.

Namely I could do nix-channel --update; nix-shell -p mypackage before adding it to my nixos config to try out if I liked the new version.

nix shell nixpkgs#mypackage does not do the same anymore. It always gets an old version.

For that reason I would also prefer setting setFlakeRegistry to false by default.

@lf-
Copy link
Member Author

lf- commented Aug 25, 2024

To me, having versions of nixpkgs mismatched with the config is always unexpected behaviour because I expect nixos-rebuild switch to always be a no-op unless I changed something visible to git. It's very easy to e.g. do a channel update and forget to run nixos-rebuild (or it fails) and have it be non obvious this has happened. A workflow where there's only one nixpkgs at play is more intuitive for new users, as evidenced by flake users having channel problems completely vanishing from support rooms after this change.

If I wanted the latest version of something, I would use, pre flakes, nix-shell -I nixpkgs=channel:nixos-unstable -p blah or with flakes, nix shell github:nixos/nixpkgs/nixos-unstable#blah. My viewpoint is the computer should never do anything under your nose so if you have a command that did something one day and then you go to bed and run it again the next day, it should do the same thing and you should have to ask for something different if that's desired.

Semantically, nixpkgs in the registry and <nixpkgs> mean "give me a nixpkgs, don't worry about which one". And there's two logical choices, either latest (modulo caching behaviour!! which makes it more confusing) or the one you have pinned.

Now that being said, the detsys and lix installer (if flakes are on) both set nixpkgs=flake:nixpkgs in NIX_PATH because channels have horrible UX. It also doesn't automatically pin it for you. But this is sensible for UX: "give me some nixpkgs" should mean the same thing across flakes and non flakes and it was confusing it wasn't the case before, regardless of what it's actually pointed to and whether it's pinned or not.

If you don't like it, turn it off. But I would really like to preserve the status quo in which this support problem is gone and the average user doesn't have to think about more than one version of nixpkgs.

rvolosatovs added a commit to rvolosatovs/nixelium that referenced this pull request Sep 18, 2024
rvolosatovs added a commit to rvolosatovs/nixelium that referenced this pull request Sep 18, 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.