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

[Mathieu Lirzin]

Hi,

Mark H Weaver <address@hidden> writes:

> Mathieu Lirzin <address@hidden> 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 <address@hidden>
>>  ;;; Copyright © 2016 Alex Kost <address@hidden>
>>  ;;; Copyright © 2016 Raymond Nicholson <address@hidden>
>> +;;; Copyright © 2016 Mathieu Lirzin <address@hidden>
>>  ;;;
>>  ;;; 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.

0001-gnu-powertop-Patch-absolute-file-names.patch
Description: Text Data

Thanks,

-- 
Mathieu Lirzin