Re: [PATCH] gnu: powertop: Patch absolute file names.

[Mathieu Lirzin <mthl@gnu.org>]

[-- Attachment #1: Type: text/plain, Size: 4497 bytes --]

Hi,

Mark H Weaver <mhw@netris.org> writes:

> Mathieu Lirzin <mthl@gnu.org> 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 <tobias.geerinckx.rice@gmail.com>
>>  ;;; Copyright © 2016 Alex Kost <alezost@gmail.com>
>>  ;;; Copyright © 2016 Raymond Nicholson <rain1@openmailbox.org>
>> +;;; Copyright © 2016 Mathieu Lirzin <mthl@gnu.org>
>>  ;;;
>>  ;;; 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.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-gnu-powertop-Patch-absolute-file-names.patch --]
[-- Type: text/x-diff, Size: 2067 bytes --]

From efb3e50fe4c86b1bf25fee50a481b45d28c5ff45 Mon Sep 17 00:00:00 2001
From: Mathieu Lirzin <mthl@gnu.org>
Date: Mon, 18 Apr 2016 17:00:44 +0200
Subject: [PATCH] gnu: powertop: Patch absolute file names.

* gnu/packages/linux.scm (powertop)[inputs]: Add kmod.
[arguments]: Patch absolute file names.
---
 gnu/packages/linux.scm | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/gnu/packages/linux.scm b/gnu/packages/linux.scm
index a26e641..9efbe22 100644
--- a/gnu/packages/linux.scm
+++ b/gnu/packages/linux.scm
@@ -1203,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$")
+                 (("/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 handles it.
+                 (("/usr/bin/hcitool") "hcitool")       ;XXX:not packaged yet
+                 (("/usr/bin/xset") "xset")
+                 (("/usr/sbin/hciconfig") "hciconfig")) ;XXX:not packaged yet
+               #t))))))
     (inputs
-     `(("zlib" ,zlib)
-       ("pciutils" ,pciutils)
+     `(("kmod" ,kmod)
+       ("libnl" ,libnl)
        ("ncurses" ,ncurses)
-       ("libnl" ,libnl)))
+       ("pciutils" ,pciutils)
+       ("zlib" ,zlib)))
     (native-inputs
      `(("pkg-config" ,pkg-config)))
     (home-page "https://01.org/powertop/")
-- 
2.8.0.rc3


[-- Attachment #3: Type: text/plain, Size: 29 bytes --]


Thanks,

-- 
Mathieu Lirzin