From: Tobias Geerinckx-Rice via Guix-patches via <guix-patches@gnu.org>
To: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
Cc: 57537@debbugs.gnu.org
Subject: [bug#57537] [PATCH v2] gnu: Add ec
Date: Fri, 02 Sep 2022 06:47:39 +0200 [thread overview]
Message-ID: <5f8e53d3d302729b6bf61823b2c5139b@tobias.gr> (raw)
In-Reply-To: <20220902023239.26540-1-GNUtoo@cyberdimension.org>
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)))
next prev parent reply other threads:[~2022-09-02 4:48 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-02 2:00 [bug#57537] [PATCH] gnu: Add ec Denis 'GNUtoo' Carikli
2022-09-02 2:14 ` Denis 'GNUtoo' Carikli
2022-09-02 2:32 ` [bug#57537] [PATCH v2] " Denis 'GNUtoo' Carikli
2022-09-02 4:47 ` Tobias Geerinckx-Rice via Guix-patches via [this message]
2022-09-03 20:32 ` Maxime Devos
2022-09-06 4:29 ` Denis 'GNUtoo' Carikli
2022-09-24 13:28 ` Denis 'GNUtoo' Carikli
2022-09-26 20:42 ` bug#57537: [PATCH] " Ludovic Courtès
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5f8e53d3d302729b6bf61823b2c5139b@tobias.gr \
--to=guix-patches@gnu.org \
--cc=57537@debbugs.gnu.org \
--cc=GNUtoo@cyberdimension.org \
--cc=me@tobias.gr \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/guix.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.