-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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
pkgs/top-level: make package sets composable #303849
base: master
Are you sure you want to change the base?
Conversation
Eval is broken :( |
That said, this is generally good stuff and we should land this (though maybe after 24.05?) |
I don't have time to review this right now but I just want to say, thank you so much for writing this, and your commit messages are great! |
I haven't been able to find out why, yet :/ |
Will change to "draft" until I have the time to figure that out. |
10fb3c4
to
1039e99
Compare
I rebased on latest master and should have fixed eval along the way. |
1039e99
to
fc5dab0
Compare
Improved the commit messages, fixed another eval failure and added another commit to do the following, as mentioned above:
Let's see where ofborg fails this time. |
fc5dab0
to
8ce3fca
Compare
Rebased and cherry-picked #314845. This makes |
eval passes now. Ready for review. Please ignore the first commit (see other comment). |
Thank for doing a very thorough job documenting your work, @wolfgangwalther, but I am a little worried about passing this information. it was originally very intentional that I transformed the ugly/bad I think I see why you are doing this, but another way to look at this is that having the other package sets mashed into |
@Ericson2314 I'm not sure what the takeaway from this is. Are you saying that package sets should not be composed? I.e. we should only have (example) Clearly the status-quo, where it's possible to call them, but they only compose in parts, but not in others, is a problem. |
Overall, I can find 5 different approaches how to do deal with those package sets:
Obviously, I'm in favor of the last option. Once we agree on where this should go, we can look into how to make this nicer architecturally. |
8ce3fca
to
8664455
Compare
I have changed the approach from "pass localSystem and crossSystem to stage.nix" to "make nixpkgsFun itself composable". So "dealing with the bad/ugly localSystem/crossSystem" only happens in default.nix now - stage.nix is a lot cleaner than before. @Ericson2314 Do you like this approach better? |
b9dd0ed
to
bfe1dd9
Compare
@Ericson2314 could you look at this once more? |
bfe1dd9
to
fdc175a
Compare
eval is broken after the latest rebase, will mark as draft until I can fix this. |
fdc175a
to
e6cd302
Compare
e6cd302
to
4ab3610
Compare
pkgs/test/top-level/default.nix
Outdated
# TODO: unexpected argument 'bintools' | ||
# assert isComposable "pkgsZig"; |
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.
I rebased this over the addition of pkgsArocc
and pkgsZig
and found that currently pkgsZig.pkgsExtraHardening.stdenv
is broken:
error: function 'anonymous lambda' called with unexpected argument 'bintools'
at pkgs/development/compilers/zig/cc.nix:1:1:
@RossComputerGuy is this combination something that even makes sense? I wasn't able to figure out quickly how to fix it.
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.
I believe #331011 might fix it.
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.
I believe #331011 might fix it.
Correct - it does!
After rebasing again, eval passes. Ready for review, again. |
This removes all specific references to pkgsCross or pkgsi686Linux, because they have become outdated with the addition of many more package sets.
This can easily be forgotten to append in stage.nix. Also the comment in default.nix explicitly states that callers of nixpkgsFun should not need to "know more than [they] need to". While the overlays argument is already present for different reasons, the fact "you need to append overlays" is something that callers should not need to know. This is written in a rather verbose form, but this allows to easily extend this in follow up commits.
Just a bit less repetition, making it easier to see the differences between package sets on first glance.
…e top No change, just move appendOverlays and extend to the top. This makes it easier to compare the other package sets side-by-side.
…arsed args When elaborating a system with both "config" and "system" arguments given, they might not match the parsed results. Example: elaborate { config = "i686-unknown-linux-gnu"; system = "x86_64-linux"; } This would result in a parsed system for i686, because the config argument is preferred. But since // args // comes after system has been inferred from parsed, it is overwritten again. This results in config and parsed all pointing to i686, while system still tells the story of x86_64. Inconsistent arguments can also be given when passing "parsed" directly. This happened in stage.nix for the various package sets. The solution is simple: One of the three arguments needs to be treated as the ultimate source of truth. "system" can already be losslessly extracted from "parsed". However, "config" currently can not, for example for various -mingw32 cases. Thus everything must be derived from "config". To do so, "system" and "parsed" arguments are made non-overrideable for systems.elaborate. This means, that "system" will be used to parse when "config" is not given - and "parsed" will be ignored entirely. The asAttrs helper is exposed on lib.systems, because follow-up commits make use of it.
This adds some basic tests to compose package sets. The cases that are currently broken are commented out, they include things like: - pkgsStatic.pkgsMusl losing the isStatic flag - pkgsCross.ppc64-musl.pkgsMusl losing the gcc.abi setting - pkgsCross.mingwW64.pkgsStatic losing the config string - pkgsLLVM.pkgsMusl losing the useLLVM flag - pkgsLLVM.pkgsStatic losing the useLLVM flag - pkgsLLVM.pkgsi686Linux losing the useLLVM flag And probably more.
This makes combinations work, where pkgsStatic is used on another package set, for example: - Keeps pkgsCross.mingwW64.pkgsStatic.stdenv.hostPlatform.config at "x86_64-w64-mingw32" - Keeps pkgsLLVM.pkgsStatic.stdenv.hostPlatform.useLLVM at "true" Makes pkgsCross.mingwW64.pkgsStatic.stdenv build. Not sure whether it actually builds static executables already, though. Resolves NixOS#281596
This makes combinations work, where pkgsi686Linux or pkgsMusl are used on another package set, for example: - Keeps pkgsStatic.pkgsMusl.stdenv.hostPlatform.isStatic at "true" - Keeps pkgsCross.ppc64-musl.pkgsMusl.stdenv.hostPlatform.gcc.abi at "elfv2" - Keeps pkgsLLVM.pkgsi686Linux.stdenv.hostPlatform.useLLVM at "true" Supersedes NixOS#136549 Resolves NixOS#114510
The previous logic to decide whether to "go musl" or "go i686" or not was based on comparing stdenv.buildPlatform and stdenv.hostPlatform. Those are the same for something like pkgsCross.gnu64 on linux x64. This then leads to the following: > pkgsCross.gnu64.pkgsMusl.stdenv.buildPlatform.config "x86_64-unknown-linux-musl" > pkgsCross.gnu64.pkgsMusl.stdenv.hostPlatform.config "x86_65-unknown-linux-gnu" This means the addition of pkgsMusl changed the *local system* and not the *cross system* - which is surely not what is intended here. Instead the cross system should be changed to musl / i686. A better way to detect this is to check whether a crossSystem had already explicitly been passed to nixpkgsFun. This also makes the following work as expected: pkgsCross.aarch64-darwin.pkgsx86_64Darwin pkgsCross.mingwW64.pkgsLinux Both now change the cross system instead of the local system. Resolves NixOS#212494
Trying to evaluate pkgsXYZ.pkgsCross is conceptually unsound: pkgsCross expects to reset crossSystem from scratch, because the whole point about this layer is to start with "well known" example configurations. It's fine to make modifications on top of that, though. Thus, nested pkgsCross is now forbidden and will throw an evaluation error.
4ab3610
to
eb5a92b
Compare
Description of changes
The various
pkgsXYZ
top-level package sets did not passlocalSystem
/crossSystem
options to lower levels, so far. This change propagates the original arguments, i.e.localSystem0
/crossSystem0
to lower levels. Those include the overrides made by an upper package set.Example:
pkgsStatic
addscrossSystem = { isStatic = true }
.pkgsStatic.pkgsMusl
previously lost this setting, because it didn't propagatecrossSystem0
, yet.The first commit removes / rewrites some outdated comments, which referred to the cross and i686 package sets. Meanwhile, we have a lot more, so I rewrote those comments to not say anything about specific sets anymore.
The second commit adds some tests to confirm composability of package sets. This helped me coming up with the fixes, I'm not sure whether it should be kept. It takes about 8-9 seconds for me to run. It also helps showing what the remaining commits fix by uncommenting the previously broken tests.
The remaining commits fix those composability issues. This:
One open question for me is how to deal with
pkgsXYZ.pkgsCross
. This has practical challenges (#114510), but is also conceptually unsound: In a waypkgsCross
expects to resetcrossSystem
from scratch, because the whole point about this layer is to start with "well known" example configurations. IMHO, it's fine to make modifications on top of that. e.g.pkgsCross.XYZ.pkgsLLVM
orpkgsCross.XYZ.pkgsStatic
, but it makes little sense to havepkgsLLVM.pkgsCross...
orpkgsStatic.pkgsCross...
. Probably completely unusable ispkgsCross.XYZ.pkgsCross.ABC
.One idea would be to just restrict
pkgsCross
to be used at the top-level, for example by adding something like this in each package set, includingpkgsCross
itself:That would also disallow
pkgsMusl.pkgsCross.XYZ
, which I think is about the only remotely reasonable use-case here: "cross compiling from a native musl environment". I'm not sure whether anyone uses that, though, and whether it's worth supporting at all.Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.