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

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

[Mathieu Lirzin]

* gnu/packages/linux.scm (powertop)[inputs]: Add kmod.
[arguments]: Patch absolute file names.  Before that launching powertop
was failing because 'modprobe' was not found.
---
 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" )
+                 (("/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")
+                 (("/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)))
     (native-inputs
      `(("pkg-config" ,pkg-config)))
     (home-page "https://01.org/powertop/")
-- 
2.7.3

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

[Ricardo Wurmus]

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.

Looks good to me, but I have one thing to nitpick below.

> +           (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 handle it.

Should it not be “handles”?

> +                 (("/usr/bin/xset") "xset")
> +                 (("/usr/sbin/hciconfig") "hciconfig") ;XXX:not packaged yet
> +                 (("/usr/bin/hcitool") "hcitool"))     ;XXX:not packaged yet
> +               #t))))))

~~ Ricardo

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

[Mathieu Lirzin]

Ricardo Wurmus <rekado@elephly.net> 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.
>
> Looks good to me, but I have one thing to nitpick below.
>
>> +           (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 handle it.
>
> Should it not be “handles”?

Indeed you are right. :)

Thanks for your review.

-- 
Mathieu Lirzin

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

[Efraim Flashner]

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

On Mon, Apr 18, 2016 at 11:01:41PM +0200, Mathieu Lirzin wrote:
> * gnu/packages/linux.scm (powertop)[inputs]: Add kmod.
> [arguments]: Patch absolute file names.  Before that launching powertop
> was failing because 'modprobe' was not found.
> ---
>  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" )
> +                 (("/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")
> +                 (("/usr/sbin/hciconfig") "hciconfig") ;XXX:not packaged yet
> +                 (("/usr/bin/hcitool") "hcitool"))     ;XXX:not packaged yet

hci* programs are part of the bluez package. According to apt-file,
debian has xset as part of x11-xserver-utils.

> +               #t))))))
>      (inputs
> -     `(("zlib" ,zlib)
> -       ("pciutils" ,pciutils)
> +     `(("kmod" ,kmod)
>         ("ncurses" ,ncurses)
> -       ("libnl" ,libnl)))
> +       ("pciutils" ,pciutils)
> +       ("libnl" ,libnl)
> +       ("zlib" ,zlib)))
>      (native-inputs
>       `(("pkg-config" ,pkg-config)))
>      (home-page "https://01.org/powertop/")
> 

-- 
Efraim Flashner   <efraim@flashner.co.il>   אפרים פלשנר
GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Mathieu Lirzin]

Efraim Flashner <efraim@flashner.co.il> writes:

> On Mon, Apr 18, 2016 at 11:01:41PM +0200, Mathieu Lirzin wrote:
>
>> +    (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 handle it.
>> +                 (("/usr/bin/xset") "xset")
>> +                 (("/usr/sbin/hciconfig") "hciconfig") ;XXX:not packaged yet
>> +                 (("/usr/bin/hcitool") "hcitool"))     ;XXX:not packaged yet
>
> hci* programs are part of the bluez package. According to apt-file,
> debian has xset as part of x11-xserver-utils.

Thank you for taking to search this.  :) I don't intend to package
"bluez" since I have no use of Bluetooth, and xset is already packaged
in Guix.  Those programs are called when doing ‘powertop --calibrate’
like this:

  if(!system("DISPLAY=:0 /usr/bin/xset dpms force off"))
    printf("System is not available\n");
                
So IMO adding them as explicit inputs will not make sense since those
‘system’ calls are meant as feature tests.  WDYT?

-- 
Mathieu Lirzin

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

[Ludovic Courtès]

Mathieu Lirzin <mthl@gnu.org> skribis:

> Efraim Flashner <efraim@flashner.co.il> writes:
>
>> On Mon, Apr 18, 2016 at 11:01:41PM +0200, Mathieu Lirzin wrote:
>>
>>> +    (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 handle it.
>>> +                 (("/usr/bin/xset") "xset")
>>> +                 (("/usr/sbin/hciconfig") "hciconfig") ;XXX:not packaged yet
>>> +                 (("/usr/bin/hcitool") "hcitool"))     ;XXX:not packaged yet
>>
>> hci* programs are part of the bluez package. According to apt-file,
>> debian has xset as part of x11-xserver-utils.
>
> Thank you for taking to search this.  :) I don't intend to package
> "bluez" since I have no use of Bluetooth, and xset is already packaged
> in Guix.  Those programs are called when doing ‘powertop --calibrate’
> like this:
>
>   if(!system("DISPLAY=:0 /usr/bin/xset dpms force off"))
>     printf("System is not available\n");
>                 
> So IMO adding them as explicit inputs will not make sense since those
> ‘system’ calls are meant as feature tests.  WDYT?

Agreed.  It’s OK to have them searched in $PATH as needed, so the patch
above is OK (without “XXX” comments even.)

Thanks,
Ludo’.

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

[Mark H Weaver]

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.

> ---
>  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.

> +                 (("/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'?

> +                 (("/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'?

      Thanks!
        Mark

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

[Mathieu Lirzin]

[-- 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

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

[Ludovic Courtès]

Mathieu Lirzin <mthl@gnu.org> skribis:

> 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.

[...]

> +               (substitute* (find-files "src" "\\.cpp$")
> +                 (("/sbin/modprobe") (string-append kmod "/bin/modprobe"))

Maybe with a comment saying “Give the right ‘modprobe’ file name so that
essential modules such as msr.ko can be loaded.”

> +                 ;; 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

I think this is fine too, but regardless, please commit the patch in
this form since it fixes the IMO important modprobe issue above.  :-)

Thanks!

Ludo’.