-
-
Notifications
You must be signed in to change notification settings - Fork 13.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
qemu: fix static build #333923
qemu: fix static build #333923
Conversation
befad0d
to
8115baf
Compare
I fixed the merge conflict and changed the PR to target staging. |
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.
This is cool, but it's pretty hacky as-is. I don't think it needs to be hacky, though — shouldn't be too difficult to clean up.
@@ -127,6 +129,9 @@ stdenv.mkDerivation (finalAttrs: { | |||
++ lib.optionals capstoneSupport [ capstone ] | |||
++ lib.optionals (!userOnly) [ dtc ]; | |||
|
|||
# a private dependency of PAM which is not linked explicitly in static builds | |||
env.NIX_LDFLAGS = lib.optionalString (stdenv.hostPlatform.isStatic && stdenv.isLinux) " -laudit "; |
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.
That sounds like a pkg-config problem we should fix in pam (or in QEMU's meson.build if it's not using pkg-config).
|
||
# qemu defined a crc32c function which clashes with one in libblkid when | ||
# building statically | ||
./static_build_crc32c_duplicate_definition.patch |
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.
Do we really need whatever is enabling this? I'd rather not add another downstream patch unless there's a very clear need.
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.
Alternatively: can it be upstreamed?
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.
At the very least, this patch should be a lib.lists.optional
conditional that checks for the static build. There's no need to enable this on every QEMU build.
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.
Do we really need whatever is enabling this? I'd rather not add another downstream patch unless there's a very clear need.
what do you mean ?
At the very least, this patch should be a lib.lists.optional conditional that checks for the static build. There's no need to enable this on every QEMU build.
to the contrary patches should be eagerly applied otherwise they bitrot.
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.
Do we really need whatever is enabling this? I'd rather not add another downstream patch unless there's a very clear need.
what do you mean ?
I mean, given we can build a subset of static qemu without this (this is now used for binfmt), then whatever feature requires this must be optional. So is it one that we need, or can we just disable it for static builds and avoid the patch?
@@ -174,6 +183,8 @@ stdenv.mkDerivation (finalAttrs: { | |||
"--cross-prefix=${stdenv.cc.targetPrefix}" | |||
(lib.enableFeature guestAgentSupport "guest-agent") | |||
] ++ lib.optional numaSupport "--enable-numa" | |||
# plugins are necessarily shared libraries | |||
++ lib.optional stdenv.hostPlatform.isStatic "--disable-plugins" |
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.
Can you please use enableFeature
for newly added options?
@@ -195,8 +206,7 @@ stdenv.mkDerivation (finalAttrs: { | |||
++ lib.optional capstoneSupport "--enable-capstone" | |||
++ lib.optional (!pluginsSupport) "--disable-plugins" | |||
++ lib.optional (!enableBlobs) "--disable-install-blobs" | |||
++ lib.optional userOnly "--disable-system" | |||
++ lib.optional stdenv.hostPlatform.isStatic "--static"; |
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?
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.
otherwise meson cannot find libaio
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 not?
I suspect that this might be the reason for the pam thing not being propagated — if Meson isn't told to do a static build, it won't ask pkg-config for private dependencies.
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.
well I don't know. Here is an excerpt of meson.log:
Running compile:
Working directory: /build/qemu-9.1.0/build/meson-private/tmpyireszaj
Code:
#ifdef __has_include
#if !__has_include("libaio.h")
#error "Header 'libaio.h' could not be found"
#endif
#else
#include <libaio.h>
#endif
-----------
Command line: `x86_64-unknown-linux-musl-gcc -m64 /build/qemu-9.1.0/build/meson-private/tmpyireszaj/testfile.c -E -P -D_FILE_OFFSET_BITS=64 -P -O0 -std=gnu11` -> 0
Has header "libaio.h" : YES
../meson.build:1031:14: ERROR: C prefer_static library 'aio' not found
corresponding to https://github.com/qemu/qemu/blob/master/meson.build#L1045
It might be that cc.find_library is broken in meson when prefer_static is true.
, jackSupport ? !stdenv.isDarwin && !nixosTestRunner && !minimal, libjack2 | ||
, gtkSupport ? !stdenv.isDarwin && !xenSupport && !nixosTestRunner && !minimal, gtk3, gettext, vte, wrapGAppsHook3 | ||
, pulseSupport ? !stdenv.isDarwin && !nixosTestRunner && !minimal && lib.meta.availableOn stdenv.hostPlatform libpulseaudio, libpulseaudio | ||
, pipewireSupport ? !stdenv.isDarwin && !nixosTestRunner && !minimal && !stdenv.hostPlatform.isStatic, pipewire |
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.
These should all use meta.availableOn rather than hardcoding the condition.
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 did not use it for pipewire because the issue is with a transitive dependency (systemd iirc)
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.
Pipewire can't be built statically, even if you can build all the dependencies (I've tried) so it would make sense for it to be marked unsupported directly.
@@ -78,6 +78,8 @@ stdenv.mkDerivation (finalAttrs: { | |||
hash = "sha256-gWtwIqi6fCrDDi4M+XPoJva8yFBTOWAyEsXt6OlNeDQ="; | |||
}; | |||
|
|||
strictDeps = true; |
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.
Doesn't this break the GTK app hook?
@@ -124,7 +124,7 @@ stdenv.mkDerivation (finalAttrs: { | |||
libtasn1 | |||
tdb | |||
libxcrypt | |||
] ++ optionals stdenv.isLinux [ liburing systemd ] | |||
] ++ optionals (stdenv.isLinux && !stdenv.hostPlatform.isStatic) [ liburing systemd ] |
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.
meta.availableOn
@@ -110,11 +110,23 @@ stdenv.mkDerivation (finalAttrs: { | |||
# https://github.com/NixOS/nixpkgs/pull/118700#issuecomment-885892436 | |||
!stdenv.isDarwin && | |||
|
|||
# tests attempt to link .so files | |||
# https://github.com/dgibson/dtc/commit/dcef5f834ea34bcb9f8d0e86db1268fde52ead77 | |||
!stdenv.hostPlatform.isStatic && |
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.
This was fixed recently in master. You should be able to drop it (with a rebase?).
@@ -112,7 +114,7 @@ stdenv.mkDerivation (finalAttrs: { | |||
++ lib.optionals smartcardSupport [ libcacard ] | |||
++ lib.optionals spiceSupport [ spice-protocol spice ] | |||
++ lib.optionals usbredirSupport [ usbredir ] | |||
++ lib.optionals stdenv.isLinux [ libcap_ng libcap attr ] | |||
++ lib.optionals stdenv.isLinux [ libcap_ng libcap attr audit ] |
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.
++ lib.optionals stdenv.isLinux [ libcap_ng libcap attr audit ] | |
++ lib.optionals stdenv.isLinux [ libcap_ng libcap attr ] | |
++ lib.optionals (stdenv.hostPlatform.isStatic && stdenv.isLinux) [ audit ] |
I rebased on a more recent commit as you suggested. A number of dependencies of qemu that were building on this branch no longer build. As I have no idea how to fix that, I give up. |
Description of changes
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.