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

From: Ludovic Courtès <[email-masked]>
Date: Mon, 12 May 2025 10:33:13 +0200

Hi Sébastien,

[email-masked] writes:

> * gnu/home/services/gnupg.scm: New parameter.
> * doc/guix.texi (GNU Privacy Guard): New description.
> * gnu/tests/gnupg.scm: four scenarii,
>   		       1) use-keyboxd? true, no keyring
> 		       2) use-keyboxd? unset, no keyring
> 		       3) use-keyboxd? false, legacy pubring.gpg
> 		       4) use-keyboxd? true, legacy pubring.gpg

Nice.  Some comments below.

> +@item @code{use-keyboxd?} (default: @code{#f}) (type: boolean)
> +Choose true if you want to use the new keys database daemon
> +managed by @command{keyboxd} ---as it is by default on a fresh
                               ^
extra space here.

s/as it is by default/the default setting/

> +install since GnuPG 2.4.1--- instead of keyring file(s).
                               ^
extra space

> +The @file{~/.gnupg/common.conf} is created with parameter
> +@code{use-keyboxd} set for the switch to happen
> +(@pxref{GPG Configuration,,, gnupg, Using the GNU Privacy Guard}).  
> +Caution: keys kept in a previous pubring file has to be imported in

Please insert a newline before “Caution.”

Also, you might want to enclose the warning like this:

  @quotation Warning
  Keys kept in a previous pubring file…
  @end quotation

s/has to be imported/have to be imported/ ?

> +the keyboxd database or will be ignored (For more informations

s/For more informations/for more information/ (singular)

> +please refer to the GnuPG README file at section `Keys database daemon`). 

“please refer to the ``Keys database daemon'' section of GnuPG's
@file{README} file”

> +++ b/gnu/tests/gnupg.scm
> @@ -0,0 +1,432 @@
> +

Extra newline here.  :-)

Please add the file to gnu/local.mk.

> +;;; GNU Guix --- Functional package management for GNU
> +;;; Copyright © 2016-2022, 2024 Ludovic Courtès <[email-masked]>

You can drop this line.

> +;;; A FAIRE
> +;;; Déplacer le fichier de charles dans le home de dorothee en changeant owner et groupe

Leftover comment?

> +(define* (run-gnupg-keyboxd-test)
> +  "Run an OS to test four situations related to 'use-keyboxd?' option :
> +- Alice : 'use-keyboxd?' true, and has no keyring yet.
> +- Bob  : 'use-keyboxd?' unset, and has no keyring.
> +- Charles 'use-keyboxd?' false, has a legacy keyring.gpg
> +- Dorothee 'use-keyboxd?' true, has a legacy keyring.gpg."

No space before colon (unlike in French :-)).

> +          ;; The rest of the tests can be done without user.
> +          (test-assert "Alice : .gnupg dir is created"
> +            (marionette-eval
> +             `(file-exists? "/home/alice/.gnupg")
> +             marionette))
> +          
> +          (test-equal "Alice : gpg-agent.conf exists and is a symlink"
> +            'symlink
> +            (marionette-eval
> +             `(and (file-exists? "/home/alice/.gnupg/gpg-agent.conf")
> +                   (stat:type (lstat "/home/alice/.gnupg/gpg-agent.conf")))
> +             marionette))
> +
> +          (test-equal "Alice : common.conf exists, is a symlink, and contains 'use-keyboxd'"
> +            '(#t symlink "use-keyboxd")
> +            (marionette-eval
> +             `(begin
> +                (use-modules (ice-9 rdelim))
> +                (list (file-exists? "/home/alice/.gnupg/common.conf")
> +                      (stat:type (lstat "/home/alice/.gnupg/common.conf"))
> +                      (call-with-input-file "/home/alice/.gnupg/common.conf" read-line)))
> +             marionette))

I would be tempted to drop these three tests (also where duplicated
below for Dorothée and Charles and Bob) because they just mirror the
code, and thus that’s a lot of line for a very low “bug-finding
performance”.

> +          (test-equal "kill 'gpg-agent', and 'keyboxd'"
> +            '(0 0) 
> +            (marionette-eval
> +             `(list
> +               (status:exit-val
> +                (system* #$(file-append procps "/bin/pkill") "gpg-agent"))
> +               (status:exit-val
> +                (system* #$(file-append procps "/bin/pkill") "keyboxd")))
> +             marionette))

You can omit ‘status:exit-val’ calls.

> +          ;; Close user session.
> +          (marionette-type "exit\n" marionette)
> +          (sleep 1)

‘sleep’?  Can this be removed?

Could you send an updated patch?

Thanks for coming up with nice tests!

Ludo’.