Subject: Re: [bug#40925] [PATCH] Add collectd

From: Ludovic Courtès <[email-masked]>
Date: Fri, 01 May 2020 01:08:54 +0200

Hi,

Lars-Dominik Braun <[email-masked]> skribis:

> From 05110cd7f0af5f870daa48a1e32bff56efb2676a Mon Sep 17 00:00:00 2001
> From: Lars-Dominik Braun <[email-masked]>
> Date: Tue, 28 Apr 2020 10:50:10 +0200
> Subject: [PATCH] gnu: Add collectd
>
> * gnu/packages/monitoring.scm (collectd): New variable
> * gnu/local.mk (dist_patch_DATA): Add new patch
> * gnu/packages/patches/collectd-5.11.0-noinstallvar.patch: New file

[...]

> +    (arguments
> +     `(#:configure-flags (list "--localstatedir=/var" "--sysconfdir=/etc")
> +                         #:phases
> +                         (modify-phases %standard-phases
> +                           ;; Required because of patched sources.
> +                           (add-before 'configure 'autoreconf
> +                             (lambda _ (invoke "autoreconf" "-vfi") #t)))))

Another option is to patch Makefile.in to avoid the dependency on
Autoconf/Automake/Libool + this phase.

> +    (license '(licenses:expat licenses:gpl2))))

This is incorrect (see patch below, which also includes indentation
fixes and minor changes to the description to placate ‘guix lint’.)

Also, could you add a comment stating whether it’s dual-licensing or
just that there’s code in both licenses?

> diff --git a/gnu/packages/patches/collectd-5.11.0-noinstallvar.patch b/gnu/packages/patches/collectd-5.11.0-noinstallvar.patch
> new file mode 100644
> index 0000000000..a8ca142147
> --- /dev/null
> +++ b/gnu/packages/patches/collectd-5.11.0-noinstallvar.patch

A one-line comment at the top explaining what it does would be great.

Otherwise LGTM.  Could you send an updated patch?

Thanks in advance!

Ludo’.