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

Previous Next

Full log

Message #35 received at 77826 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: sebastien-gp <at> laposte.net
Cc: 77826 <at> debbugs.gnu.org
Subject: Re: bug#77826: [PATCH] home: home-gpg-agent-service: add new
 parameter 'use-keyboxd?'.
Date: Mon, 12 May 2025 10:33:13 +0200

Hi Sébastien,

sebastien-gp <at> laposte.net 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 <ludo <at> gnu.org>

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’.

Previous Next

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