[bug#57537] [PATCH] gnu: Add ec

[bug#57537] [PATCH] gnu: Add ec

[Denis 'GNUtoo' Carikli]

* gnu/packages/linux.scm (ec): New variable.

Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
---
 gnu/packages/linux.scm | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/gnu/packages/linux.scm b/gnu/packages/linux.scm
index 5254b20dc4..bf71dad722 100644
--- a/gnu/packages/linux.scm
+++ b/gnu/packages/linux.scm
@@ -1375,6 +1375,44 @@ (define-public librem-ec-acpi-linux-module
 and the notification, WiFi, and Bluetooth LED.")
     (license license:gpl2)))
 
+(define-public ec
+  (package
+    (name "ec")
+    (version (package-version linux-libre))
+    (source (package-source linux-libre))
+    (build-system gnu-build-system)
+    (native-inputs (list coreutils))
+    (arguments
+     '(#:make-flags (list (string-append "DESTDIR="
+                                         (assoc-ref %outputs "out")))
+       #:phases (modify-phases %standard-phases
+                  (delete 'configure) ;no configure script
+                  (add-after 'unpack 'patch-Makefile
+                    (lambda _
+                      (substitute* "tools/power/acpi/Makefile.config"
+                        (("/bin/true")
+                         (which "true")))
+                      (substitute* "tools/power/acpi/Makefile.config"
+                        (("/usr/bin/install")
+                         (which "install"))) #t))
+                  (add-after 'patch-Makefile 'enter-subdirectory
+                    (lambda _
+                      (chdir "tools/power/acpi/tools/ec") #t)))
+       #:tests? #f)) ;no tests
+    (home-page (package-home-page linux-libre))
+    (synopsis
+     "Low level utility for reading or writing Embedded Controller registers")
+    (description
+     "This utility can read or write specific registers or all the
+available registers of the Embedded Controllers supported by the Linux
+kernel.  To work it needs to run as root, to have the ec_sys driver
+loaded, and to have the debugfs filesystem mounted at
+/sys/kernel/debug/.  To make write support work, the ec_sys module
+needs to be loaded with the write_support=1 parameter.  Write support
+can also be enabled after loading the module with
+the 'echo 1 > /sys/module/ec_sys/parameters/write_support' command.")
+    (license license:gpl2)))
+
 (define-public lkrg
   (package
     (name "lkrg")

base-commit: 4d361a6b5147e3f91573e9d3c8c540a233e7e142
-- 
2.37.2

[bug#57537] [PATCH] gnu: Add ec

[Denis 'GNUtoo' Carikli]

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

Hi,

On Fri,  2 Sep 2022 04:00:49 +0200
Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org> wrote:
> +     '(#:make-flags (list (string-append "DESTDIR="
> +                                         (assoc-ref %outputs "out")))
This installs the 'ec' binary in DESTDIR/usr/sbin which is wrong.

I'll fix it and send a v2.

Denis.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[bug#57537] [PATCH v2] gnu: Add ec

[Denis 'GNUtoo' Carikli]

* gnu/packages/linux.scm (ec): New variable.
---
 gnu/packages/linux.scm | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/gnu/packages/linux.scm b/gnu/packages/linux.scm
index 5254b20dc4..dd91ca60fc 100644
--- a/gnu/packages/linux.scm
+++ b/gnu/packages/linux.scm
@@ -1375,6 +1375,45 @@ (define-public librem-ec-acpi-linux-module
 and the notification, WiFi, and Bluetooth LED.")
     (license license:gpl2)))
 
+(define-public ec
+  (package
+    (name "ec")
+    (version (package-version linux-libre))
+    (source (package-source linux-libre))
+    (build-system gnu-build-system)
+    (native-inputs (list coreutils))
+    (arguments
+     '(#:make-flags (list (string-append "DESTDIR="
+                                         (assoc-ref %outputs "out"))
+                          "sbindir=/sbin")
+       #:phases (modify-phases %standard-phases
+                  (delete 'configure) ;no configure script
+                  (add-after 'unpack 'patch-Makefile
+                    (lambda _
+                      (substitute* "tools/power/acpi/Makefile.config"
+                        (("/bin/true")
+                         (which "true")))
+                      (substitute* "tools/power/acpi/Makefile.config"
+                        (("/usr/bin/install")
+                         (which "install"))) #t))
+                  (add-after 'patch-Makefile 'enter-subdirectory
+                    (lambda _
+                      (chdir "tools/power/acpi/tools/ec") #t)))
+       #:tests? #f)) ;no tests
+    (home-page (package-home-page linux-libre))
+    (synopsis
+     "Low level utility for reading or writing Embedded Controller registers")
+    (description
+     "This utility can read or write specific registers or all the
+available registers of the Embedded Controllers supported by the Linux
+kernel.  To work it needs to run as root, to have the ec_sys driver
+loaded, and to have the debugfs filesystem mounted at
+/sys/kernel/debug/.  To make write support work, the ec_sys module
+needs to be loaded with the write_support=1 parameter.  Write support
+can also be enabled after loading the module with
+the 'echo 1 > /sys/module/ec_sys/parameters/write_support' command.")
+    (license license:gpl2)))
+
 (define-public lkrg
   (package
     (name "lkrg")

base-commit: 4d361a6b5147e3f91573e9d3c8c540a233e7e142
-- 
2.37.2

[bug#57537] [PATCH v2] gnu: Add ec

[Tobias Geerinckx-Rice via Guix-patches via]

Hi Denis,

This tool has come in handy a few times.  (But did I package it?  Nope…) 
  Thank you for doing so!

Don't forget to add a copyright line for yourself at the top of the 
file.

Denis 'GNUtoo' Carikli 写道：
> This installs the 'ec' binary in DESTDIR/usr/sbin which is wrong.

Indeed, using ‘DESTDIR’ is almost always a mistake.

There are a handful of occurrences in Guix, as a hack to get a buggy 
Makefile to work, but those are (thankfully) rare.

Your v2 still has the ‘DESTDIR’ in it, though.  Missed when committing?

> gnu: Add ec

We ♥ full stops, even here.

> +(define-public ec
> +  (package
> +    (name "ec")
> +    (version (package-version linux-libre))
> +    (source (package-source linux-libre))
> +    (build-system gnu-build-system)
> +    (native-inputs (list coreutils))

I doubt this has any effect?

> +    (arguments
> +     '(#:make-flags (list (string-append "DESTDIR="
> +                                         (assoc-ref %outputs "out"))

The magical %output{s,} is fragile and almost never necessary in new 
code.  Make the arguments a list of keywords/gexps and use #$output 
instead.

> +                          "sbindir=/sbin")
> +       #:phases (modify-phases %standard-phases
> +                  (delete 'configure) ;no configure script

It makes no difference to Guix, but for the sake of humans: manipulate 
phases in their original order when there's no reason not to.

Here, 'configure runs after 'unpack.

> +                  (add-after 'unpack 'patch-Makefile
> +                    (lambda _
> +                      (substitute* "tools/power/acpi/Makefile.config"
> +                        (("/bin/true")
> +                         (which "true")))
> +                      (substitute* "tools/power/acpi/Makefile.config"
> +                        (("/usr/bin/install")
> +                         (which "install"))) #t))

Aside: no need to call substitute* twice on the exact same (list of) 
file(s).
Nor is it necessary to look up binaries in PATH ‘by hand’.  Most tools 
do that already.

                       (substitute* "Makefile.config"
                         (("/bin/true") "true")
                         (("/usr/bin/install") "install"))

However, substitution is overkill as that Makefile doesn't hard-code 
either name.

Instead, let's just use the make-flags upstream provides:

   λ grep '= /.*bin' Makefile*
   Makefile.config:INSTALL = /usr/bin/install -c
   Makefile.config:      STRIPCMD = /bin/true -Since_we_are_debugging

> +                  (add-after 'patch-Makefile 'enter-subdirectory
> +                    (lambda _
> +                      (chdir "tools/power/acpi/tools/ec") #t)))

I'm happy to be the one to inform you that trailing #ts are 
long-obsolete.

Feel free to remove them whenever you encounter them.  It is very 
satisfying.

> +       #:tests? #f)) ;no tests
> +    (home-page (package-home-page linux-libre))
> +    (synopsis
> +     "Low level utility for reading or writing Embedded Controller 
> registers")
> +    (description
> +     "This utility can read or write specific registers or all the
> +available registers of the Embedded Controllers supported by the Linux

The ‘ec’ description should probably mention the word ‘EC’ *somewhere*… 
;-)

   → ‘@acronym{EC, Embedded Controller}’

> +kernel.  To work it needs to run as root, to have the ec_sys driver
> +loaded, and to have the debugfs filesystem mounted at
> +/sys/kernel/debug/.

‘@code{ec_sys} module loaded’, ‘@code{debugfs} file system’,
‘@file{/sys/kernel/debug}’.

>                       To make write support work, the ec_sys module
> +needs to be loaded with the write_support=1 parameter.  Write support
> +can also be enabled after loading the module with
> +the 'echo 1 > /sys/module/ec_sys/parameters/write_support' command.")

Using @code{}/@file{}/@samp{}… also applies here, but do you know if 
there's *any* documentation we could install?  This kind of 
highly-specific how-to isn't a great package description.

I wanted to send back a proper patch but this (below) is all I have time 
for.  I can't push yet anyway, so no rush.

Thanks again,

T G-R

(define-public ec
   (package
     (name "ec")
     (version (package-version linux-libre))
     (source (package-source linux-libre))
     (build-system gnu-build-system)
     (arguments
      (list
       #:tests? #f                      ; no tests
       #:make-flags
       #~(list (string-append "sbindir=" #$output "/sbin")
               "INSTALL=install" "STRIPCMD=true")
       #:phases
       #~(modify-phases %standard-phases
           (add-after 'unpack 'enter-subdirectory
             (lambda _
               (chdir "tools/power/acpi/tools/ec")))
           (delete 'configure))))       ; no configure script
     (home-page (package-home-page linux-libre))
     (synopsis "Low level utility to read or write Embedded Controller 
registers")
     (description
      "…")
     (license license:gpl2)))

[bug#57537] [PATCH v2] gnu: Add ec

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 367 bytes --]


On 02-09-2022 06:47, Tobias Geerinckx-Rice via Guix-patches via wrote:
> #~(list (string-append "sbindir=" #$output "/sbin")
>               "INSTALL=install" "STRIPCMD=true") 

For cross-compilation, you probably need to add (string-append "CC=" 
#$(cc-for-target)) as well, assuming it's the same system as for 'tmon'.

Greetings,
Maxime.


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[bug#57537] [PATCH v2] gnu: Add ec

[Denis 'GNUtoo' Carikli]

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

On Fri, 02 Sep 2022 06:47:39 +0200
Tobias Geerinckx-Rice <me@tobias.gr> wrote:

> Hi Denis,
Hi,

> This tool has come in handy a few times.  (But did I package it?
> Nope…) Thank you for doing so!
Thanks for the review and sorry to have left so many issues in the
patch.

> Don't forget to add a copyright line for yourself at the top of the 
> file.
I thought I already had it but apparently I forgot to add it in the
commit f16e6b505d5d2630b786a0477ec73b42e77b04e4 ("gnu: Add
usbip-utils"). So I'll add it in the v3 of this ec patch.

> > gnu: Add ec
> 
> We ♥ full stops, even here.
Fixed.

> > +(define-public ec
> > +  (package
> > +    (name "ec")
> > +    (version (package-version linux-libre))
> > +    (source (package-source linux-libre))
> > +    (build-system gnu-build-system)
> > +    (native-inputs (list coreutils))
> 
> I doubt this has any effect?
It indeed still works without it. I added it because it didn't find
true, and then I added the substitute after that. And I forgot to test
without (native-inputs (list coreutils)).

> > +    (arguments
> > +     '(#:make-flags (list (string-append "DESTDIR="
> > +                                         (assoc-ref %outputs
> > "out"))
> 
> The magical %output{s,} is fragile and almost never necessary in new 
> code.  Make the arguments a list of keywords/gexps and use #$output 
> instead.
Thanks I've now fixed it.

> 
> > +                          "sbindir=/sbin")
> > +       #:phases (modify-phases %standard-phases
> > +                  (delete 'configure) ;no configure script
> 
> It makes no difference to Guix, but for the sake of humans:
> manipulate phases in their original order when there's no reason not
> to.
Thanks, I've also fixed that.

> > +                  (add-after 'unpack 'patch-Makefile
> > +                    (lambda _
> > +                      (substitute*
> > "tools/power/acpi/Makefile.config"
> > +                        (("/bin/true")
> > +                         (which "true")))
> > +                      (substitute*
> > "tools/power/acpi/Makefile.config"
> > +                        (("/usr/bin/install")
> > +                         (which "install"))) #t))
> 
> Aside: no need to call substitute* twice on the exact same (list of) 
> file(s).
Thanks. Here I forgot to try to do something like that.

> Nor is it necessary to look up binaries in PATH ‘by hand’.  Most
> tools do that already.
> 
>                        (substitute* "Makefile.config"
>                          (("/bin/true") "true")
>                          (("/usr/bin/install") "install"))
Thanks for the tip, that's indeed way better.

> However, substitution is overkill as that Makefile doesn't hard-code 
> either name.
Right, it works if I do STRIPCMD=true and INSTALL=install. I should
re-read the GNU make manual on variables assignments (I assumed '=' was
like ':=').

> > +                  (add-after 'patch-Makefile 'enter-subdirectory
> > +                    (lambda _
> > +                      (chdir "tools/power/acpi/tools/ec") #t)))
> 
> I'm happy to be the one to inform you that trailing #ts are 
> long-obsolete.
Nice!

> > +       #:tests? #f)) ;no tests
> > +    (home-page (package-home-page linux-libre))
> > +    (synopsis
> > +     "Low level utility for reading or writing Embedded Controller 
> > registers")
> > +    (description
> > +     "This utility can read or write specific registers or all the
> > +available registers of the Embedded Controllers supported by the
> > Linux
> 
> The ‘ec’ description should probably mention the word ‘EC’
> *somewhere*… ;-)
> 
>    → ‘@acronym{EC, Embedded Controller}’
> > +kernel.  To work it needs to run as root, to have the ec_sys driver
> > +loaded, and to have the debugfs filesystem mounted at
> > +/sys/kernel/debug/.
> 
> ‘@code{ec_sys} module loaded’, ‘@code{debugfs} file system’,
> ‘@file{/sys/kernel/debug}’.
>
> >                       To make write support work, the ec_sys module
> > +needs to be loaded with the write_support=1 parameter.  Write
> > support +can also be enabled after loading the module with
> > +the 'echo 1 > /sys/module/ec_sys/parameters/write_support'
> > command.")
> 
> Using @code{}/@file{}/@samp{}… also applies here
Thanks I've now fixed that.

> do you know if 
> there's *any* documentation we could install?
There is Documentation/ABI/testing/debugfs-ec, that mention the ec
program, but it doesn't really tell how to use it. Though it has a very
useful warnings.

I also wonder if it's that useful to have a complete HOWTO as this tool
is only useful for advanced users that know what they are doing or for
reuse in other programs/scripts that do the necessary checks.

In my case I use it in a userspace program to force the detection of the
Thinkpad X200 dock in userspace and also for leds (though here I could
also upstream better Linux support for that, but that requires time).

> This kind of highly-specific how-to isn't a great package
> description.
The issue here is that I've been trying to make two different thing fit
inside the package description.

Since Guix also has experimental support for HURD and that it can also
cross compile binaries for Windows through mingw, I think it's a good
idea to at least mention the dependencies of that utility.

If I use the word "driver" instead of module, to make sure that people
do understand that the ec_sys is not an out of tree module, and that I
remove the HOWTO part I can shrink it to that:
> This utility can read or write specific registers or all the available
> registers of the @acronym{EC, Embedded Controller} supported by the
> @code{ec_sys} Linux driver.")

Denis.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[bug#57537] [PATCH v2] gnu: Add ec.

[Denis 'GNUtoo' Carikli]

* gnu/packages/linux.scm (ec): New variable.
---
 gnu/packages/linux.scm | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/gnu/packages/linux.scm b/gnu/packages/linux.scm
index 306c18e398..0b38bc03f8 100644
--- a/gnu/packages/linux.scm
+++ b/gnu/packages/linux.scm
@@ -64,6 +64,7 @@
 ;;; Copyright © 2022 Artyom V. Poptsov <poptsov.artyom@gmail.com>
 ;;; Copyright © 2022 Rene Saavedra <nanuui@protonmail.com>
 ;;; Copyright © 2022 muradm <mail@muradm.net>
+;;; Copyright © 2022 Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
 
 ;;;
 ;;; This file is part of GNU Guix.
@@ -1342,6 +1343,31 @@ (define-public librem-ec-acpi-linux-module
 and the notification, WiFi, and Bluetooth LED.")
     (license license:gpl2)))
 
+(define-public ec
+  (package
+    (name "ec")
+    (version (package-version linux-libre))
+    (source (package-source linux-libre))
+    (build-system gnu-build-system)
+    (arguments
+     (list #:make-flags #~(list (string-append "sbindir="
+                                               #$output "/sbin")
+                                "INSTALL=install" "STRIPCMD=true")
+           #:phases #~(modify-phases %standard-phases
+                        (add-after 'unpack 'enter-subdirectory
+                          (lambda _
+                            (chdir "tools/power/acpi/tools/ec")))
+                        (delete 'configure)) ;no configure script
+           #:tests? #f)) ;no tests
+    (home-page (package-home-page linux-libre))
+    (synopsis
+     "Low level utility for reading or writing @acronym{EC, Embedded Controller} registers")
+    (description
+     "This utility can read or write specific registers or all the available
+registers of the @acronym{EC, Embedded Controller} supported by the
+@code{ec_sys} Linux driver")
+    (license license:gpl2)))
+
 (define-public lkrg
   (package
     (name "lkrg")

base-commit: dc7191302e6d099a26673e08b78eb5f4b2a2b17b
-- 
2.37.3

bug#57537: [PATCH] gnu: Add ec

[Ludovic Courtès]

Hi Denis,

Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org> skribis:

> * gnu/packages/linux.scm (ec): New variable.

Applied with minor tweaks, thanks!

Ludo’.