[bug#77826] [PATCH] home: home-gpg-agent-service: add new parameter 'use-keyboxd?'.

[Ludovic Courtès]

Hi Sébastien,

Sébastien Farge <sebastien-farge@laposte.net> writes:

> * gnu/home/services/gnupg.scm: New parameter.
> * doc/guix.texi (GNU Privacy Guard): New description.
> * gnu/tests/gnupg.scm: Alice use keyboxd, Bob normal keyring, test if both 
> works
>
> Change-Id: I27b4f686086b9740943dbb5347a14ada245cc9fb

Nice!

Overall LGTM.  Some comments below.

Please add the new file to ‘gnu/local.mk’ next to its friends.

> +@item @code{use-keyboxd?} (default: @code{#f}) (type: boolean)
> +Whether to enable keyboxd and its keybox database instead of usual keyring. 
> When true,
> +@command{gpg-agent} call @command{keyboxd} who take care of keys management 
> process and database. 

“@command{gpg-agent} spawns a separate @command{keyboxd} process, which
is responsible for managing the key database.”

Nitpick: Please leave two spaces after end-of-sentence periods.

It’s the first time I hear about keyboxd and the gnupg manual doesn’t
say much about it.  When would you set it to #true?

> +(define (home-gpg-common-configuration-file config)
> +  "Return the @file{common.conf} file for @var{config}."
> +  (match-record config <home-gpg-agent-configuration>
> +    (use-keyboxd?)
> +    (mixed-text-file "common.conf" "use-keyboxd\n")))

You can remove ‘match-record’ altogether.

> +++ b/gnu/tests/gnupg.scm
> @@ -0,0 +1,246 @@
> +;;; GNU Guix --- Functional package management for GNU
> +;;; Copyright © 2016-2022, 2024 Ludovic Courtès <ludo@gnu.org>
> +;;; Copyright © 2017, 2018 Clément Lassieur <clement@lassieur.org>
> +;;; Copyright © 2017 Marius Bakke <mbakke@fastmail.com>

I think this is inaccurate.  :-)

Very nice that you wrote tests for this!

> +              (service home-gpg-agent-service-type
> +                       (home-gpg-agent-configuration
> +                        (default-cache-ttl 820))))
> +             %base-home-services))
> +    ))

No lonely parens please (throughout this file.)

> +(define %gnupg-os
> +  (operating-system
> +    (inherit (simple-operating-system (service guix-home-service-type 
> `(("alice" ,%keyboxd-home)
> +                                                                        
> ("bob" ,%keyring-home)))))
> +

Please insert a newline after ‘simple-operating-system’.

> +          (define (file-get-all-strings fname)

s/file-get-all-strings/file-contents/ maybe?

And s/fname/file/ (this is what’s usually done).

> +          (define (vm-type cmd-or-list)
> +            (let ((cmd-list (if (list? cmd-or-list) cmd-or-list (list 
> cmd-or-list))))

Avoid polymorphic procedures; have it take either a list of a string.

> +(define %test-gnupg-keyboxd
> +  (system-test
> +   (name "gnupg-keyboxd")
> +   (description "Test gnupg using keyboxd or keyring.")

s/gnupg/GnuPG/

“using both keyboxd and a local keyring” maybe?

Could you send an updated patch?

Thanks!

Ludo’.