Hi, Mark H Weaver writes: > Mathieu Lirzin writes: > >> * gnu/packages/linux.scm (powertop)[inputs]: Add kmod. >> [arguments]: Patch absolute file names. Before that launching powertop >> was failing because 'modprobe' was not found. > > By convention, we don't include rationales in the commit log. When > needed, they should go in the source code, but in this case I don't > think it's needed, so I would just drop the last sentence above. This sentence is not a “rationale” since it only describes the overall purpose of the patch. Nonetheless I think it is OK to remove it since the title is clear enough in the context of Guix. >> --- >> gnu/packages/linux.scm | 25 ++++++++++++++++++++++--- >> 1 file changed, 22 insertions(+), 3 deletions(-) >> >> diff --git a/gnu/packages/linux.scm b/gnu/packages/linux.scm >> index d554ecc..88afa8e 100644 >> --- a/gnu/packages/linux.scm >> +++ b/gnu/packages/linux.scm >> @@ -10,6 +10,7 @@ >> ;;; Copyright © 2016 Tobias Geerinckx-Rice >> ;;; Copyright © 2016 Alex Kost >> ;;; Copyright © 2016 Raymond Nicholson >> +;;; Copyright © 2016 Mathieu Lirzin >> ;;; >> ;;; This file is part of GNU Guix. >> ;;; >> @@ -1202,11 +1203,29 @@ devices. It replaces 'iwconfig', which is deprecated.") >> (base32 >> "0nlwazxbnn0k6q5f5b09wdhw0f194lpzkp3l7vxansqhfczmcyx8")))) >> (build-system gnu-build-system) >> + (arguments >> + '(#:phases >> + (modify-phases %standard-phases >> + ;; TODO: Patch some hardcoded "wlan0" in calibrate/calibrate.cpp to >> + ;; allow calibrating the network interface in GuixSD. >> + (add-after 'unpack 'patch-absolute-file-names >> + (lambda* (#:key inputs #:allow-other-keys) >> + (let ((kmod (assoc-ref inputs "kmod"))) >> + (substitute* (find-files "src" ".*\\.cpp" ) > > Instead of ".*\\.cpp", it should be "\\.cpp$". Also, please remove the > space before the close parenthesis. Sure. >> + (("/sbin/modprobe") (string-append kmod "/bin/modprobe")) >> + ;; These programs are only needed to calibrate, so using >> + ;; relative file names avoids adding extra inputs. When they >> + ;; are missing powertop gracefully handle it. >> + (("/usr/bin/xset") "xset") > > Shouldn't 'xset' be patched to use an absolute file name, as you did > with 'modprobe'? As I attempted to explain in the above comment and in my previous answer to Efraim, xset is not a dependency here since an X server is not required to to use powertop (but modprobe is) and providing xset at compile time does not snap additional features via the configure script (unlike Emacs for example). In fact xset is only invoked when doing ‘powertop --calibrate’ which tries to invoke various programs for battery consumption measurements. In the case of xset, it is done like this: if (!system ("DISPLAY=:0 /usr/bin/xset dpms force off")) printf ("System is not available\n"); Not adding xset as an input has the advantage of keeping the closure size low. However what sucks is that Desktop users are currently required to have it explicitly installed in the current user or system profile to allow powertop to find it. Maybe we could create a ‘%desktop-packages’ variable which would provide utilities like xrdb, xset, xrandr, .... This variable would indeed be included in the Desktop OS configuration templates. WDYT? Apparently my commentary in the source code fails to explain the rationale. Do you have a better suggestion? >> + (("/usr/sbin/hciconfig") "hciconfig") ;XXX:not packaged yet >> + (("/usr/bin/hcitool") "hcitool")) ;XXX:not packaged yet >> + #t)))))) >> (inputs >> - `(("zlib" ,zlib) >> - ("pciutils" ,pciutils) >> + `(("kmod" ,kmod) >> ("ncurses" ,ncurses) >> - ("libnl" ,libnl))) >> + ("pciutils" ,pciutils) >> + ("libnl" ,libnl) >> + ("zlib" ,zlib))) > > Is there a reason that you rearranged these instead of simply adding > 'kmod'? I think I started to rearrange them in lexicographical order, and then forgot about it while hacking. ;) Here is the updated patch.