Use fix and extend function for all-packages.nix by nbp · Pull Request #14000 · NixOS/nixpkgs

nbp · 2016-03-17T21:35:29Z

This pull request aims at making tiny incremental changes to all-packages.nix in order to progressively converge to what got achieved by @Mathnerd314 in #9400.

The goal of this pull request is to change the layout of the code without changing the data flow. If this predicate is not honored, the patch includes a big comment describing how this is identical / different.

Note for reviewers:

Review one patch at a time.
- The entire set of changes might be hard to digest.
Always leave a comment and a reaction (:+1:, :-1:, :confused:, …).
- If you approve, disapprove, need clarification, or when you would be able to address this review.

cc @edolstra, @shlevy and @Mathnerd314

shlevy · 2016-03-18T18:52:36Z

pkgs/top-level/all-packages.nix

Why this blank space?

Because the lines above are the end of a function, while the 2 lines below are the end of an enclosing let-in which goes through the whole file.

As soon as all packages are moved in another file than the fix-point logic of nixpkgs, I will re-indent these lines properly. In the mean time this extra line is a way to split the 2.

shlevy · 2016-03-18T18:56:30Z

Overall seems fine for me.

nbp · 2016-03-19T13:00:53Z

Note, in a similar way as #9400, this is blocking #10851 , as we need to be able to express nixpkgs as a single function to handle security issues properly.

zimbatm · 2016-03-19T23:57:31Z

Just reviewed all the changes. In the end it's not a lot of lines that move but it was helpful to split it in different commits like that for the review.

The `helperFunctions` and `stdenvAdapters` both use the `pkgs` attribute as input, either to inherit some properties, either to use it as argument. The `pkgs` binding used in both expressions of the `helperFunctions` and `stdenvAdapters` is no longer the result of the `applyGlobalOverrides` function, but the argument of the `pkgsFun` function. The `pkgsFun` functions is called twice under `applyGlobalOverrides`, and in both cases, the first argument of `pkgsFun` correspond to the result of `applyGlobalOverrides`. Thus, this modification will change the bindings, but the evaluation of `<nixpkgs>`. A third call the `pkgsFun` exists under `overridePackages` in the set of all packages. Previously, the `helperFunctions` and `stdenvAdapaters` would use the functions defined as part of the default `<nixpkgs>` set. With this modification, the `helperFunctions` and the `stdenvAdapters` are now using the fix-point of the newly evaluated package set. This implies that this modification allow the user to use `overridePackages`, which is already not recommended for performance reasons, to override the inputs of the `helperFucntions` and `stdenvAdapaters` too, where this was not possible before.

…tion.

…etter.

This modification change the names bound to the `helperFunctions` attribute set, to be bound to `self` which is constructed by merging the same `helperFunctions` set with the set of all packages. This patch works as expected because none of the helperFunction names is aliased by the name of a package.

Note, the aliases are now computed against the set of packages defined in the set of all packages, and no longer apply to any overriden package. I think this is better as this reduces the amount of surprizes.

…aluating stdenvCross. While evaluating the derivation of xbursttools: the condition `pkgs.stdenv ? overrides` causes the evaluation of `stdenvCross`. This evaluation comes too early during the execution, as it prevents the resolution of names such as `pkgs.lib`, and `stdenvAdapaters.makeStdenvCross`, which we want to take from `pkgs` instead of `self` in following patches. By swapping the conditions, we effectively make the resolution of `pkgs.lib` and `stdenvAdapaters.makeStdenvCross` possible through the pkgs attribute.

Extract stdenvDefault from the set of all packages. As this set of attributes are inter-dependant, probably due to stdenvOverrides, we have to keep them in a close set of inter-dependent options. I guess I will have to investigate more ...

…elf.

…alue of pkgsFun.

zimbatm · 2016-03-20T21:15:06Z

pkgs/top-level/all-packages.nix

+  topLevelArguments = {
+    inherit system bootStdenv noSysDirs gccWithCC gccWithProfiling config
+      crossSystem platform lib;
+  };


Aliasing these arguments seem a bit superfluous if they can be passed to import ./stdenv {} directly. A later change might invalidate my thinking.

Later it's used on two places.

zimbatm · 2016-03-20T21:38:27Z

Finished reviewing all the changes. Once we agree on the changes most of this should be squashed into bigger commits don't you think ?

Overall +1 for separating the all-packages in more logical chunks and making everything more flexible. I would like some more testing to make sure that cross-compilation, and ~/.nixpkgs/config.nix. I also don't know what performance impact these changes are going to be. If that's the case then we should split the changes into two so that at least the first part can go in.

nbp · 2016-03-20T21:44:00Z

Once we agree on the changes most of this should be squashed into bigger commits don't you think ?

I do not think there is much value in squashing the commits, as once merged we would have a single commit in nixpkgs.

This would also be bad if we ever want to bisect through the changes, since this might help us investigate the issue better.

nbp · 2016-03-20T21:54:31Z

I would like some more testing to make sure that cross-compilation

In fact, when running nox-review, we are already generating a package which uses a cross compiler, and do that by setting crossSystem. So this is actually tested by nox-review.

and ~/.nixpkgs/config.nix

I already have a non-empty config.nix, so I will double check that these are getting the same sha.

For the override of stdenv packages, I honestly do not see much value, except adding debug symbols to them and I would be fine to restore the old behaviour back, if requested.

aszlig · 2016-03-20T22:38:08Z

pkgs/top-level/default.nix

Spelling: overridden

zimbatm · 2016-03-21T15:47:27Z

When comparing time nix-env -f. -qaP > /dev/null this branch is a tiny bit faster that the commit it's based over. Over multiple runs I get approximately:

Before:

real    0m2.094s
user    0m1.908s
sys 0m0.185s

After:

real    0m2.070s
user    0m1.915s
sys 0m0.153s

Ericson2314 · 2016-03-21T21:29:25Z

I've been a major fan of these changes through their many iterations, but the way this is split up is just beautiful. I will recommend this PR as a followup to anyone wishing to learn more about how nixpkgs is structured after reading the nix pills.

nbp · 2016-03-21T22:57:01Z

Apparently the split of stdenv seems to have cause issues with stdenv_32bits, when used in overriden packages, as well as the split of stdenvOverrides and customOverrides. I am investigating the drv files differences to see from where this issue comes from. (wish nix-store would output diff-able results)

nbp · 2016-03-22T01:58:38Z

Ok, I will check that later, but my guess is that when I separated the stdenv to use with pkgs instead of with self (commit 020bb40), I left over a few bindings which are unfortunately redefined by stdenvOverrides.

Thus my guess is that the gcc variable of gcc_multi[1] is now bound to the stage4 [2], instead of the all-packages one [3].

[1] https://github.com/nbp/nixpkgs-2/blob/fix-extend/pkgs/top-level/all-packages.nix#L3976
[2] https://github.com/NixOS/nixpkgs/blob/master/pkgs/stdenv/linux/default.nix#L304
[3] https://github.com/nbp/nixpkgs-2/blob/fix-extend/pkgs/top-level/all-packages.nix#L4087

nbp · 2016-03-23T22:06:26Z

Ok, I cannot explain from where the difference of evaluation is coming from, but now I tempted to not fix it at all.

The reason why I will not fix it is because now, if we write the same expression in config.nix, as we were to write it in all-packages.nix, then we would get the same hashes. Which was not the case before.

I think this is a non-neglectable addition for landing this fix-extend approach.
I am just sad I cannot explain why the old version is using fetchurlBoot sooner than expected.

zimbatm · 2016-03-24T10:36:13Z

It's not clear to me what the issue is. Does it cause an error or just changes the derivation hash ? What do you do to reproduce the issue ?

peti · 2016-03-24T11:05:44Z

@nbp, thank you very much for the effort you've put into this clean-up and for submitting the change as a series of easily reviewable patches. I'd would be great to get those changes merged into master ASAP. Even if this re-factoring does have unintended consequences that we find out only after merging it, then I'm confident that we can find a way to fix them, so IMHO the risk of merging this update is rather small.

nbp · 2016-03-24T19:51:24Z

Thanks @zimbatm , @aszlig and @vcunat for reporting the typos while reviewing the commits :)

@zimbatm

It's not clear to me what the issue is. Does it cause an error or just changes the derivation hash ? What do you do to reproduce the issue ?

I tested to clone rr expression in my config.nix file, and renaming it rr4 to make sure I do not get the old one, and checked the hash of the derivation with nix-instantiate ./ -A rr4. I was able to evaluate both fine, but got different hashes.

Then I compared the hash with the hash of the derivation of nix-instantiate ./ -A rr, before and after this patch series. I got a different result for the base branch, and an identical result of this branch.

This is why I mentioned that I am not going to fix this issue, as I get the same behaviour in config.nix as-if the package was written in all-packages.nix.

base-branch$ nix-instantiate ./ -A rr4
/nix/store/gbg1jpm6nb8z2kcgjm0qw8ysy56z0a6j-rr-4.2.0.drv
base-branch$ nix-instantiate ./ -A rr
/nix/store/y7xqd9i08f09nhqz8ypfhv4jcy6dr1ym-rr-4.2.0.drv
fix-extend$ nix-instantiate ./ -A rr4
/nix/store/y7xqd9i08f09nhqz8ypfhv4jcy6dr1ym-rr-4.2.0.drv
fix-extend$ nix-instantiate ./ -A rr
/nix/store/y7xqd9i08f09nhqz8ypfhv4jcy6dr1ym-rr-4.2.0.drv

vcunat · 2016-03-25T16:38:40Z

Thanks a lot for cleaning the override mess and making the transition reviewable! (I finished reviewing this a bit late.) In some commits I didn't really see the motivation why the new state should be preferable to the old one, but on the whole it seems much easier to understand now.

Ericson2314 · 2016-03-25T18:14:49Z

pkgs/top-level/all-packages.nix

-
-
-let config_ = config; platform_ = platform; in # rename the function arguments
+with pkgs;


Shouldn't this be self? pkgs = super, right? I am reading 295b7a6 and getting even more confused :).

While you've argued well that aliases should refer to the unoverriden packages, dependencies should definitely be overridden.

In the final result, self is equivalent to rec, and pkgs is equivalent to the self of the fix-point. super would be all the stdenvAdapaters and the trivialBuilders.

Ah, I missed https://github.com/NixOS/nixpkgs/blob/master/pkgs/top-level/default.nix#L119

grep --color -E 'self( |$|\.)' pkgs/top-level/all-packages.nix Convinces me we could remove the self arg altogether from all-packages and put everything in aliases.

nbp · 2016-03-25T19:35:12Z

@vcunat

so this should finally remove the distinction between using foo and pkgs.foo in the inside of the file :-)

Mainly, yes, but with the security branch which is coming we should default to pkgs.foo for dependencies, and default to self.foo for statically linked packages, or for overridden packages.

Maybe it would be useful to have a variant of the // operator that would throw an error on collision instead of overriding...

I guess we could add a check for that as part of the aliases.nix file.

nbp added 0.kind: enhancement Add something new or improve an existing system. 8.has: clean-up This PR removes packages or removes other cruft labels Mar 17, 2016

nbp self-assigned this Mar 17, 2016

nbp added this to the 16.09 milestone Mar 17, 2016

shlevy reviewed Mar 18, 2016
View reviewed changes

nbp mentioned this pull request Mar 18, 2016

Move all aliases into their own file. #14029

Merged

1 task

nbp force-pushed the fix-extend branch from 4c17bb8 to e9e277f Compare March 18, 2016 20:29

nbp added 18 commits March 20, 2016 14:31

Split miss-indented last lines. to simplify future diffs.

9444d6e

Re-indent the top of pkgsFun.

a39c5ff

Move 'with helperFunctions' above the set of all packages.

11a566b

Move pkgsOrig outside applyGlobalOverrides and provide it as argument.

f9a2988

Add an extra argument to the overrider function of applyGlobalOverrides.

a6e260c

Unify the applyGlobalOverride function with the overridePackages func…

a5188bd

…tion.

Extract stdenvOverrides function out of mkOverrides function.

00963c3

Move applyGlobalOverrides comment to stdenvOverrides, where it fits b…

acaa99c

…etter.

Move mkOverrides under applyGlobalOverrides.

71b2fd9

Under applyGlobalOverrides, inline mkOverrides in overrides.

a8374f2

Under pkgsFun, move the merge of helper functions from self_ to self.

ffcb668

Under pkgsFun, expand self attribute, and rename self_ to self.

8d355b8

Note, the aliases are now computed against the set of packages defined in the set of all packages, and no longer apply to any overriden package. I think this is better as this reduces the amount of surprizes.

Split stdenv attribute set.

020bb40

Extract stdenvDefault from the set of all packages. As this set of attributes are inter-dependant, probably due to stdenvOverrides, we have to keep them in a close set of inter-dependent options. I guess I will have to investigate more ...

Replace with statements of stdenvDefault to rely on pkgs instead of s…

be3531a

…elf.

Move merge of stdenvDefault from the self attribute to the returned v…

f7c86ee

…alue of pkgsFun.

nbp force-pushed the fix-extend branch from 30dad92 to f7c86ee Compare March 20, 2016 14:34

Move tweakAlias function into the aliases.nix file, and rename it.

0b67f7c

zimbatm reviewed Mar 20, 2016
View reviewed changes

aszlig reviewed Mar 20, 2016
View reviewed changes

pkgs/top-level/default.nix Outdated

Copy link

Member

aszlig Mar 20, 2016

Choose a reason for hiding this comment

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

Spelling: overridden

zimbatm mentioned this pull request Mar 20, 2016

Mutiple replaceRuntimeDependencies may reintroduce depenendices #4336

Closed

Fix comments typos.

87ad35e

nbp force-pushed the fix-extend branch from d515905 to 87ad35e Compare March 24, 2016 19:41

nbp merged commit 5d6a4a6 into NixOS:master Mar 24, 2016

Ericson2314 mentioned this pull request Mar 25, 2016

all-packages: Apply packageOverrides package-by-package #9400

Merged

Ericson2314 reviewed Mar 25, 2016
View reviewed changes

nbp mentioned this pull request Mar 27, 2016

Shipping Security Updates #10851

Closed

6 tasks

vcunat mentioned this pull request Apr 5, 2016

stdenv_32bit.gcc is broken, breaking e.g. virtualbox #14465

Closed

Ericson2314 mentioned this pull request Apr 27, 2016

top-level: simplify stdenv calling of top-level: fewer inferred and re-inferring arguments #15043

Merged

nbp mentioned this pull request Feb 26, 2017

RFC to describe the RFC process NixOS/rfcs#1

Merged

4 tasks



		let config_ = config; platform_ = platform; in # rename the function arguments
		with pkgs;

Uh oh!

Comments

Conversation

nbp commented Mar 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shlevy commented Mar 18, 2016

Uh oh!

nbp commented Mar 19, 2016

Uh oh!

zimbatm commented Mar 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zimbatm commented Mar 20, 2016

Uh oh!

nbp commented Mar 20, 2016

Uh oh!

nbp commented Mar 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zimbatm commented Mar 21, 2016

Uh oh!

Ericson2314 commented Mar 21, 2016

Uh oh!

nbp commented Mar 21, 2016

Uh oh!

nbp commented Mar 22, 2016

Uh oh!

nbp commented Mar 23, 2016

Uh oh!

zimbatm commented Mar 24, 2016

Uh oh!

peti commented Mar 24, 2016

Uh oh!

nbp commented Mar 24, 2016

Uh oh!

vcunat commented Mar 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nbp commented Mar 25, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants