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

fix splicing in overrideAttrs #201734

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Conversation

Artturin
Copy link
Member

see commit messages

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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

lib/customisation.nix Outdated Show resolved Hide resolved
lib/customisation.nix Outdated Show resolved Hide resolved
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

The root cause of the problem is doing // on an attrset that is supposed to be modified with .overrideAttrs instead.
This happens here:

// (lib.optionalAttrs (pkgsBuildHost ? ${name}) { nativeDrv = valueBuildHost; })
// (lib.optionalAttrs (pkgsHostTarget ? ${name}) { crossDrv = valueHostTarget; })
// {

What this code should do instead, is overrideAttrs with passthru.__spliced. That way the splicing is preserved, but broken, because overrideAttrs doesn't implement your logic yet. For this, we need to make overrideAttrs overridable. You may cherry-pick hercules-ci@43c8b43 for that. Here's a crude example showing that splicing can be made to work:

{ pkgs ? import ./. { }, ... }:
rec {
  inherit pkgs;
  inherit (pkgs) lib;

  addEntangled = origOverrideAttrs: f:
    origOverrideAttrs (
      lib.composeExtensions f (self: super: {
        passthru = super.passthru // {
          entangled = super.passthru.entangled.overrideAttrs f;
          overrideAttrs = addEntangled self.overrideAttrs;
        };
      })
    );

  entangle = pkg1: pkg2: pkg1.overrideAttrs (self: super: {
    passthru = super.passthru // {
      entangled = pkg2;
      overrideAttrs = addEntangled self.overrideAttrs;
    };
  });

  example = entangle pkgs.hello pkgs.figlet;

  overrides1 = example.overrideAttrs (self: super: { pname = "a-better-${super.pname}"; });

  repeatedOverrides = overrides1.overrideAttrs (self: super: { pname = "${super.pname}-with-blackjack"; });

  test =
    assert repeatedOverrides.pname == "a-better-hello-with-blackjack";
    assert repeatedOverrides.entangled.pname == "a-better-figlet-with-blackjack";
    "ok";
}

@roberth
Copy link
Member

roberth commented Nov 18, 2022

Here's a crude example

I'll add that it's very crude. I've untangled the entangle function a bit by floating out addEntangled. See edited example above.
I think the pattern could be extracted into a higher order function, but I don't know if that will make things better or worse. Maybe it's best to adapt it to __spliced and avoid the indirection.

@Artturin
Copy link
Member Author

Thank you @roberth!!

This test passes

let
  pkgs = ((builtins.getFlake (toString ./.)).legacyPackages.${builtins.currentSystem});
  pkgsCross = pkgs.pkgsCross.aarch64-multiplatform.__splicedPackages;
  lib = pkgs.lib;
  test = let
    pythonInNativeBuildInputs = lib.elemAt pkgsCross.python3Packages.xpybutil.nativeBuildInputs 0;
    overridenAttr = pkgsCross.hello.overrideAttrs (_: { something = "hello"; });
    #overridenAttr = (pkgsCross.hello.overrideAttrs (previousAttrs: { something = "hello"; })) // { __spliced = { buildHost = { something = "hello"; }; };};
  in {
    shouldBeNative = pythonInNativeBuildInputs.stdenv.hostPlatform.system == "x86_64-linux";
    notCrossOverriden = pkgs.hello.overrideAttrs (_: _: { passthru = { o = "works"; }; });
    inherit overridenAttr;
    overridenAttrShouldHaveSpliced = overridenAttr ? __spliced;
    overridenAttrShouldHaveSplicedAndSomething = if overridenAttr ? __spliced then overridenAttr.something == overridenAttr.__spliced.buildHost.something else false;

  };
in
  assert test.shouldBeNative == true;
  assert test.overridenAttrShouldHaveSpliced == true;
  assert test.overridenAttrShouldHaveSplicedAndSomething == true;
  assert test.notCrossOverriden.o == "works";
  test

@Artturin
Copy link
Member Author

Artturin commented Nov 19, 2022

output getting broken now
before

nix-repl> toString pkgsCross.aarch64-multiplatform.__splicedPackages.zlib.dev
"/nix/store/rdn3ky83pa8xz9prqip7mkw7618jjcv3-zlib-aarch64-unknown-linux-gnu-1.2.13-dev"

nix-repl> toString pkgsCross.aarch64-multiplatform.__splicedPackages.zlib.out
"/nix/store/br4j85mlmiw2qsh2aan1wv7dyz061d80-zlib-aarch64-unknown-linux-gnu-1.2.13"

after

nix-repl> toString pkgsCross.aarch64-multiplatform.__splicedPackages.zlib.dev
"/nix/store/zdx7909kskhm21l89jg62w0qmmgnqjc4-zlib-aarch64-unknown-linux-gnu-1.2.13"

nix-repl> toString pkgsCross.aarch64-multiplatform.__splicedPackages.zlib.out
"/nix/store/zdx7909kskhm21l89jg62w0qmmgnqjc4-zlib-aarch64-unknown-linux-gnu-1.2.13"

hmm

nix-repl> toString pkgsCross.aarch64-multiplatform.__splicedPackages.zlib.__spliced.hostTarget.dev
"/nix/store/9c7539rzxnjy0630c09pda4nmsp55x80-zlib-aarch64-unknown-linux-gnu-1.2.13-dev"

nix-repl> toString pkgsCross.aarch64-multiplatform.__splicedPackages.zlib.dev
"/nix/store/zdx7909kskhm21l89jg62w0qmmgnqjc4-zlib-aarch64-unknown-linux-gnu-1.2.13"

nix-repl> toString pkgsCross.aarch64-multiplatform.__splicedPackages.zlib
"/nix/store/zdx7909kskhm21l89jg62w0qmmgnqjc4-zlib-aarch64-unknown-linux-gnu-1.2.13"

nix-repl> toString pkgsCross.aarch64-multiplatform.__splicedPackages.zlib.__spliced.hostTarget
"/nix/store/zdx7909kskhm21l89jg62w0qmmgnqjc4-zlib-aarch64-unknown-linux-gnu-1.2.13"

@roberth
Copy link
Member

roberth commented Nov 19, 2022

Pushed a fix.

nix-repl> toString pkgsCross.aarch64-multiplatform.__splicedPackages.zlib.dev  
"/nix/store/rdn3ky83pa8xz9prqip7mkw7618jjcv3-zlib-aarch64-unknown-linux-gnu-1.2.13-dev

The TODOs in it are out of scope for this PR.

@Artturin
Copy link
Member Author

Awesome, thanks.

@@ -19,9 +19,9 @@ let
ff = f origArgs;
overrideWith = newArgs: origArgs // (if pkgs.lib.isFunction newArgs then newArgs origArgs else newArgs);
in
if builtins.isAttrs ff then (ff // {
if builtins.isAttrs ff then ff.overrideAttrs (previousAttrs: { passthru = (previousAttrs.passthru or {}) // {
Copy link
Member

Choose a reason for hiding this comment

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

For performance, it'd be better to composeExtensions (or similar) this to the origArgs. That way we don't evaluate all python packages twice.

@Artturin
Copy link
Member Author

Some overrides still missing
pkgsCross.aarch64-multiplatform.nix:

nix-repl> pkgsCross.aarch64-multiplatform.__splicedPackages.nix.boehmgc.configureFlags
[ "--enable-cplusplus" "--with-libatomic-ops=none" "--enable-mmap" "--enable-large-config" "--build=x86_64-unknown-linux-gnu" "--host=aarch64-unknown-linux-gnu" ]

which attr are you talking about? they seem to be there

# merge base
$ git checkout dba0c71b6d15e323d4b8235d9474322520938dc8
$ nix repl .
nix-repl> pkgsCross.aarch64-multiplatform.nix
«derivation /nix/store/dwp0b9ncbwp9h1ypxl42pml3y7vilpx4-nix-aarch64-unknown-linux-gnu-2.11.0.drv»

# this pr
$ git checkout bc5e0b3dae45d858980cead0cb657a38d1be5e22
$ nix repl .
nix-repl> pkgsCross.aarch64-multiplatform.nix                        
«derivation /nix/store/g8hvw7ckadfs1x7ls62zqb7kp2wh36yl-nix-aarch64-unknown-linux-gnu-2.11.0.drv»

$ nix-diff /nix/store/dwp0b9ncbwp9h1ypxl42pml3y7vilpx4-nix-aarch64-unknown-linux-gnu-2.11.0.drv /nix/store/g8hvw7ckadfs1x7ls62zqb7kp2wh36yl-nix-aarch64-unknown-linux-gnu-2.11.0.drv
# nix-diff needs color to be readable.
# `--enable-large-config` and some aws flags went missing as described

So this does appear to be a regression. I don't understand why it doesn't show in attributes.

figured out that this is caused by these

dependencies = map (map lib.chooseDevOutputs) [
[
(map (drv: drv.__spliced.buildBuild or drv) (checkDependencyList "depsBuildBuild" depsBuildBuild))
(map (drv: drv.__spliced.buildHost or drv) (checkDependencyList "nativeBuildInputs" nativeBuildInputs
++ lib.optional separateDebugInfo' ../../build-support/setup-hooks/separate-debug-info.sh
++ lib.optional stdenv.hostPlatform.isWindows ../../build-support/setup-hooks/win-dll-link.sh
++ lib.optionals doCheck checkInputs
++ lib.optionals doInstallCheck' installCheckInputs))
(map (drv: drv.__spliced.buildTarget or drv) (checkDependencyList "depsBuildTarget" depsBuildTarget))
]
[
(map (drv: drv.__spliced.hostHost or drv) (checkDependencyList "depsHostHost" depsHostHost))
(map (drv: drv.__spliced.hostTarget or drv) (checkDependencyList "buildInputs" buildInputs))
]
[
(map (drv: drv.__spliced.targetTarget or drv) (checkDependencyList "depsTargetTarget" depsTargetTarget))
]
];
propagatedDependencies = map (map lib.chooseDevOutputs) [
[
(map (drv: drv.__spliced.buildBuild or drv) (checkDependencyList "depsBuildBuildPropagated" depsBuildBuildPropagated))
(map (drv: drv.__spliced.buildHost or drv) (checkDependencyList "propagatedNativeBuildInputs" propagatedNativeBuildInputs))
(map (drv: drv.__spliced.buildTarget or drv) (checkDependencyList "depsBuildTargetPropagated" depsBuildTargetPropagated))
]
[
(map (drv: drv.__spliced.hostHost or drv) (checkDependencyList "depsHostHostPropagated" depsHostHostPropagated))
(map (drv: drv.__spliced.hostTarget or drv) (checkDependencyList "propagatedBuildInputs" propagatedBuildInputs))
]
[
(map (drv: drv.__spliced.targetTarget or drv) (checkDependencyList "depsTargetTargetPropagated" depsTargetTargetPropagated))
]
];

before we just chose the overriden drv because there were no __spliced after .override
now we're choosing the non .overriden drv from __spliced

Comment on lines 20 to 25
args = lib.fix (lib.extends
(_: previousAttrs: {
passthru = (previousAttrs.passthru or { }) // {
overridePythonAttrs = newArgs: makeOverridablePythonPackage f (overrideWith newArgs);
};
})
(_: origArgs));
Copy link
Member Author

Choose a reason for hiding this comment

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

is this correct @roberth ?

Copy link
Member

Choose a reason for hiding this comment

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

I think the ordering of the extends arguments is ok. "Self" is ignored, so I think the types line up (but I didn't prove it).
Looks about right.

Copy link
Member

Choose a reason for hiding this comment

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

Performance has improved by 2-3%, judging by memory stats before and after "use extends instead of overrideAttrs".
Not enough to make python changes as cheap as the other changes though.
I wonder how much change/noise is caused by master, which I believe is merged into the feature branch before ofborg evaluates.

Copy link
Member Author

Choose a reason for hiding this comment

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

rebased on master again

Copy link
Member Author

Choose a reason for hiding this comment

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

image

Copy link
Member

Choose a reason for hiding this comment

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

We should probably bench this manually anyway, since cross is not part of ofborg's eval I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

image

Copy link
Member Author

@Artturin Artturin Dec 18, 2022

Choose a reason for hiding this comment

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

image

Copy link
Member Author

Choose a reason for hiding this comment

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

current stats image

Comment on lines 106 to 112
# is this even necessary?
# nix-diff shows no difference in cross/non
# nix-diff $(nix eval --raw "nixpkgs/$(git merge-base upstream/master HEAD)#nix.drvPath") $(nix eval --raw ".#nix.drvPath")
# nix-diff $(nix eval --raw "nixpkgs/$(git merge-base upstream/master HEAD)#pkgsCross.aarch64-multiplatform.nix.drvPath") $(nix eval --raw ".#pkgsCross.aarch64-multiplatform.nix.drvPath")
#overrideAttrs = fdrv:
# overrideResult (x: x.overrideAttrs fdrv);
#}
Copy link
Member Author

Choose a reason for hiding this comment

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

the overrideAttrs attr was added in commit 39b64b5

the commit message says

Also update makeOverridable so that uses of overrideAttrs can be
followed by override and overrideDerivation, i.e. they can be
mix-and-matched.

but it seems not necessary anymore

nix-repl> (bash.overrideAttrs(_: { x = "y"; }))
«derivation /nix/store/vk0b51d4bq0ciyrr83nvgrbf51lp5f7s-bash-5.2-p15.drv»

nix-repl> (bash.overrideAttrs(_: { x = "y"; })).override { withDocs = true; }
«derivation /nix/store/y11vah7v46mah4yjj3yj7161fcaq7lkq-bash-5.2-p15.drv»

Copy link
Member Author

@Artturin Artturin Jan 15, 2023

Choose a reason for hiding this comment

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

no overrideAttrs after .override so something like this is necessary

nix-repl> p = (pkgsCross.aarch64-multiplatform.__splicedPackages.bash.overrideAttrs(_: { x = "y"; })).over
ride { withDocs = true; }
nix-repl> p.passthru
{ override = { ... }; overrideDerivation = «lambda @ /home/hm7/nixgits/my-nixpkgs/lib/customisation.nix:96:34»; shellPath = "/bin/bash"; }

just uncommenting it gives

$ nix build ".#sway" --show-trace
error:
       … while calling the 'derivationStrict' builtin

         at <nix/derivation-internal.nix>:9:12:

            8|
            9|   strict = derivationStrict drvAttrs;
             |            ^
           10|

       … while evaluating the attribute 'paths' of the derivation 'sway-1.8'

       … while evaluating the attribute 'text' of the derivation 'sway'

       … while evaluating the attribute 'buildInputs' of the derivation 'sway-unwrapped-1.8'

       … while evaluating the attribute 'buildInputs' of the derivation 'pango-1.50.12'

       … while evaluating the attribute 'buildCommand' of the derivation 'gobject-introspection-wrapped-1.74.0'

       … from call site

         at /home/artturin/nixgits/my-nixpkgs/pkgs/development/libraries/gobject-introspection/wrapper.nix:100:11:

           99|         eval fixupPhase
          100|         ${lib.concatMapStrings (output: ''
             |           ^
          101|           mkdir -p ${"$" + "${output}"}

       … while calling 'concatMapStrings'

         at /home/artturin/nixgits/my-nixpkgs/lib/strings.nix:55:25:

           54|   */
           55|   concatMapStrings = f: list: concatStrings (map f list);
             |                         ^
           56|

       … while calling anonymous lambda

         at /home/artturin/nixgits/my-nixpkgs/pkgs/development/libraries/gobject-introspection/wrapper.nix:100:33:

           99|         eval fixupPhase
          100|         ${lib.concatMapStrings (output: ''
             |                                 ^
          101|           mkdir -p ${"$" + "${output}"}

       … while evaluating the attribute 'buildInputs' of the derivation 'gobject-introspection-1.74.0'

       … while calling 'getOutput'

         at /home/artturin/nixgits/my-nixpkgs/lib/attrsets.nix:827:23:

          826|   */
          827|   getOutput = output: pkg:
             |                       ^
          828|     if ! pkg ? outputSpecified || ! pkg.outputSpecified

       … while calling anonymous lambda

         at /home/artturin/nixgits/my-nixpkgs/pkgs/stdenv/generic/make-derivation.nix:228:13:

          227|       (map (drv: drv.__spliced.hostHost or drv) (checkDependencyList "depsHostHost" depsHostHost))
          228|       (map (drv: drv.__spliced.hostTarget or drv) (checkDependencyList "buildInputs" buildInputs))
             |             ^
          229|     ]

       … while calling anonymous lambda

         at /home/artturin/nixgits/my-nixpkgs/lib/lists.nix:117:29:

          116|   */
          117|   imap1 = f: list: genList (n: f (n + 1) (elemAt list n)) (length list);
             |                             ^
          118|

       … from call site

         at /home/artturin/nixgits/my-nixpkgs/lib/lists.nix:117:32:

          116|   */
          117|   imap1 = f: list: genList (n: f (n + 1) (elemAt list n)) (length list);
             |                                ^
          118|

       … while calling anonymous lambda

         at /home/artturin/nixgits/my-nixpkgs/pkgs/stdenv/generic/make-derivation.nix:199:81:

          198|   checkDependencyList = checkDependencyList' [];
          199|   checkDependencyList' = positions: name: deps: lib.flip lib.imap1 deps (index: dep:
             |                                                                                 ^
          200|     if lib.isDerivation dep || isNull dep || builtins.typeOf dep == "string" || builtins.typeOf dep == "path" then dep

       … from call site

         at /home/artturin/nixgits/my-nixpkgs/pkgs/stdenv/generic/make-derivation.nix:200:8:

          199|   checkDependencyList' = positions: name: deps: lib.flip lib.imap1 deps (index: dep:
          200|     if lib.isDerivation dep || isNull dep || builtins.typeOf dep == "string" || builtins.typeOf dep == "path" then dep
             |        ^
          201|     else if lib.isList dep then checkDependencyList' ([index] ++ positions) name dep

       … while calling 'isDerivation'

         at /home/artturin/nixgits/my-nixpkgs/lib/attrsets.nix:571:5:

          570|     # Value to check.
          571|     value: value.type or null == "derivation";
             |     ^
          572|

       … from call site

         at /home/artturin/nixgits/my-nixpkgs/pkgs/development/libraries/gobject-introspection/default.nix:84:6:

           83|   buildInputs = [
           84|     (python3.withPackages pythonModules)
             |      ^
           85|   ];

       … while calling anonymous lambda

         at /home/artturin/nixgits/my-nixpkgs/pkgs/development/interpreters/python/with-packages.nix:3:1:

            2|
            3| f: let packages = f pythonPackages; in buildEnv.override { extraLibs = packages; }
             | ^
            4|

       … from call site

         at /home/artturin/nixgits/my-nixpkgs/lib/trivial.nix:430:7:

          429|     { # TODO: Should we add call-time "type" checking like built in?
          430|       __functor = self: f;
             |       ^
          431|       __functionArgs = args;

       … while calling anonymous lambda

         at /home/artturin/nixgits/my-nixpkgs/lib/customisation.nix:88:32:

           87|       # Re-call the function but with different arguments
           88|       overrideArgs = copyArgs (newArgs: makeOverridable f (overrideWith newArgs));
             |                                ^
           89|       # Change the result of the function call by applying g to it

       … from call site

         at /home/artturin/nixgits/my-nixpkgs/lib/customisation.nix:88:41:

           87|       # Re-call the function but with different arguments
           88|       overrideArgs = copyArgs (newArgs: makeOverridable f (overrideWith newArgs));
             |                                         ^
           89|       # Change the result of the function call by applying g to it

       … while calling 'makeOverridable'

         at /home/artturin/nixgits/my-nixpkgs/lib/customisation.nix:78:24:

           77|   */
           78|   makeOverridable = f: origArgs:
             |                        ^
           79|     let

       … from call site

         at /home/artturin/nixgits/my-nixpkgs/lib/trivial.nix:430:7:

          429|     { # TODO: Should we add call-time "type" checking like built in?
          430|       __functor = self: f;
             |       ^
          431|       __functionArgs = args;

       … while calling anonymous lambda

         at /home/artturin/nixgits/my-nixpkgs/lib/customisation.nix:90:54:

           89|       # Change the result of the function call by applying g to it
           90|       overrideResult = g: makeOverridable (copyArgs (args: g (f args))) origArgs;
             |                                                      ^
           91|

       … from call site

         at /home/artturin/nixgits/my-nixpkgs/lib/customisation.nix:90:60:

           89|       # Change the result of the function call by applying g to it
           90|       overrideResult = g: makeOverridable (copyArgs (args: g (f args))) origArgs;
             |                                                            ^
           91|

       … while calling anonymous lambda

         at /home/artturin/nixgits/my-nixpkgs/lib/customisation.nix:111:29:

          110|           overrideAttrs = fdrv:
          111|             overrideResult (x: x.overrideAttrs fdrv);
             |                             ^
          112|         }

       error: function 'anonymous lambda' called with unexpected argument 'extraLibs'

       at /home/artturin/nixgits/my-nixpkgs/pkgs/build-support/buildenv/default.nix:8:2:

            7| lib.makeOverridable
            8| ({ name
             |  ^
            9|

@Artturin Artturin marked this pull request as draft January 24, 2023 13:22
Artturin added a commit to Artturin/nixpkgs that referenced this pull request Jan 28, 2023
@github-actions github-actions bot removed 6.topic: python 6.topic: stdenv Standard environment labels Mar 1, 2023
@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict labels Mar 19, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: cross-compilation Building packages on a different sort platform than than they will be run on 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants