all-packages: Apply packageOverrides package-by-package

[Mathnerd314]

This commit rewrites the package recursion so that, rather than a package expression seeing all of the un-overriden packages, it sees all of the overriden packages except for the package that is currently being evaluated.

It also implements recursive merging, but this isn't useful yet because the meta-package has to use 'with pkgs.x' instead of rec.

Also note that this is not a mass-rebuild (hashes are identical).

[Mathnerd314]

Oh, and this fixes #2817.

[lucabrunox]

So what if you want to override 2 packages a and b, and one also depends on the other? Does a see the new b or the old b?

[lucabrunox]

Also the manual must be updated. Have to study this change in the next days, cc @edolstra in the while.

[Mathnerd314]

a sees the new b. Here's the fragment I used to test mergeFuncs:

let
  x = m: with m; { a = false; b = ! a; d = ! c ; e = (b == d) != e; f = e != g; qx = with m.qx; { a = true; b = ! a; }; };
  overrider = n: with n; { b = ! b; c = a; e = ! b; g = a; qx = with n.qx; { b = ! b; c = b; }; };
in mergeFuncs x overrider
# output: { a = false; b = false; c = false; d = true; e = true; f = true; g = false; qx = { a = true; b = true; c = true; }; }

[nbp]

This commit rewrites the package recursion so that, rather than a package expression seeing all of the un-overriden packages, it sees all of the overriden packages except for the package that is currently being evaluated.

Why do we have to do that? I would expect that we could manage to avoid recursive dependencies manually without having to rely on the package dependencies?

From what I understand, this means that one can override one package at a time, but not multiple one. Basically this sounds like an old VCS issues (CVS) where revisions were per-file and not per-set of changes.

I would prefer if we could avoid automation of work-around, when there is an issue in terms of definition. Being able to define a set of packages extending the previous one (extending) is as useful as being able to replace a set of packages as a replacement (overriding) of the previous one (bootstraping).

[Mathnerd314]

@nbp Updated, this uses an approach similar to haskellPackages.

This sets of functions / value is very hard to read.

It should be clearer now that magicMerge is its own function, please tell me if there is still an issue.

Why do we have to do that? I would expect that we could manage to avoid recursive dependencies manually without having to rely on the package dependencies?

We don't "have" to, it's just that this provides more convenient behavior. With the override-Packages approach, you have to write out self and super every time. magicMerge prescribes a particular usage pattern, foo = some-combination-of self.bar self.baz super.foo, which seems to be used in all the overriding code I could find, and provides syntax sugar, foo = with pkgs; some-combination-of bar baz foo

Being able to define a set of packages extending the previous one (extending) is as useful as being able to replace a set of packages as a replacement (overriding) of the previous one (bootstraping).

overridePackages can chain (pkgs.overridePackages.overridePackages), and magicMerge can extend (Defining new attributes/packages). I'm not sure what else is needed.

What is the overhead in terms of time of evaluation to compute the derivation of a NixOS image?

$ `which time` -v <command>

        Command being timed: "bash -c ~/nix/inst/bin/nix-instantiate --readonly-mode ~/nixpkgs-clean -k --show-trace &> /dev/null"
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:24.82
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:24.92
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:24.96
        Maximum resident set size (kbytes): 1523568
        Maximum resident set size (kbytes): 1523940
        Maximum resident set size (kbytes): 1524140

        Command being timed: "bash -c ~/nix/inst/bin/nix-instantiate --readonly-mode ~/nixpkgs -k --show-trace &> /dev/null"
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:27.14
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:27.30
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:27.39
        Maximum resident set size (kbytes): 1665804
        Maximum resident set size (kbytes): 1666080
        Maximum resident set size (kbytes): 1666380

        Command being timed: "bash -c ~/nix/inst/bin/nix-instantiate --readonly-mode --eval -E "import ~/nixpkgs-clean {}" -k --show-trace &> /dev/null"
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:00.10
        Maximum resident set size (kbytes): 16520
        Maximum resident set size (kbytes): 16584
        Maximum resident set size (kbytes): 16600

        Command being timed: "bash -c ~/nix/inst/bin/nix-instantiate --readonly-mode --eval -E "import ~/nixpkgs {}" -k --show-trace &> /dev/null"
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:00.10
        Maximum resident set size (kbytes): 16132
        Maximum resident set size (kbytes): 16196
        Maximum resident set size (kbytes): 16312

        Command being timed: "bash -c ~/nix/inst/bin/nix-instantiate --readonly-mode ~/nixpkgs-clean -A firefox -k --show-trace &> /dev/null"
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:00.39
        Maximum resident set size (kbytes): 58520
        Maximum resident set size (kbytes): 58544
        Maximum resident set size (kbytes): 58584

        Command being timed: "bash -c ~/nix/inst/bin/nix-instantiate --readonly-mode ~/nixpkgs -A firefox -k --show-trace &> /dev/null"
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:00.34
        Maximum resident set size (kbytes): 44516
        Maximum resident set size (kbytes): 44588
        Maximum resident set size (kbytes): 44656

        Command being timed: "bash -c ~/nix/inst/bin/nix-instantiate --readonly-mode -k ~/nixpkgs-clean/nixos &> /dev/null"
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:13.68
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:13.68
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:13.76
        Maximum resident set size (kbytes): 1031272
        Maximum resident set size (kbytes): 1031532
        Maximum resident set size (kbytes): 1031768

        Command being timed: "bash -c ~/nix/inst/bin/nix-instantiate --readonly-mode -k ~/nixpkgs/nixos &> /dev/null"
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:12.92
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:12.97
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:12.98
        Maximum resident set size (kbytes): 905148
        Maximum resident set size (kbytes): 905276
        Maximum resident set size (kbytes): 905980

So it seems to be faster/ lower memory in all cases except for fully evaluating the full nixpkgs set.

[nbp]

Based on the existing comment, I think the packageOverrides function should rely on super instead of self. If you can separate the merge function in a separate commit, I think this would be a net improvement which would not be blocked by the changed behaviour of packageOverrides.

Also, even if I think this merging function makes sense, I do think that it might be terrible in terms of memory consumption, as we would have a new version of the pkgs attribute set for each overridden package, while we only care about the intersection of the package arguments with the new set of packages.

[Mathnerd314]

I've made a new version, without magicMerge; it has no rebuilds and is faster/less memory in all cases AFAIK.

[Ericson2314]

Big 👍

[copumpkin]

For anyone not paying attention, @edolstra seems to have temporarily reverted this for breaking in some way. @edolstra, if you point out what's wrong, perhaps @Mathnerd314 can fix it and we can try again?

[edolstra]

I reverted it because I haven't had time to review it yet, and this looks like it might have a big impact so I feel I need to give it proper attention. Sorry about that...

[Mathnerd314]

Well, it doesn't do anything to the builds, those are unchanged. I guess it might have done something to the evaluator; there seem to be a lot of aborted and queued builds. I can't say I'm surprised though, reverts seem to be the Nix development style.

[Ericson2314]

This is unfortunate.

[domenkozar]

@Mathnerd314 could you reopen the PR?

[Mathnerd314]

I can't AFAICT. I could file a new PR though.

[Ericson2314]

Better do new PR then?

[Mathnerd314]

I'll open it eventually. It's on my calendar at June 24, 2016.

[nbp]

@edolstra , when would you be able to review this change?

I would appreciate if we review and fix what is preventing it to be merge, as this is a blocker for the rest of the security update issue.

[nbp]

For anyone not paying attention, @edolstra seems to have temporarily reverted this […]

This change got reverted in commit 75f361f

[Ericson2314]

Can close now that #14000 is merged?

[Mathnerd314]

It's already closed.

[Ericson2314]

Oh, github race condition or my bad :).