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

[Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>]

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