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.

Sign up for GitHub

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

Jump to bottom

flake.nix: NixOS nixpkgs registry pin as nixos #197424

Closed
wants to merge 1 commit into from
Closed

flake.nix: NixOS nixpkgs registry pin as nixos #197424

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions flake.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
system.nixos.versionSuffix =
".${final.substring 0 8 (self.lastModifiedDate or self.lastModified or "19700101")}.${self.shortRev or "dirty"}";
system.nixos.revision = final.mkIf (self ? rev) self.rev;
nix.registry.nixos.flake = "${self}";
Copy link
Member

Choose a reason for hiding this comment

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

  • The nix registry (flake registry?) just reinvents NIX_PATH for the CLI. Both are extra namespaces that are set implicitly through the environment, whether that's an env variable or a non-local file. Instead, we could remove the registry indirection by adding a /etc/nixpkgs/flake.{nix,lock}, so that we can do nix shell /etc/nixpkgs#foo? This makes the dependency obvious.

The other issues are of secondary importance, but I'll list them anyway because they correspond to requirements for a /etc/nixpkgs solution as well.

  • This line defeats lazy trees by unpacking the archive.
  • It does not seem to preserve flake metadata such as sourceInfo.
  • Closure size: this is only relevant for systems that need to be capable of evaluating. Many deployment tools do not require this feature. This includes nixos-rebuild with --target-host.
  • By depending on the Nixpkgs revision, we'll be creating more system generations where nothing changes except the Nixpkgs revision. This could lead to huge boot menus and unnecessary remote activations.

Copy link
Author

@nrdxp nrdxp Oct 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

  • so that we can do nix shell /etc/nixpkgs#foo

That's fine and all, but not quite as convenient as a nixos#foo. I am admittedly bias on this point though, as I was also in favour of NixOS/nix#4438. It would be nice to have a solution for the underlying "local ref in flake.lock breaking everything" from Nix itself though. Not sure what we could do there, maybe a fallback remote uri?

  • This line defeats lazy trees by unpacking the archive.

True, I realized that. I guess this should only be active for interactive installations of NixOS, since in that case, I can't really see a time when a user wouldn't want a local reference to nixpkgs to avoid superflous fetching, and building.

Although I'm not sure there is a nice way to detect that automatically unfortunately. Maybe we need a whole new module for sane interactive defaults. We could also add nix.nixPath = [ "nixpkgs=${pkgs.path}" ] to such a modue as well, for example.

Even so, NixOS has always had a local copy of nixpkgs (i.e. channels) in the past, and it seems like a generally good idea, especially with the eval cache available. For example, nix search nixos from a pinned nixpkgs is typically a lot faster after the first invocation, and is just an all around better experience.

  • It does not seem to preserve flake metadata such as sourceInfo.

I took the recommended workaround mentioned in the lazy-tree's PR, which advises against just flake = self.

  • Closure size: this is only relevant for systems that need to be capable of evaluating. Many deployment tools do not require this feature. This includes nixos-rebuild with --target-host.

see my response to the first point

  • By depending on the Nixpkgs revision, we'll be creating more system generations where nothing changes except the Nixpkgs revision. This could lead to huge boot menus and unnecessary remote activations.

Is this really a risk in practice? nixpkgs changes so often, I've never really ran into a situation where I bumped my pin and nothing changed, but I suppose it's feasible on very minimal systems.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is a reasonable default value, but I have following issues:

  1. The closure size of a NixOS raises by the size of nixpkgs, which is not always wanted.
  2. Default values should not defined in flake.nix.

Copy link
Author

Choose a reason for hiding this comment

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

  1. The closure size of a NixOS raises by the size of nixpkgs, which is not always wanted.

That is true. We might want a toggle, but I didn't do this originally since then we have the same, non-default behavior as before and the user has to going searching for the option, so I figured there isn't much improvement there.

2. Default values should not defined in flake.nix.

Right, but I wanted to only make this the case with flake based configurations, which are the only configs that call this function. Of course, I don't think it would hurt anything for non-flake configs so perhaps we should move this to the module after all.

Copy link
Member

Choose a reason for hiding this comment

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

We're supposed to be the distro with hermeticity and no mutable global state magic. The registry can't be fixed. Do not use it.

Suggested change
nix.registry.nixos.flake = "${self}";

Copy link
Member

Choose a reason for hiding this comment

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

Refs NixOS/nix#7422

Copy link
Author

Choose a reason for hiding this comment

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

This would be my response: NixOS/nix#7422 (comment)

}];
} // lib.optionalAttrs (! args?system) {
# Allow system to be set modularly in nixpkgs.system.
Expand Down