elfutils: enable debuginfod support#223240
Conversation
ofborg
bot
added
10.rebuild-darwin: 101-500
|
@ofborg eval |
trofi
left a comment
trofi
left a comment
There was a problem hiding this comment.
LGTM overall. Added one suggestion not to manually split out the dependency.
There was a problem hiding this comment.
I would suggest it to move to existing $bin, not the new $libdebuginfo. bin/debuginfod depends on libdebuginfo.so anyways. There seems to be no reason to keep a new snowflake derivation output if nothing uses it.
There was a problem hiding this comment.
most debug tools (gdb, perf, ...) depend on libdebuginfod.so but not on debuginfod itself (the lib is a client while debuginfod is a server implem), so moving debuginfod to $bin seems wasteful from a closure size point of view.
There was a problem hiding this comment.
The need for patchelf to keep the package running always irks me: it always has a potential missing some reference to patch. Why not keep it in $lib instead? elfutils is a toolchain-style package I would say. It's OK to have a few extra dependencies. Looking at the individual deps you mention:
libarchiveseems to pull in extra 2MB: 800K of libarchive itself and 1.2MB of libxml2 (the rest looks like baseline packages)libcurlseems to pull in extra 5.2MB: curl itself (800KB), libktb5 (2.2MB), brotli (1.7MB) (the rest looks like baseline packages)
I would say these are benign and worth pulling in unconditionally. The curl one is especially weak I would say as curl is already present in a typical system.
WDYT of just installing debuginfod by default?
There was a problem hiding this comment.
WDYT of just installing debuginfod by default?
tbh I expected that people would oppose that, given that elfutils is in the closure of approximately everything through systemd (udev), hence the extra work I did here. If you don't mind I'll wait for a second opinion before going in this direction.
The need for patchelf to keep the package running always irks me: it always has a potential missing some reference to patch.
there are install tests, and they catch the regression if one remove the patchelf invocation, so that seems unlikely.
To give actual numbers: elfutils.bin grows from 47MB to 73MB in closure size by enabling debuginfod support, and the exact list of added dependencies is sqlite, curl, libmicrohttpd, libarchive. It is true that systemd depends on curl and libmicrohttpd, and that nix depends the remaining two, so we will likely have them in most nixos closures.
most reverse dependencies are debuggers which benefit from this support, plus systemd which already depends on half the additional depenendencies.
symphorien
force-pushed
the
elfutils_full
branch
from
8bdf892 to
b7bad82
Compare
|
I changed the pull request to just setting enableDebuginfod to true without patchelf hacks. |
|
This PR breaks the It would be nice if we could only enable |
|
Yeah, conditionally enabling in Can you give it a try? |
3 participants
elfutils contains several libraries, and when debuginfod support is enabled, only libdebuginfod.so depends on libarchive and curl, among others, leading to a large closure size increase.
To mitigate this effect, libdebuginfod.so is moved to a separate output. libarchive and libcurl remain in the build closure, but derivations which depend on, say, libelf, but not libdebuginfod only retain a reference to the out output and not the libdebuginfod output and their runtime closure size does not increase.
Enabling libdebuginfod by default allows to use nixseparatedebuginfod without recompiling gdb and perf, mostly.
This admittedly complex contraption ensures we don't get two copies of libelf (one for systemd withouth debuginfod support, and one for debuginfod enabled gdb) in this case.
The bin output contains build tools, and since curl and libarchive are always in the build closure we dont move debuginfod and debuginfod-find to a separate output.
tested by rebuilding
/nix/store/axqgmkgmhy2rzlbffglhjv1npy5ni0ra-gdb-13.1
/nix/store/73cbnzc96xir60xff5qblhvw6fb7n309-perf-linux-6.1.21
/nix/store/nk7lla63w1pbzyavgz7hcvdhmrd9qgqy-systemtap-6.1.21-4.8
/nix/store/bhss1whsh67b0wa1mj5h3z1p4nwndxd4-valgrind-3.20.0
/nix/store/v2hlzj0gbnycj7ssznvjvhw5bxwxsbmm-hotspot-1.4.1
/nix/store/7cnyh97201zk780zwbh1sh913wflsgjp-heaptrack-1.4.0
/nix/store/1893dw0r67hh6iqbjdhwbpb8dsc1i8iy-networkmanager-1.40.12
/nix/store/xc5yfb7v042ypmbwwswpsq91jwapls4k-dwz-0.14
/nix/store/vfyvlmrnns4k0dppd6fnddkxc1s3dkck-linux-6.1.21
and checking which of these keep the debuginfod reference.
Description of changes
Things done
sandbox = trueset 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/)