treewide: fail when nixpkgs.config is set with explicit pkgs, remove all nixpkgs.config usages in in-tree modules

[K900]

Description of changes

This gun has been pointed at too many feet now. Kill it.

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:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• 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.

[Gerg-L]

Very nice!

[alois31]

I tested this on my system, and everything still works. Although my system config doesn't make use of any of the places where this was previously defined, the diff looks good to me as well.

[misuzu]

Maybe add a changelog entry?

[K900]

I did?

[K900]

Oops. Had it on the wrong branch.

[K900]

Just realized ofborg doesn't automatically request reviews from affected module maintainers, so I've done that manually. Please check up on your modules and tell me what I did wrong here!

[SuperSandro2000]

Tbh I don't know why we have this. Other packages also just hardcode paths.

[SuperSandro2000]

Maybe I just create a PR dropping this entirely if @peterhoeg does not object

[K900]

I guess someone wanted to put their locate database on some external storage?

[SuperSandro2000]

I think this can be accomplished with symlinks which removes some not really required complexity.

[jtojnar]

👍 for removing the vim option from GNOME. It was introduced to avoid GTK 2 variant but I would hope that is the default these days. If not, we should probably do that – it would be nice to get GTK 2 out of graphical installation ISO.

[arcnmx]

This likely should've started as a warning; it's a rather broken and incomplete way to solve the problem, with no reasonable workarounds available to use while it's broken. Maybe it could assert that cfg.config == {} || cfg.config == pkgs.config to enable workarounds in some way?

[K900]

Why is it broken, and why is it an incomplete way to solve the problem?

[arcnmx]

I mostly just personally disagree with the strict approach when not everyone necessarily has control over whether a possibly out-of-tree module has decided to set an option under config, though I guess you could config = mkForce {} to work around that? It just seems fragile, and the way the meaning and interpretation of the option changes contextually based on how pkgs is passed is awkward.

The reason it's broken though is because not all uses of nixpkgs.config have been removed here:

• nixpkgs/nixos/modules/hardware/all-firmware.nix

  Line 72 in 58aa673

  assertion = !cfg.enableAllFirmware || config.nixpkgs.config.allowUnfree;

• nixpkgs/nixos/modules/programs/firefox.nix

  Line 240 in 58aa673

  nixpkgsConfig = config.nixpkgs.config.${applicationName} or {};

• nixpkgs/nixos/modules/virtualisation/virtualbox-host.nix

  Line 94 in 58aa673

  warnings = mkIf (config.nixpkgs.config.virtualbox.enableExtensionPack or false)

Presumably these need to be changed to use pkgs.config? I haven't done a thorough check but that might be all that's left outside of tests using it?

[K900]

Read accesses to those shouldn't break anything, as the option isn't removed, and the assertion won't be triggered by reading it. However, yes, those all should be changed to pkgs.config, as that is more correct.

[alois31]

It just seems fragile, and the way the meaning and interpretation of the option changes contextually based on how pkgs is passed is awkward.

This was already the case before, in fact that's the entire rationale for this PR.

[arcnmx]

Read accesses to those shouldn't break anything, as the option isn't removed, and the assertion won't be triggered by reading it. However, yes, those all should be changed to pkgs.config, as that is more correct.

Oh I mean broken as in those config options are broken: if pkgs is set explicitly and one wants to set any of the options those module read, you're SoL. There's no migration path available here since the assertion prevents you from setting them temporarily in nixpkgs.config to be read by modules as a workaround.
It's not more correct, it's just currently impossible to set both enableAllFirmware and pkgs since you'll hit a error: attribute 'allowUnfree' missing error from it trying to check for that config entry. All reads and writes must be removed from nixpkgs.

This was already the case before, in fact that's the entire rationale for this PR.

At least one could import <nixpkgs. { inherit (config.nixpkgs) config; } previously. I'd argue this introduces a new footgun now that module authors must beware of an option they should never set or read from - though a gun pointed at nixpkgs contributors may be better than one pointed at users I suppose. Also reading from pkgs.config was likely more correct from the start (assuming that using config is even correct to begin with, which I won't comment on)

[K900]

That's actually a completely separate bug.

[K900]

(and using pkgs.config.allowUnfree does fix it)

[alyssais]

The locate change doesn't work at all:

       error: attribute 'override' missing

       at /home/qyliss/src/nixpkgs/nixos/modules/misc/locate.nix:233:37:

          232|
          233|     environment.systemPackages = [ (cfg.locate.override { dbfile = cfg.output; }) ];
             |                                     ^
          234|

(Config is just services.locate.enable = true;.)