autoPatchelfHook: improve arch/ABI compatibility, fix packages that use stdenvNoCC #137886
Conversation
|
/rebase staging |
impl
force-pushed
the
autopatchelfhook-arch-abi-compat
branch
from
b3f40c1 to
3729409
Compare
impl
changed the title
|
@symphorien Sorry, I missed your earlier comment when I added that most recent commit! Did you want me to target this PR against the staging branch instead? |
|
Hum there used to have a github action to do it automatically, but maybe it does not exist anymore. Yes in theory PRs with > 500 rebuilds should target staging. |
Fully enabling crossSystem support for autoPatchelfHook came with some perhaps unintended consequences of being a bit more aggressive about patching ELF files from architectures/ABIs that differ from the target (previously, those files would be ignored because ldd usually couldn't handle them). This change adds architecture and rough OS ABI detection to the script so that it doesn't try to blindly replace the interpreter of files that can't possibly use that interpreter, and also makes sure it doesn't accidentally use libraries of other architectures/ABIs.
autoPatchelfHook actually doesn't depend on stdenv and only needs bintools (with its wrapper). This change uses $NIX_BINTOOLS instead of $NIX_CC and makes the dependency on bintools explicit.
impl
force-pushed
the
autopatchelfhook-arch-abi-compat
branch
from
3729409 to
a7f5e83
Compare
impl
requested review from
adisbladis,
Ericson2314,
FRidh,
jonringer,
LnL7,
marsam,
matthewbauer,
Mic92,
Profpatsch,
thoughtpolice,
ttuegel,
WilliButz and
zowoq
as code owners
github-actions
bot
added
6.topic: bsd
github-actions
bot
removed
8.has: documentation
|
👍 Makes sense, I changed the base branch to staging. It looks like I should have done that before I force-pushed my rebased branch, so my apologies the rest of you that just got flagged as reviewers on this! 😭 |
r-burns
removed request for
thoughtpolice,
matthewbauer,
adisbladis,
marsam,
Mic92,
ttuegel,
LnL7,
Ericson2314,
FRidh,
Profpatsch,
jonringer,
WilliButz and
zowoq
ofborg
bot
added
the
2.status: merge conflict
|
No worries, it's a pretty common footgun. FYI you can rebase onto |
ofborg
bot
removed
the
2.status: merge conflict
| autoPatchelfHook = makeSetupHook | ||
| { name = "auto-patchelf-hook"; deps = [ bintools ]; } |
There was a problem hiding this comment.
| autoPatchelfHook = makeSetupHook | |
| { name = "auto-patchelf-hook"; deps = [ bintools ]; } | |
| autoPatchelfHook = makeSetupHook { name = "auto-patchelf-hook"; deps = [ bintools ]; } |
| if [ -f "$dep" ]; then | ||
| continue | ||
| fi | ||
| elif [ -f "$libcLib/$dep" ]; then |
There was a problem hiding this comment.
Why are we mixing [ and [[?
There was a problem hiding this comment.
I prefer to always use [[ myself when I'm guaranteed to have a bash shell, but I was trying to match the existing style of the file which seemed to only use [[ when it was required and [ otherwise. Maybe I was reading too much into it though. :-)
Since this was already merged, would you like me to open another PR for some of these stylistic fixes?
There was a problem hiding this comment.
NixOS's stdenv is guaranteed to use bash. I wouldn't worry personally about portability at all. You can keep it like it is for now but in the future I think it would be better to either fully use [ or [[ and not mix and match them.
There was a problem hiding this comment.
There was a problem hiding this comment.
I think I am fine with always using [[. This is also how I handle new code that I add to stdenv. We don't have a written style guide yet on this.
Motivation for this change
Fully enabling crossSystem support for autoPatchelfHook came with some perhaps unintended consequences of being a bit more aggressive about patching ELF files from architectures/ABIs that differ from the target (previously, those files would be ignored because ldd usually couldn't handle them).
This change adds architecture and rough OS ABI detection to the script so that it doesn't try to blindly replace the interpreter of files that can't possibly use that interpreter, and also makes sure it doesn't accidentally use libraries of other architectures/ABIs.
I also took the liberty of eliminating the last
shellcheckerror from the file (which, granted, I also introduced). :-)Some examples of packages that will be able to build correctly under this change:
For package maintainers, this also removes the implicit dependency on
stdenv(#137982).cc @symphorien
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)