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

[Tobias Geerinckx-Rice]

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