Skip to content

lib.addMetaAttrs: use overrideAttrs when available#423615

Merged
winterqt merged 1 commit intoNixOS:masterfrom
winterqt:push-rzpnszxrqoty
Sep 2, 2025
Merged

lib.addMetaAttrs: use overrideAttrs when available#423615
winterqt merged 1 commit intoNixOS:masterfrom
winterqt:push-rzpnszxrqoty

Conversation

@winterqt
Copy link
Member

@winterqt winterqt commented Jul 8, 2025

Previously, any subsequent change to a derivation after an addMetaAttrs call would cause said attribute(s) to be unconditionally reset back to their previous value(s):

nix-repl> ((oldPkgs.lib.lowPrio oldPkgs.hello).overrideAttrs {a = 1;}).meta.priority
error:
    … while evaluating the attribute 'meta.priority'
        at /nix/store/nxdiklm6dnf9pma1zg7srls2nzrykrjf-nixos/nixos/pkgs/stdenv/generic/make-derivation.nix:846:17:
        845|         inherit passthru overrideAttrs;
        846|         inherit meta;
            |                 ^
        847|       }

    error: attribute 'priority' missing
    at «string»:1:67:
            1| ((oldPkgs.lib.lowPrio oldPkgs.hello).overrideAttrs {a = 1;}).meta.priority
            |                                                                   ^

This change makes it so that this does not happen (unless, of course, a subsequent override does affect those values):

nix-repl> ((newPkgs.lib.lowPrio oldPkgs.hello).overrideAttrs {a = 1;}).meta.priority
10

Fixes #323624.


Note: This work was funded by Antithesis.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • Nixpkgs 25.11 Release Notes (or backporting 25.05 Nixpkgs Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
  • NixOS 25.11 Release Notes (or backporting 25.05 NixOS Release notes)
    • (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, pkgs/README.md, maintainers/README.md and other contributing documentation in corresponding paths.

Add a 👍 reaction to pull requests you find important.

@winterqt
Copy link
Member Author

winterqt commented Jul 8, 2025

One thing I wasn't sure about was how to cleanly get a callPackage in a test, so that this can be properly tested; though we can ofc skip testing given it's in misc.

@nixpkgs-ci nixpkgs-ci bot added 6.topic: lib The Nixpkgs function library 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. labels Jul 8, 2025
Copy link
Contributor

@andrewhamon andrewhamon left a comment

Choose a reason for hiding this comment

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

This technique is identical to that used in readTree, which has been serving me well in Discord's monorepo for over a year now. The only issue we have with readTree is the interaction with addMetaAttrs, which this PR fixes.

@nixpkgs-ci nixpkgs-ci bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Jul 9, 2025
Copy link
Member

@alyssais alyssais left a comment

Choose a reason for hiding this comment

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

Makes sense to me

@nixpkgs-ci nixpkgs-ci bot added 12.approvals: 2 This PR was reviewed and approved by two persons. and removed 12.approvals: 1 This PR was reviewed and approved by one person. labels Jul 9, 2025
@eclairevoyant
Copy link
Contributor

Was this change tested with other meta attrs? Like position, license, etc.?

@winterqt
Copy link
Member Author

winterqt commented Aug 27, 2025

meta.position is ✨ special ✨ in that it gets ~unconditionally overridden in check-meta:

nix-repl> ((lib.addMetaAttrs {position = "meow";} pkgs.hello).overrideAttrs {a = 1;}).meta.position
"/home/winter/src/nixpkgs/pkgs/by-name/he/hello/package.nix:47"

nix-repl> ((lib.addMetaAttrs {license = "meow";} pkgs.hello).overrideAttrs {a = 1;}).meta.license
"meow"

The way around this is to override the pos attr instead, which is used in mkDerivationSimple to set the value of meta.position:

nix-repl> ((pkgs.hello.overrideAttrs {pos = "meow";})).meta.position
error:
       … while evaluating the attribute 'meta.position'
         at /home/winter/src/nixpkgs/pkgs/stdenv/generic/check-meta.nix:616:7:
          615|     // optionalAttrs (pos != null) {
          616|       position = pos.file + ":" + toString pos.line;
             |       ^
          617|     }while selecting 'file'
         at /home/winter/src/nixpkgs/pkgs/stdenv/generic/check-meta.nix:616:22:
          615|     // optionalAttrs (pos != null) {
          616|       position = pos.file + ":" + toString pos.line;
             |                      ^
          617|     }

       error: expected a set but found a string: "meow"

@winterqt
Copy link
Member Author

Removing backport, not sure I trust this to not be a breaking change out-of-tree, even if the dependence on it is of course incorrect.

Previously, any subsequent change to a derivation after
an `addMetaAttrs` call would cause said attribute(s) to
be unconditionally reset back to their previous value(s):

    nix-repl> ((oldPkgs.lib.lowPrio oldPkgs.hello).overrideAttrs {a = 1;}).meta.priority
    error:
        … while evaluating the attribute 'meta.priority'
            at /nix/store/nxdiklm6dnf9pma1zg7srls2nzrykrjf-nixos/nixos/pkgs/stdenv/generic/make-derivation.nix:846:17:
            845|         inherit passthru overrideAttrs;
            846|         inherit meta;
                |                 ^
            847|       }

        error: attribute 'priority' missing
        at «string»:1:67:
                1| ((oldPkgs.lib.lowPrio oldPkgs.hello).overrideAttrs {a = 1;}).meta.priority
                |                                                                   ^

This change makes it so that this does not happen (unless, of
course, a subsequent override does affect those values):

    nix-repl> ((newPkgs.lib.lowPrio oldPkgs.hello).overrideAttrs {a = 1;}).meta.priority
    10

Fixes NixOS#323624.
@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. and removed 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Aug 27, 2025
@winterqt
Copy link
Member Author

Rebased for posterity/the hell of it.

Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

Makes sense to me.

@winterqt
Copy link
Member Author

I'm not sure I like that the changed package count is >0, and that it went up from July.

@nixpkgs-ci nixpkgs-ci bot added 12.approvals: 3+ This PR was reviewed and approved by three or more persons. and removed 12.approvals: 2 This PR was reviewed and approved by two persons. labels Aug 27, 2025
@philiptaron
Copy link
Contributor

philiptaron commented Aug 27, 2025

I'm not sure I like that the changed package count is >0, and that it went up from July.

Here's the link to the specifics.

https://github.com/NixOS/nixpkgs/actions/runs/17278235558/attempts/1#summary-49040691560

attic-server

The environments do not match:

- NIX_MAIN_PROGRAM=attic
+ NIX_MAIN_PROGRAM=atticd

dejavu_fontsEnv

The environments do not match:

    pkgs=''
- [{"paths":["/nix/store/jm20pdjzh3fg070nfnlw59hbyapf0l60-dejavu-fonts-2.37"],"priority":5}]
+ [{"paths":["/nix/store/jm20pdjzh3fg070nfnlw59hbyapf0l60-dejavu-fonts-2.37"],"priority":10}]
''

dig

The environments do not match:

+ NIX_MAIN_PROGRAM=dig

envision

Huge diff, concerning, haven't looked at the derivation.

nixos-install-tools

Rebuilt because there's a different lib path. This one is OK.

nixpkgs-manual

Rebuilt because there's a different lib path. This one is OK.

pihole

Rebuilt because dig.

pihole-web

Rebuilt because dig

tests.nixos-functions.nixos-test

Rebuilt because there's a different lib path. This one is OK.

vlagent

The environments do not match:

- NIX_MAIN_PROGRAM=victoria-logs
+ NIX_MAIN_PROGRAM=vlagent

vmagent

The environments do not match:

- NIX_MAIN_PROGRAM=victoria-metrics
+ NIX_MAIN_PROGRAM=vmagent

@philiptaron
Copy link
Contributor

Of these, only envision looks like a "wat" to me.

@winterqt
Copy link
Member Author

winterqt commented Aug 27, 2025

Thanks! I’ll take a look at this tomorrow, though it looks like all of these are just making the env more correct! (Besides the scary one, of course.)

(I saw the changed attribute names @philiptaron, but how did you get the nix-diff output?)

@philiptaron
Copy link
Contributor

philiptaron commented Aug 27, 2025

how did you get the nix-diff output

I did a git fetch upstream pull/423615/head:pr-423615/addMetaAttrs to get a local branch, then did a loop over the changed names with a little bash script that did

  1. git checkout pr-423615/addMetaAttrs
  2. DRV_AFTER=$(nix-instantiate -A $name)
  3. git checkout HEAD^1
  4. DRV_BEFORE=$(nix-instantiate -A $name)
  5. nix-diff --line-oriented --color=never $DRV_BEFORE $DRV_AFTER > /tmp/$name.diff

Then I looked at them.

@philiptaron
Copy link
Contributor

OK, envision wasn't scary either. Inside of a rootfs-builder derivation, it passed derivations information through. I extracted that with nix derivation show.

diff -U4 <(nix derivation show $drvBefore | jq) <(nix derivation show $drvAfter | jq)
           "paths": [
             "/nix/store/cngwgf1q9jxx6k5dgk9j91kvpqzymdj2-v4l-utils-1.30.1-dev",
             "/nix/store/cngwgf1q9jxx6k5dgk9j91kvpqzymdj2-v4l-utils-1.30.1-dev"
           ],
-          "priority": 5
+          "priority": 10
         },
         {
           "paths": [
             "/nix/store/b5r4kj91isan88a0sff3y5ax26vpxzsf-libxau-1.0.12-dev",

Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

After diving into rebuilds, I believe this is just straightforward goodness.

@winterqt
Copy link
Member Author

winterqt commented Sep 2, 2025

OK, envision wasn't scary either. Inside of a rootfs-builder derivation, it passed derivations information through. I extracted that with nix derivation show.

diff -U4 <(nix derivation show $drvBefore | jq) <(nix derivation show $drvAfter | jq)
           "paths": [
             "/nix/store/cngwgf1q9jxx6k5dgk9j91kvpqzymdj2-v4l-utils-1.30.1-dev",
             "/nix/store/cngwgf1q9jxx6k5dgk9j91kvpqzymdj2-v4l-utils-1.30.1-dev"
           ],
-          "priority": 5
+          "priority": 10
         },
         {
           "paths": [
             "/nix/store/b5r4kj91isan88a0sff3y5ax26vpxzsf-libxau-1.0.12-dev",

I see this in my diff (expectedly), but doing a nix derivation show /nix/store/gky8cgkrq1z47kazqwkg18v9kh2575vh-envision-2.0.1-fhsenv-rootfs.drv | jq still has the __json field collapsed -- am I missing an easier way to do this than extracting __json and then running that through jq?

@winterqt winterqt merged commit ba37aa7 into NixOS:master Sep 2, 2025
52 of 53 checks passed
@winterqt winterqt deleted the push-rzpnszxrqoty branch September 2, 2025 19:23
@philiptaron
Copy link
Contributor

I don't see the __json field in nix derivation show. What version of Nix are you running? I'm on 2.30.2.

@winterqt
Copy link
Member Author

winterqt commented Sep 2, 2025

I’m on the latest version of Lix. I guess this is something that differs between them!

@eclairevoyant
Copy link
Contributor

This broke tesseract.languages and not sure what else.

@PatrickDaG
Copy link
Contributor

PatrickDaG commented Sep 7, 2025

The reason it broke tesseract is that tesseract is using // to set passthru. Basically pkgs.tesseract // {passthru = {languages = ...;}}.
This PR takes the result from the // and calls overrideAttrs on it which, if I understand it correctly looses these keys introduced by the attr merge.
I think this is just a bad code from in the tesseract package.I'll open a PR to fix it soon.

@PatrickDaG PatrickDaG mentioned this pull request Sep 7, 2025
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: lib The Nixpkgs function library 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. 12.approvals: 3+ This PR was reviewed and approved by three or more persons.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

lib.hiPrio gets erased after calling override or overrideAttrs

6 participants