-
-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
pkgsStatic.htop: fix build #209111
pkgsStatic.htop: fix build #209111
Conversation
@@ -33,8 +34,7 @@ stdenv.mkDerivation rec { | |||
postFixup = | |||
let | |||
optionalPatch = pred: so: lib.optionalString pred "patchelf --add-needed ${so} $out/bin/htop"; | |||
in | |||
'' | |||
in lib.optionalString (!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.
in lib.optionalString (!stdenv.hostPlatform.isStatic) '' | |
in lib.optionalString systemdSupport '' |
avoid the repetion on condition (you omitted stdenv.isLinux)
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 not about systemd, it is about static linking. Static linking => no shared libraries => no patchelf. You may choose to build without systemd, but with lm_sensors on dynamic system, at which point readelf should be called.
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 if you build without systemd, but with lm_sensors on dynamic system, you'd need to patchelf libsensors but not libsystemd, right? So I think you still need to make the first patchelf command conditional on !isStatic
but the second one conditional on systemdSupport
.
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 think this code is correct as is. Note that there are two layers of conditionals.
Outer one, which says that patchelf
only applies to dynamic systems.
Inner one, embedded into optionalPatch
that says that you patch only library you built.
Honestly, I don't understand why patchelf
is needed in first place, but that it exercise for another day. Point is that my patch is not making things worse.
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.
Ok I remove my request for change
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.
Oops I remove my approval as I haven't tested
Fair enough. Here is asciinema, fwiw: https://asciinema.org/a/dBt7RALg1QZoHRq1juc1bvG3B |
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.
pkgsStatic.htop builds OK, just have a question about the patchelf conditions above
@ofborg build pkgsStatic.htop |
maintainers: @rbvermaa @relrod @SuperSandro2000
build log on x86_64 NixOS: https://gist.github.com/31f7af603abb5c68a7c5c964d57641d3