Skip to content
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

Merged
merged 1 commit into from
Jan 6, 2023

Conversation

KAction
Copy link
Contributor

@KAction KAction commented Jan 4, 2023

pkgs/tools/system/htop/default.nix Show resolved Hide resolved
@@ -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) ''
Copy link
Contributor

@Et7f3 Et7f3 Jan 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
in lib.optionalString (!stdenv.hostPlatform.isStatic) ''
in lib.optionalString systemdSupport ''

avoid the repetion on condition (you omitted stdenv.isLinux)

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@Et7f3 Et7f3 left a 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

Copy link
Contributor

@Et7f3 Et7f3 left a 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

@KAction
Copy link
Contributor Author

KAction commented Jan 5, 2023

Fair enough. Here is asciinema, fwiw: https://asciinema.org/a/dBt7RALg1QZoHRq1juc1bvG3B

Copy link
Contributor

@r-burns r-burns left a 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

@SuperSandro2000
Copy link
Member

@ofborg build pkgsStatic.htop

@SuperSandro2000 SuperSandro2000 merged commit dc475f5 into NixOS:master Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants