Re: [PATCH v2] gnu: curl: Update to 7.41.0. Fix #20121.

[Mark H Weaver]

Tomáš Čech <address@hidden> writes:

> * gnu/packages/patches/curl-gss-api-fix.patch: Delete file.
> * gnu/packages/patches/curl-enable_capath-conf.patch: New file.
> * gnu/packages/patches/curl-enable_capath.patch: New file.

Why the mixture of dashes and underscores in the patch name?
Normally we use dashes.  How about calling them:

  curl-support-capath-on-gnutls.patch
  curl-support-capath-on-gnutls-conf.patch

> * gnu-system.am (dist_patch_DATA): Add new patches, remove old one.
> * gnu/packages/curl.scm (curl): Update to 7.41.0. Remove old patch, add two
>   new ones. Disable one unit test.

Please put two spaces between sentences.  Also, instead of writing
"Fix #20121" in the summary line, which will mean nothing to someone who
doesn't remember that bug by its number, we prefer to summarize the
actual changes made.  When fixing bugs, we include the short URL to the
bug report on its own line.

So, how about something like this:

--8<---------------cut here---------------start------------->8---
gnu: curl: Update to 7.41.0.  Support CURLOPT_CAPATH on GnuTLS.

Fixes <http://bugs.gnu.org/20121>.

* gnu/packages/patches/curl-gss-api-fix.patch: Delete file.
* gnu/packages/patches/curl-enable_capath-conf.patch: New file.
* gnu/packages/patches/curl-enable_capath.patch: New file.
* gnu-system.am (dist_patch_DATA): Add new patches, remove old one.
* gnu/packages/curl.scm (curl): Update to 7.41.0.  Remove old patch, add
  two new ones.  Disable one unit test.
--8<---------------cut here---------------end--------------->8---

> diff --git a/gnu/packages/curl.scm b/gnu/packages/curl.scm
> index 821a957..f466dcc 100644
> --- a/gnu/packages/curl.scm
> +++ b/gnu/packages/curl.scm

Please add your copyright line to this file.

> @@ -37,15 +37,21 @@
>  (define-public curl
>    (package
>     (name "curl")
> -   (version "7.40.0")
> +   (version "7.41.0")
>     (source (origin
>              (method url-fetch)
>              (uri (string-append "http://curl.haxx.se/download/curl-";
>                                  version ".tar.lzma"))
>              (sha256
>               (base32
> -              "1a15fdc26b3vwwmchzzpd3l1hfyhx06dn7b6lkikqd7kgwvg5ps7"))
> -            (patches (list (search-patch "curl-gss-api-fix.patch")))))
> +              "08n7vrhdfzziy3a7n93r7qjhzk8p26q464hxg8w9irdk3v60pi62"))
> +            ;; This is backport of patch which fixes handling of both
> +            ;; --with-ca-path and --without-ca-path for curl built against
> +            ;; GnuTLS. First patch is identical to upstream, second one 
> changes
> +            ;; configure script accordingly without need of reconfigure.
> +            ;; Fixes #20121.

This comment talks about enabling a feature that we don't use, namely
the --with-ca-path configure flag.  The important aspect of the patch is
that it adds support for CURLOPT_CAPATH in the GnuTLS backend.

Anyway, I think it's best to remove this entire comment.  The
description of the patch belongs in the patch itself, and needn't be
reproduced here.

> +            (patches (list (search-patch "curl-enable_capath.patch")
> +                           (search-patch "curl-enable_capath-conf.patch")))))
>     (build-system gnu-build-system)
>     (inputs `(("gnutls" ,gnutls)
>               ("gss" ,gss)
> @@ -68,7 +74,10 @@
>         (lambda _
>           (substitute* "tests/runtests.pl"
>             (("/bin/sh") (which "sh")))
> -
> +         ;; Test #1135 requires extern-scan.pl, which is not part of the
> +         ;; tarball due to mistake. It was fixed already in upstream. We can
> +         ;; simply ignore the test as it aims VMS and OS/400.
> +         (delete-file "tests/data/test1135")

Two spaces between sentences please.
s/mistake/a mistake/
s/It was fixed already in upstream/It has been fixed upstream/
s/ignore/disable/
s/as it aims/as it is specific to/

Please add a blank line after the 'delete-file' call.

> diff --git a/gnu/packages/patches/curl-enable_capath-conf.patch 
> b/gnu/packages/patches/curl-enable_capath-conf.patch
> new file mode 100644
> index 0000000..6d4ba8e
> --- /dev/null
> +++ b/gnu/packages/patches/curl-enable_capath-conf.patch
> @@ -0,0 +1,16 @@
> +Following patch allows --with-ca-path for curl built against GnuTLS even
> +without need of reconfigure.
> +

How about this instead:

  This patch updates 'configure' as autoreconf would have done after
  applying curl-support-capath-on-gnutls.patch.

> +--- a/configure       2015-03-22 01:11:23.178743705 +0100
> ++++ b/configure       2015-02-25 00:05:37.000000000 +0100
> +@@ -23952,8 +24432,8 @@
> +         ca="$want_ca"
> +     capath="no"
> +   elif test "x$want_capath" != "xno" -a "x$want_capath" != "xunset"; then
> +-        if test "x$OPENSSL_ENABLED" != "x1" -a "x$POLARSSL_ENABLED" != 
> "x1"; then
> +-      as_fn_error $? "--with-ca-path only works with openSSL or PolarSSL" 
> "$LINENO" 5
> ++        if test "x$OPENSSL_ENABLED" != "x1" -a "x$GNUTLS_ENABLED" != "x1" 
> -a "x$POLARSSL_ENABLED" != "x1"; then
> ++      as_fn_error $? "--with-ca-path only works with OpenSSL, GnuTLS or 
> PolarSSL" "$LINENO" 5
> +     fi
> +     capath="$want_capath"
> +     ca="no"
> diff --git a/gnu/packages/patches/curl-enable_capath.patch 
> b/gnu/packages/patches/curl-enable_capath.patch
> new file mode 100644
> index 0000000..0094a1b
> --- /dev/null
> +++ b/gnu/packages/patches/curl-enable_capath.patch
> @@ -0,0 +1,103 @@
> +Following patch allows to use --with-ca-path for curl built against GnuTLS.
> +
> +

How about this instead:

  This patch adds support for CURLOPT_CAPATH in the GnuTLS backend.

Can you send an updated patch?

     Thanks!
       Mark