-
-
Notifications
You must be signed in to change notification settings - Fork 14.8k
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
lm_sensors: fix build, split outputs, modernize and remove perl from library closure #373242
Conversation
]; | ||
|
||
outputs = [ | ||
"bin" |
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.
5 outputs are a lot for such a small tool. We can probably drop the bin output without any impact
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.
The split of bin
and out
is the main purpose of this PR. It removes perl
dependency for downstream packages that only want libsensors, which is the main use case from a quick search.
I personally want to remove "doc" but I want to wait for opinion from @ peterhoeg
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.
The fan2go changes lgtm. Thanks for improving the closure size!
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.
Tested this and it seems to work well. Diff LGTM.
@oxalica are we good to merge this?
Yes, it's ready, thanks. We can always remove "doc" output later if needed. |
assert sensord -> rrdtool != null; | ||
let | ||
version = "3.6.0"; | ||
tag = lib.replaceStrings [ "." ] [ "-" ] version; |
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.
Should this be "V3-6-0" rather than just "3-6-0"?
I'm getting the following error when trying to build from source:
trying https://github.com/lm-sensors/lm-sensors/archive/refs/tags/3-6-0.tar.gz
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0
0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0
curl: (22) The requested URL returned error: 404
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.
Thanks for the catch! Filed #375622
I think either this change or a different one relating to lm_sensors has broken the hardware.fancontrol option. The service it creates exec's but the sbin dir no longer exists, so it should be bin/fancontrol instead right? |
yeah, probably. sbin has no special meaning in nixpkgs. |
sensord = true
on GCC 14.perl
, removingperl
from closure of its dependents (especially, htop).tag =
forfetchFromGitHub
.--replace-fail
forsubstituteInPlace
.enableParallelBuilding = true
Unresolved:
lm-sensors
(dash instead of underscore), because of the upstream project name? The code and docs seem to use lm_sensors and lm-sensors interchangeable. For reference, Arch and Fedora both uselm_sensors
(underscore). I don't have a strong opinion about it.doc
output really useful? It's done by manuallycp -r configs doc/* <outdir>
and it's not installed by upstream Makefile. The arch package does not install these files either. cc the introducer @peterhoegThings done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.