-
-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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
Use fix and extend function for all-packages.nix #14000
Conversation
in lib.mapAttrs tweakAlias aliases // self; in pkgs | ||
in | ||
lib.mapAttrs tweakAlias aliases // self; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this blank space?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Overall seems fine for me. |
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.
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 ...
topLevelArguments = { | ||
inherit system bootStdenv noSysDirs gccWithCC gccWithProfiling config | ||
crossSystem platform lib; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aliasing these arguments seem a bit superfluous if they can be passed to import ./stdenv {}
directly. A later change might invalidate my thinking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Later it's used on two places.
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 |
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. |
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.
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. |
# mechanism to implement some staged compilation of the stdenv. | ||
# | ||
# We don't want stdenv overrides in the case of cross-building, or | ||
# otherwise the basic overrided packages will not be built with the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling: overridden
When comparing Before:
After:
|
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. |
Apparently the split of stdenv seems to have cause issues with |
Ok, I will check that later, but my guess is that when I separated the stdenv to use Thus my guess is that the [1] https://github.com/nbp/nixpkgs-2/blob/fix-extend/pkgs/top-level/all-packages.nix#L3976 |
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 I think this is a non-neglectable addition for landing this fix-extend approach. |
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 ? |
@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 |
Thanks @zimbatm , @aszlig and @vcunat for reporting the typos while reviewing the commits :)
I tested to clone Then I compared the hash with the hash of the derivation of This is why I mentioned that I am not going to fix this issue, as I get the same behaviour in
|
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. |
|
||
|
||
let config_ = config; platform_ = platform; in # rename the function arguments | ||
with pkgs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Mainly, yes, but with the security branch which is coming we should default to
I guess we could add a check for that as part of the |
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:
cc @edolstra, @shlevy and @Mathnerd314