[PATCH consfigurator v2 5/6] add gnupg scaffolding for test suite

[Sean Whitton]

Hello,

On Wed 09 Mar 2022 at 08:28pm -04, David Bremner wrote:

> This is ported from the bash function add_gpg_home in the notmuch test suite.

Looks like it is add_gnupg_home not add_gpg_home.

> diff --git a/tests/gnupg-secret-key.asc b/tests/gnupg-secret-key.asc
> new file mode 100644
> index 0000000..744361b
> --- /dev/null
> +++ b/tests/gnupg-secret-key.asc
> @@ -0,0 +1,58 @@
> +-----BEGIN PGP PRIVATE KEY BLOCK-----
> +
> +lQOYBGIl6x0BCAC9OB9xE8eO9zASZ7/J5q62IXs55ZfwHuYT2dKCAsBrWfBHG5sn

Would be good to have a file like the gnupg-secret-key.NOTE alongside
this from notmuch.  Then I'll probably regenerate the key using that
procedure, just so I know exactly what I'm committing.  Any reason you
used a much longer key btw?  Nice to keep it less than a screenful.

> diff --git a/tests/gnupg.lisp b/tests/gnupg.lisp
> new file mode 100644
> index 0000000..9ed9eed
> --- /dev/null
> +++ b/tests/gnupg.lisp
> +
> +(defvar *gnupghome* nil
> +  "Home directory for gnupg tests. Because gnupg uses Unix domain sockets
> +  internally, this path should be short enough to avoid the 108 chars limit on
> +  socket paths.")

I believe this should be DEFPARAMETER not DEFVAR.

> +
> +(defmacro with-gnupghome (&rest body)
> +  "Run BODY with GNUPGHOME to test suite value set in environment"
> +  `(let ((val nil))
> +     (setf (osicat:environment-variable "GNUPGHOME") (namestring *gnupghome*))
> +     (setq val (progn , at body))
> +     (osicat:makunbound-environment-variable "GNUPGHOME")
> +     val))

There is UIOP:SETENV imported unqualified into CONSFIGURATOR already.
Using that over OSICAT:ENVIRONMENT-VARIABLE would be more readable.  I
believe this macro should be using UNWIND-PROTECT; that would also
eliminate the LET (which otherwise needs a gensym).

Changing the environment like this affects all of the Lisp image's
threads.  If we could instead wrap gpg(1) in env(1) we could avoid that
-- is it necessary to do it using the environment?  If so, I think we
need a warning somewhere that running this part of the test suite has
the potential to break things going on in other threads.

> +(defun gpg (args &key input)
> +  "run gpg on the test suite gnupg home directory"
> +  (run-program  `("gpg" "--homedir" ,(namestring *gnupghome*) , at args)
> +                :input input
> +                :output :string :error-output :output))
> +
> +(defun gpg-setup (test-home)
> +  "Set up gnupg homedir for test suite in TEST-HOME."
> +  (setf *gnupghome* (ensure-directory-pathname #?"${test-home}/gnupg"))

Using a DEFPARAMETER for *GNUPGHOME* is right, but it should be
let-bound around running all the tests rather than setting the
thread-global value.  It's enough to create an empty binding in RUNNER
and then set it here.  Though I would be inclined just to merge (the
central part of) RUNNER, GPG-SETUP and GPG-CLEANUP into a single macro,
and using UNWIND-PROTECT to ensure the cleanup always occurs.

-- 
Sean Whitton