From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mathieu Lirzin Subject: Re: [PATCH] gnu: powertop: Patch absolute file names. Date: Sat, 23 Apr 2016 14:12:15 +0200 Message-ID: <87shyccyn4.fsf@gnu.org> References: <1461013301-13196-1-git-send-email-mthl@gnu.org> <87twitf0u0.fsf@netris.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:39285) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1atwQB-0001T6-Ov for guix-devel@gnu.org; Sat, 23 Apr 2016 08:12:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1atwQ7-0004uA-HF for guix-devel@gnu.org; Sat, 23 Apr 2016 08:12:23 -0400 In-Reply-To: <87twitf0u0.fsf@netris.org> (Mark H. Weaver's message of "Fri, 22 Apr 2016 23:41:59 -0400") List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: "Guix-devel" To: Mark H Weaver Cc: guix-devel@gnu.org --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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 =E2=80=9Crationale=E2=80=9D 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 =C2=A9 2016 Tobias Geerinckx-Rice >> ;;; Copyright =C2=A9 2016 Alex Kost >> ;;; Copyright =C2=A9 2016 Raymond Nicholson >> +;;; Copyright =C2=A9 2016 Mathieu Lirzin >> ;;; >> ;;; This file is part of GNU Guix. >> ;;; >> @@ -1202,11 +1203,29 @@ devices. It replaces 'iwconfig', which is depre= cated.") >> (base32 >> "0nlwazxbnn0k6q5f5b09wdhw0f194lpzkp3l7vxansqhfczmcyx8")))) >> (build-system gnu-build-system) >> + (arguments >> + '(#:phases >> + (modify-phases %standard-phases >> + ;; TODO: Patch some hardcoded "wlan0" in calibrate/calibrate.c= pp 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 usi= ng >> + ;; relative file names avoids adding extra inputs. Wh= en 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 =E2=80=98powertop --calibrate=E2=80=99 which tries to invoke various progra= ms for battery consumption measurements. In the case of xset, it is done like this: if (!system ("DISPLAY=3D:0 /usr/bin/xset dpms force off")) printf ("System is not available\n"); =20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20 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 =E2=80=98%desktop-packages=E2=80=99 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 package= d yet >> + (("/usr/bin/hcitool") "hcitool")) ;XXX:not package= d 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. --=-=-= Content-Type: text/x-diff Content-Disposition: attachment; filename=0001-gnu-powertop-Patch-absolute-file-names.patch >From efb3e50fe4c86b1bf25fee50a481b45d28c5ff45 Mon Sep 17 00:00:00 2001 From: Mathieu Lirzin 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 --=-=-= Content-Type: text/plain Thanks, -- Mathieu Lirzin --=-=-=--