nixos/system-path: convert environment.systemPackages to an attrset by r-vdp · Pull Request #255086 · NixOS/nixpkgs

r-vdp · 2023-09-14T10:01:37Z

Description of changes

Lists are pretty bad for overriding, this PR proposes to use an attribute set for environment.systemPackages so that we can more easily disable packages or override the derivation for a package.

In order not to break the current API, we use the coercedTo type so that we can still accept a list of derivations and we convert this list into an attribute set. This should therefore be a non-breaking change.

We also allow for a shorthand syntax:

environment.systemPackages = {
  inherit (pkgs) acl;
};

Which gets coerced into

environment.systemPackages = {
  acl = {
    enable = true;
    package = pkgs.acl;
    outputs = {
      bin = true;
      man = true;
    };
  };
};

where the outputs were taken from meta.outputsToInstall and can be individually disabled (hence the attrset that maps every output to a boolean).

The nixos test gives a good overview of what the syntax would look like.

@roberth @infinisil @nikstur : I'd love to get your thoughts on this.

nikstur · 2023-09-14T12:38:22Z

You should mark this as a draft if you're still working on it.

r-vdp · 2023-09-14T12:46:13Z

Right, I removed the WIP label and converted to draft for now.

I'd want to do a bit more tidying up before merging, but before spending more time on this I'd first like to know that people agree with the approach.

nikstur · 2023-09-14T12:50:35Z

I think the approach is really good. I've been discussing with some people that the systemPackages should be an overridable attrset but never acted on it, so I'm happy that someone else picked it up.

The attrset way of declaring a package is a little verbose, but I'd assume that most people are just using the list way, so its fine.

nixos/modules/config/system-path.nix

lib/types.nix

r-vdp · 2023-09-14T22:45:46Z

@nikstur after implementing the suggestions from @infinisil, the way to declare a package using the attrset approach became a lot more convenient, as you can see in the test case. We can deduce the outputs automatically now.

nixos/modules/config/system-path.nix

lib/types.nix

nixos/modules/config/system-path.nix

roberth · 2023-09-20T16:01:22Z

I think the following definition leads to an unfortunate error message:

systemPackages = pkgs.hello;

hello is an attrset, so its package attributes will be treated as packages instead. I think we should detect this mistake in the type and report that it should be systemPackages.hello = <pkg>.

roberth · 2023-09-21T10:52:43Z

This also needs a release note and an update to the option description to clarify what the attribute name is for.

infinisil

This PR currently adds 4 non-trivial, untested and undocumented lib functions. Either those should be done in smaller PRs (see also the new lib PR guidelines), or we shouldn't expose them publicly, as hinted by Robert.

roberth

Some clarifications needed

roberth · 2024-01-16T11:47:44Z

nixos/doc/manual/release-notes/rl-2405.section.md

I checked several of the rebuilds and they are due to the ordering of the paths in the resulting list. This was in definition order before, and is in alphabetical order now, due to attrsets being sorted.

I believe this is important enough to be added to the release note.
While the definition order is sufficiently "unpredictable" that it should not be relied upon, that doesn't exclude the possibility that some configurations happen to rely on it by accident.
Could you add an item to describe this breaking aspect, in the Backward Incompatibilities section below?

roberth · 2024-01-16T11:54:31Z

nixos/doc/manual/release-notes/rl-2405.section.md

Suggested change

This change is backwards compatible, but we might deprecate the list syntax in a future release.

This change is backwards compatible, unless the configuration relies on a specific ordering of packages. We might deprecate the list syntax in a future release.

Suppose I want util-linux for obvious reasons, but I also want my own custom mount executable from a custom package. How do I achieve that? After reading this, it
d looks like I'm stuck, or I'd have to meddle with the util-linux definition. I think this also needs to reference the priority option.

Please describe an alternative to list ordering here and in the option doc.

roberth · 2024-01-16T12:00:08Z

nixos/modules/config/system-path.nix

Could you describe the effect of priority?
E.g. what does a large value do? Do packages without a priority have a default priority?
I think you could link to https://nixos.org/manual/nixpkgs/stable/#var-meta-priority, but that doc isn't much better.
Maybe this? https://nixos.org/manual/nix/stable/command-ref/nix-env/install.html?highlight=priority#description

I think it'd be ok to copy some of this information, because

NixOS/nixpkgs duplicates this logic anyway

It's not going to change in Nix, because that'd break profiles for no good reason

SuperSandro2000 · 2024-01-16T20:51:40Z

I am sorry if that was already answered in some thread, I didn't read all of them but what happens to packages that where before duplicated by name in the list but not by the files they contain?

r-vdp · 2024-01-16T23:25:41Z

I am sorry if that was already answered in some thread, I didn't read all of them but what happens to packages that where before duplicated by name in the list but not by the files they contain?

What do you mean exactly by "the files they contain"? Packages added through the list syntax, end up in the attrset with their drv path as key, so unless they come from the same derivation, they would end up under different keys. If they do come from the same derivation, then their outputs will be combined in the list of outputs to install.

Does that answer your question?

SuperSandro2000 · 2024-01-16T23:39:58Z

Yeah, I first thought they would be using the package variable as a key which isn't unique but the drv path must be unique. 👍🏼

This allows easily disabling or overriding a package.

… that have different priorities

r-vdp · 2024-01-25T11:53:54Z

There's currently still an issue with bash being selected instead of bashInteractive, I need to find the time to figure out why that is.

nixos-discourse · 2024-04-15T10:18:33Z

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

https://discourse.nixos.org/t/question-about-defining-environment-systempackages-via-builtins-attrvalues-versus-direct-list/43410/3

nyabinary · 2024-08-22T00:32:52Z

There's currently still an issue with bash being selected instead of bashInteractive, I need to find the time to figure out why that is.

Any update on this? :3

nixos-discourse · 2025-01-20T13:09:40Z

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

https://discourse.nixos.org/t/best-practices-with-with/58857/20

RazYang · 2025-04-01T03:38:25Z

Any progress?

nixos-discourse · 2025-04-30T22:04:03Z

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

https://discourse.nixos.org/t/breaking-changes-announcement-for-unstable/17574/82

nixos-discourse · 2026-01-22T04:10:56Z

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

https://discourse.nixos.org/t/from-a-nixos-module-is-it-possible-to-delete-an-entry-from-an-attrset-defined-by-a-different-module/73289/17

r-vdp requested review from edolstra and infinisil as code owners September 14, 2023 10:01

r-vdp force-pushed the system_packages_as_attr_set branch from 74a0c32 to 02edf48 Compare September 14, 2023 10:04

infinisil added the 1.severity: significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc. label Sep 14, 2023

r-vdp force-pushed the system_packages_as_attr_set branch from 02edf48 to f0ce34f Compare September 14, 2023 10:18

ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Sep 14, 2023

r-vdp changed the title ~~[WIP] nixos/system-path: convert environment.systemPackages to an attrset~~ nixos/system-path: convert environment.systemPackages to an attrset Sep 14, 2023

r-vdp marked this pull request as draft September 14, 2023 12:44

infinisil requested changes Sep 14, 2023

View reviewed changes

nixos/modules/config/system-path.nix Outdated Show resolved Hide resolved

nixos/modules/config/system-path.nix Outdated Show resolved Hide resolved

lib/types.nix Outdated Show resolved Hide resolved

r-vdp force-pushed the system_packages_as_attr_set branch 2 times, most recently from a2d8e5d to 176d3ca Compare September 14, 2023 17:05

r-vdp requested review from infinisil and nikstur September 14, 2023 22:46

r-vdp force-pushed the system_packages_as_attr_set branch from 4ab1693 to c1d6dfe Compare September 14, 2023 22:51

roberth reviewed Sep 15, 2023

View reviewed changes

nixos/modules/config/system-path.nix Outdated Show resolved Hide resolved

nixos/modules/config/system-path.nix Outdated Show resolved Hide resolved

nixos/modules/config/system-path.nix Outdated Show resolved Hide resolved

roberth reviewed Sep 20, 2023

View reviewed changes

lib/types.nix Outdated Show resolved Hide resolved

nixos/modules/config/system-path.nix Outdated Show resolved Hide resolved

nixos/modules/config/system-path.nix Outdated Show resolved Hide resolved

github-actions bot removed 6.topic: module system About "NixOS" module system internals 6.topic: lib The Nixpkgs function library labels Sep 21, 2023

r-vdp force-pushed the system_packages_as_attr_set branch from e8d72ef to 6c86b56 Compare January 12, 2024 09:14

infinisil reviewed Jan 15, 2024

View reviewed changes

r-vdp force-pushed the system_packages_as_attr_set branch from 6c86b56 to e119b86 Compare January 15, 2024 23:04

roberth reviewed Jan 16, 2024

View reviewed changes

r-vdp force-pushed the system_packages_as_attr_set branch 2 times, most recently from b350003 to 209683e Compare January 19, 2024 08:55

r-vdp added 6 commits January 25, 2024 12:46

lib.derivations: init defaultOutput, outputsFor, setOutputsToInstall

cc35e3a

lib.types: init packageByOutPath

ac776d6

lib.meta: init setPrioRecursively

df954e5

build-support/buildenv: use lib.derivations.outputsFor

33868e4

system-path: convert environment.systemPackages to an attrset

2efad5a

This allows easily disabling or overriding a package.

nixos/system-path: correctly deal with packages with multiple outputs…

4c10a51

… that have different priorities

r-vdp force-pushed the system_packages_as_attr_set branch from 209683e to 4c10a51 Compare January 25, 2024 11:48

wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 20, 2024

wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 2, 2025

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 1, 2025

drupol marked this pull request as draft May 14, 2025 05:45

nixpkgs-ci bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 28, 2025

roberth added this to Nixpkgs Lib Oct 28, 2025

nixpkgs-ci bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 22, 2026

	This change is backwards compatible, but we might deprecate the list syntax in a future release.
	This change is backwards compatible, unless the configuration relies on a specific ordering of packages. We might deprecate the list syntax in a future release.

Uh oh!

Conversation

r-vdp commented Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Uh oh!

nikstur commented Sep 14, 2023

Uh oh!

r-vdp commented Sep 14, 2023

Uh oh!

nikstur commented Sep 14, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

r-vdp commented Sep 14, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

roberth commented Sep 20, 2023

Uh oh!

roberth commented Sep 21, 2023

Uh oh!

infinisil left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roberth left a comment

Choose a reason for hiding this comment

Uh oh!

roberth Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

roberth Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

roberth Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

SuperSandro2000 commented Jan 16, 2024

Uh oh!

r-vdp commented Jan 16, 2024

Uh oh!

SuperSandro2000 commented Jan 16, 2024

Uh oh!

r-vdp commented Jan 25, 2024

Uh oh!

nixos-discourse commented Apr 15, 2024

Uh oh!

nyabinary commented Aug 22, 2024

Uh oh!

nixos-discourse commented Jan 20, 2025

Uh oh!

RazYang commented Apr 1, 2025

Uh oh!

nixos-discourse commented Apr 30, 2025

Uh oh!

nixos-discourse commented Jan 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

r-vdp commented Sep 14, 2023 •

edited

Loading

infinisil left a comment •

edited

Loading