-
-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
linux-headers: Improve derivation, removing cross arg #28519
linux-headers: Improve derivation, removing cross arg #28519
Conversation
@@ -12666,7 +12664,6 @@ with pkgs; | |||
uclibc = callPackage ../os-specific/linux/uclibc { }; | |||
|
|||
uclibcCross = lowPrio (callPackage ../os-specific/linux/uclibc { | |||
inherit (buildPackages) linuxHeaders; |
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.
Now that its built in the same stage.
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.
Looks good, I didn't test it.
in | ||
|
||
stdenv.mkDerivation { | ||
stdenvNoCC.mkDerivation { |
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.
Um, why would this use stdenvNoCC
given that the build needs to compile some stuff?
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.
Stdenv.cc is a build->host cc, but I need a build->build cc
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.
Such thing also belongs to the standard build environment even when cross compiling (e.g Buildroot and Yocto do). If nixpkgs doesn't anymore then that needs fixing instead.
if stdenv.isArm then "arm" else | ||
if stdenv.platform ? kernelArch then stdenv.platform.kernelArch else | ||
abort "don't know what the kernel include directory is called for this platform"; | ||
platform = hostPlatform.platform.kernelArch or ( |
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 think this fallback code (when kernelArch
is not defined) should really go away.
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 tried not to change any interfaces. Mind doing this in a follow up commit?
Note that for native kernelArch
isn't currently defined by default. There is hostPlatform.parsed.cpu.name
however.
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.
kernelArch
is always defined because the kernel derivation uses it unconditionally. And it must not be something else than kernelArch
because the name we need to pass here is not something standard but an implementation detail of the Linux kernel.
- Perl is used at build time, so must be in `nativeBuildInputs`. It's not used at run time so it should not be in `buildInputs`, too. - Don't treat headers like a compiler---use the build and host platforms not host and target. Perhaps that would make sense if every library's headers could be a separate derivation, but since that is not feasible, best to keep the implementation and interface in the same stage. To do this, we used `stdenvNoCC` to get rid of the normal toolchain, and added a dependency for the toolchain targeting the build platform --- `buildPackages.stdenv.cc` --- thus everything is effectively slid a stage black.
0d420ca
to
791ce59
Compare
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.
With that note, I think I'll merge for the same reasons of unblocking everything else I want to do for 17.09.
@dezgeg We can move the kernelArch
fallback code to https://github.com/NixOS/nixpkgs/blob/master/lib/systems/default.nix#L17 if you like.
|
||
buildInputs = [perl]; | ||
# It may look odd that we use `stdenvNoCC`, and yet explicit depend on a cc. |
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.
@dezgeg Added this explanatory comment, as it does indeed look weird at first glance.
In NixOS#28519 / 791ce59 I made linux headers be intended to be used from the stage stage, as it would be if it were a library containing headers and code. I forgot to update glibc, however, so it was incorrectly using headers for the build platform, not host platform. This fixes that, basically reverting a small portion of changes I made a few months ago in 25edc47 and its parent. No native hashes are changed.
Motivation for this change
Perl is used at build time, so must be in
nativeBuildInputs
. It's not used at run time so it should not be inbuildInputs
, too.Don't treat headers like a compiler---use the build and host platforms not host and target. Perhaps that would make sense if every library's headers could be a separate derivation, but since that is not feasible, best to keep the implementation and interface in the same stage.
To do this, we used
stdenvNoCC
to get rid of the normal toolchain, and added a dependency for the toolchain targeting the build platform ---buildPackages.stdenv.cc
--- thus everything is effectively slid a stage black.Things done
(nix.useSandbox on NixOS,
or option
build-use-sandbox
innix.conf
on non-NixOS)
nix-shell -p nox --run "nox-review wip"
./result/bin/
)