-
-
Notifications
You must be signed in to change notification settings - Fork 14.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
luajit: fix cross build for 32 bit architectures #212760
Conversation
buildStdenv = if buildPackages.stdenv.isx86_64 && stdenv.is32bit | ||
then buildPackages.pkgsi686Linux.buildPackages.stdenv | ||
else buildPackages.stdenv; |
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.
While this looks good, I wonder if there is a more universal solution that works on other crosses.
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.
Honestly, I consider the luajit approach of requiring the bitness of the build host to match the build system pretty stupid. It is also the only package I know about that requires it (..although I have not looked for it. It is just from my practice with maintaining packages for OpenWrt.).
I am also afraid that this can't be easily achieved on other platforms. Only some CPUs have the ability to run 32-bit code alongside 64-bit. It is standard on x86_64 but not necessary on ARM or RISC-V (I think that one of the examples is Apple's chips).
I should have added a comment there that it is a speciality of luajit, not some common approach.
8de2ea2
to
5151356
Compare
@@ -88,8 +94,7 @@ stdenv.mkDerivation rec { | |||
"PREFIX=$(out)" | |||
"DEFAULT_CC=cc" | |||
"CROSS=${stdenv.cc.targetPrefix}" | |||
# TODO: when pointer size differs, we would need e.g. -m32 |
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.
The note should be preserved, this special casing only works x86_64.
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.
Except that this comment is almost only just for x86 because that is not some universal option. The only other applicable (also directly supported by nixpkgs) I found is PowerPC.
Nixpkgs provides, in general, only a single target compilers. That is why gcc_multi
exists alongside gcc
. That gcc_multi
currently also hard depends on i686 and doesn't work on other platforms.
$ nix shell nixpkgs#pkgsCross.aarch64-multiplatform.stdenv.cc -c aarch64-unknown-linux-gnu-gcc -E -march=help -xc /dev/null
...
cc1: note: valid arguments are: armv8-a armv8.1-a armv8.2-a armv8.3-a armv8.4-a armv8.5-a
$ nix shell nixpkgs#pkgsCross.aarch64-multiplatform.stdenv_32bit.cc
error: Multilib aarch64-unknown-linux-gnu-stage-final-gcc-wrapper-9.5.0 not supported for ‘aarch64-linux’
(use '--show-trace' to show detailed location information)
I am going to add TODO
at the other place instead of this one where support for other platforms should be required. The issue is that it won't be that easy for ARM for example because not every aarch64 supports for example armv7l.
The best practice here would be to use stdenv_32bit
but that is broken as I described in #212494. I will suggest that in the TODO
.
The 32bit compiler has to be used when building on 64bit system.
5151356
to
af28395
Compare
Description of changes
The 32bit compiler has to be used when building on 64bit system.
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)nixos/doc/manual/md-to-db.sh
to update generated release notesThe implementation is tied to my journey here #212494. The magic is in the usage of
pkgsi686Linux.buildPackages
instead of just plainpkgsi686Linux
.