fix(nix): add dlopen RPATH entries in postFixup, after the shrink - #458
Conversation
The assertion added last week caught this on its first real run, which is the whole reason it was rewritten: ldd would have gone green. stdenv's fixupPhase runs `patchelf --shrink-rpath`, which drops every RPATH entry that no DT_NEEDED library needs. An entry that exists solely so a dlopen can resolve is by construction exactly what that describes, so adding it in postInstall meant adding it and then watching it be removed minutes later. The shipped binary carried: $ORIGIN/helper-ffmpeg:/nix/store/...-ffmpeg-tree.../lib: /nix/store/...-glibc.../lib:/nix/store/...-gcc-lib/lib precisely the entries the linker could justify, and not the one that matters. libpipewire-0.3.so.0 would then have failed to load on any host without an ld.so.cache -- every NixOS host -- which is the exact failure packaging the helper was meant to remove. postFixup runs at the end of fixupPhase, after the shrink, so the entry survives. dontPatchELF would also work and is worse: it disables the shrink for the whole output to fix one entry. Worth recording for the next dlopen'd dependency: the compositor addon's vulkan-loader entry is added the same way and is presumably being stripped the same way. It is not covered by any assertion yet, and export works on the runner only because ubuntu-latest has a system libvulkan. Co-Authored-By: Claude <noreply@anthropic.com>
Same defect as the helper's, found by proving the helper's. The vulkan-loader entry was added in installPhase, and fixupPhase's `patchelf --shrink-rpath` removes every RPATH entry no DT_NEEDED library needs -- which is precisely what an entry added for a dlopen is. It has been stripped on every build so far. Nothing noticed because the runner cannot notice: ubuntu-latest carries a system libvulkan that satisfies ash's dlopen regardless of what the RPATH says, and export has been passing there for exactly that reason. On NixOS, where the whole entry exists, it would have failed. That asymmetry is the argument for asserting rather than exercising, so the RPATH check is now a function and runs over both objects: the helper against libpipewire-0.3.so.0 and the addon against libvulkan.so.1. The second one is a claim CI could never have reached by running anything. Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 1 minute Limit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
What the assertion caught
Run 32532627281:
Buildpassed — whisper-stt compiles CPU-only, and with it the FetchContent overrides, the tarball hashes and the subdirectorygitTrackedfilter are all exercised for the first time and all work. Then:The shipped helper's RPATH was:
Precisely the entries the linker could justify, and not the one that matters.
Why
stdenv's
fixupPhaserunspatchelf --shrink-rpath, which drops every RPATH entry that noDT_NEEDEDlibrary needs. An entry that exists solely so adlopencan resolve is, by construction, exactly what that describes. Adding it inpostInstallmeant adding it and then watching it be removed minutes later.postFixupruns at the end offixupPhase, after the shrink, so the entry survives.dontPatchELFwould also work and is worse: it disables the shrink for the whole output to fix one entry.The same bug in the compositor addon
Proving it for the helper proved it for
vulkan-loadertoo — same construction, sameinstallPhase, stripped on every build so far.Nothing noticed, and nothing could have. ubuntu-latest carries a system libvulkan that satisfies ash's
dlopenregardless of the RPATH, which is why export has been passing on the runner. On NixOS — the host the entry exists for — it would have failed.That asymmetry is the argument for asserting rather than exercising, so the RPATH check is now a function running over both objects: the helper against
libpipewire-0.3.so.0, the addon againstlibvulkan.so.1. The second is a claim CI could never have reached by running anything.On the review that made this possible
This is the second time the same review comment pays out. The original check used
ldd, which resolvesDT_NEEDEDentries only — so adlopened soname never appears in it. It would have gone green here, twice, and both components would have shipped unable to load their dlopen dependency on the one platform the packaging targets.Verified
Workflow YAML parses; all five
runblocks passbash -n. The refactored check was tested on four cases — both components with the entry present and with it absent — andmissingpropagates correctly out of the function in each.Not verified: not built here. But the change is a hook move plus a comment, and the failing assertion is now the thing that will confirm or deny it on the next run.
🤖 Generated with Claude Code