GNU bug report logs - #66193
[PATCH] gnu: Add keyd.

Previous Next

Full log

View this message in rfc822 format

From: Jack Hill <jackhill <at> jackhill.us>
To: ShinyZero0 <shinyzero0 <at> tilde.club>
Cc: 66193 <at> debbugs.gnu.org
Subject: [bug#66193] [PATCH] gnu: Add keyd.
Date: Mon, 25 Sep 2023 14:59:53 -0400 (EDT)

On Mon, 25 Sep 2023, ShinyZero0 wrote:

> From 01e89ff48c77e12b7a8f206098901e08bc979935 Mon Sep 17 00:00:00 2001
> From: "zero <at> fedora" <shinyzero0 <at> tilde.club>
> Date: Mon, 25 Sep 2023 19:39:00 +0300
> Subject: [PATCH] gnu: Add keyd
>
> * gnu/packages/keyd.scm: New file.

Thanks for your submission. keyd looks like a neat package that I did not 
know about before!

I'll leave some comments below about some things that I think can be 
improved. Can you look into them and send a second version?

> ---
> gnu/packages/keyd.scm | 43 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 43 insertions(+)
> create mode 100644 gnu/packages/keyd.scm

When adding a new file, it should also be added to the list of files in 
gnu/local.mk. Also, the "This file is party of GNU Guix" header should be 
added to the beginning of this file (it can be copied from another one). 
You can also add a copyright line for yourself!

Alternatively, you could put keyd in an existing file. linux.scm might be 
a good one. This is a little bit of a judgment call, and I'm not 
experienced enough to know what's best. Maybe someone else will chime in 
here, or you could ask for advice on IRC.

> diff --git a/gnu/packages/keyd.scm b/gnu/packages/keyd.scm
> new file mode 100644
> index 0000000000..48afd9f877
> --- /dev/null
> +++ b/gnu/packages/keyd.scm
> @@ -0,0 +1,43 @@
> +(define-module (gnu packages keyd))
> +(use-modules
> +  (guix packages)
> +  (gnu packages linux)
> +  (guix git-download)
> +  ((guix licenses) #:prefix license:)
> +  (guix build-system gnu))
> +(define-public keyd
> +  (package
> +    (name "keyd")
> +    (version "2.4.3")
> +    (source
> +      (origin
> +        (method git-fetch)
> +        (uri (git-reference
> +               (url "https://github.com/rvaiya/keyd")
> +               (commit (string-append "v" version))))
> +        (sha256
> +          (base32
> +            "1awdp863amq95y990fi4wj389ssv3ip2daqz2ph23lsahwa6f5in"))))
> +    (arguments
> +      (list #:tests? ; no tests
> +            #f

I looks to me like there are tests in the t directory. Can they be 
enabled?

https://github.com/rvaiya/keyd/blob/5e4ef41b41ce02f7d6a9f2e51298810d84589e76/Makefile#L87-L91

Also, a little bit of a nitpick, but I think it would be easier to read if 
#tests? and #f were on the same line.

> +            #:make-flags
> +            '(list "CC=gcc"

Unfortunately, there's a subtle bug here. Most of the time this will work, 
but when cross compiling, it will pick the wrong GCC (oops!). Fortunately 
we have a cc-for-target procedure that will pick the right one. Here's an 
example of it in use:

https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/packages/linux.scm?id=fafd3caef0d51811a5da81d6061789e2908b0dac#n1591

> +                   "PREFIX="
> +                   (string-append
> +                     "DESTDIR="
> +                     (assoc-ref %outputs "out")))

This can probably be upgraded to the new style of using gexps to find the 
output. The above example package does that. See also this blog post for 
the long explanation:

https://guix.gnu.org/en/blog/2021/the-big-change/

> +            #:phases
> +            '(modify-phases
> +               %standard-phases
> +               (delete 'configure)))) ; no autoconf
> +    (build-system gnu-build-system)
> +    (inputs (list linux-libre-headers))
> +    (synopsis "A key remapping daemon for linux.")
> +    (description
> +      "Keyd is a keyboard remapping utility with intuitive ini
> +      config file format. Keyd has several unique features, many of
> +      which are traditionally only found in custom keyboard firmware
> +      like QMK")

Another nitpick: intuitive and unique sound like marketing words to me and 
can probably be left out. However that's just my taste.

> +    (home-page "https://github.com/rvaiya/keyd")
> +    (license license:expat)))

That's all I have for know. Thanks again for your contribution, and 
hopefully you found this review helpful.

All the best,
Jack

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.