Subject: Re: [bug#42404] [PATCH 3/5] gnu: Add rnp.

From: Ludovic Courtès <[email-masked]>
Date: Fri, 24 Jul 2020 12:27:45 +0200

Hi Justus,

Justus Winter <[email-masked]> skribis:

> * gnu/packages/openpgp.scm (rnp): New variable.
> * gnu/packages/patches/rnp-disable-ruby-rnp-tests.patch: New file.
> * gnu/packages/patches/rnp-fix-cp.patch: New file.
> * gnu/packages/patches/rnp-fix-gnupg-list-packets.patch: New file.
> * gnu/packages/patches/rnp-fix-test-setup.patch: New file.
> * gnu/packages/patches/rnp-fix-test.patch: New file.
> * gnu/packages/patches/rnp-fix-true-false.patch: New file.
> * gnu/packages/patches/rnp-unbundle-googletest.patch: New file.
> * gnu/packages/patches/rnp-update-expiration-16ecb289.patch: New file.

Could you add the patches to gnu/local.mk?

> +    (home-page "https://www.rnpgp.com/")
> +    (license (list license:bsd-2 license:asl2.0 license:bsd-3))))

Could you add a comment (a couple of lines) stating whether it’s triple
licensing or rather that several parts come under different licenses?

The patch overall LGTM but I’m concerned by the relatively large number
of patches and the fact that their upstream status is unknown.  Seems to
me that many of them ought to be upstream no?  If they are upstream, can
we instead either wait for a release that includes them, or take a
snapshot that includes them?

> diff --git a/gnu/packages/patches/rnp-disable-ruby-rnp-tests.patch b/gnu/packages/patches/rnp-disable-ruby-rnp-tests.patch
> new file mode 100644
> index 0000000000..5c8c06524d
> --- /dev/null
> +++ b/gnu/packages/patches/rnp-disable-ruby-rnp-tests.patch
> @@ -0,0 +1,25 @@
> +From 9f3c07601393e219cc5979f93fda57bf2d07dee7 Mon Sep 17 00:00:00 2001
> +From: Justus Winter <[email-masked]>
> +Date: Tue, 21 Jul 2020 16:10:21 +0200
> +Subject: [PATCH 6/6] Disable ruby-rnp tests.

What’s the rationale?  Would #:tests? #f have the same effect?

> diff --git a/gnu/packages/patches/rnp-fix-cp.patch b/gnu/packages/patches/rnp-fix-cp.patch
> new file mode 100644
> index 0000000000..039912d953
> --- /dev/null
> +++ b/gnu/packages/patches/rnp-fix-cp.patch
> @@ -0,0 +1,27 @@
> +From c163e1b12511e9e7df752a01767a2a8ba56c4196 Mon Sep 17 00:00:00 2001
> +From: Justus Winter <[email-masked]>
> +Date: Tue, 21 Jul 2020 15:52:37 +0200
> +Subject: [PATCH 1/6] Make copying more robust.
> +
> +Let the shell locate 'cp'.  This is more robust in environments such
> +as Guix or Nix that do not provide /bin/cp.
> +---
> + src/tests/support.cpp | 2 +-
> + 1 file changed, 1 insertion(+), 1 deletion(-)
> +
> +diff --git a/src/tests/support.cpp b/src/tests/support.cpp
> +index 3d6a6dc9..d260e166 100644
> +--- a/src/tests/support.cpp
> ++++ b/src/tests/support.cpp
> +@@ -283,7 +283,7 @@ copy_recursively(const char *src, const char *dst)
> +     // TODO: maybe use fts or something less hacky
> +     char buf[2048];
> + #ifndef _WIN32
> +-    snprintf(buf, sizeof(buf), "/bin/cp -a '%s' '%s'", src, dst);
> ++    snprintf(buf, sizeof(buf), "cp -a '%s' '%s'", src, dst);

I’d rather add a build phase that replaces /bin/cp with (which "cp") or
similar.  That way, the package would be self-contained (no need to
manually add Coreutils on $PATH).

> +++ b/gnu/packages/patches/rnp-fix-true-false.patch
> @@ -0,0 +1,253 @@
> +From 028a2f50fbf47d989bbf79be589945bec55b4825 Mon Sep 17 00:00:00 2001
> +From: Justus Winter <[email-masked]>
> +Date: Tue, 21 Jul 2020 15:57:57 +0200
> +Subject: [PATCH 3/6] Use 'true' and 'false' instead of 'TRUE' and 'FALSE'.
> +
> +The latter are not guaranteed to be defined.

This should probably be upstream, but if we have to have it, how about
making this change with ‘substitute*’ instead?

> +++ b/gnu/packages/patches/rnp-update-expiration-16ecb289.patch
> @@ -0,0 +1,208 @@
> +commit 16ecb28974b18e51f9060c86c7229f3b7e1cbb88
> +Author: Nickolay Olshevsky <[email-masked]>
> +Date:   Fri Apr 10 16:29:52 2020 +0300
> +
> +    Update expiration date of test keys.

This is bound to expire again.  :-)

How about using faketime instead, as is done for the ‘nss’ package?

Thanks!

Ludo’.