Nicolò Balzarotti <anothersms@gmail.com> writes:

> Hi guix!
> This patch adds Network UPS Tools (nut).

Cool!

[...]

>>From 0e3a16eb19bc481d38edbe36c241ec4631cbdd33 Mon Sep 17 00:00:00 2001
> From: nixo <nicolo@nixo.xyz>
> Date: Tue, 27 Oct 2020 10:56:35 +0100
> Subject: [PATCH] gnu: Add nut.
>
> * gnu/packages/admin.scm (nut): New variable.

[...]
  
> +(define-public nut
> +  (package
> +    (name "nut")
> +    (version "2.7.4")
> +    (source
> +     (origin
> +       (method url-fetch)
> +       (uri "https://networkupstools.org/source/2.7/nut-2.7.4.tar.gz")

Avoid hard-coding versions in URLs.  You can use the version-major+minor
procedure to create that "2.7" URL component.

> +       (patches
> +        (list
> +         ;; OpenSSL 1.1 support
> +         (origin
> +           (method url-fetch)
> +           (uri (string-append "https://patch-diff.githubusercontent.com/raw/"
> +                               "networkupstools/nut/pull/504.patch"))
> +           (sha256
> +            (base32 "1dabbzlmb886qy39xjx6nxlfrzfkm99n48la1ip32zm3baal6qph"))
> +           (file-name (string-append name "-openssl1.1")))))))

This got merged in 2017, and still not released!

Looking at the pull request[0], this URL is a squashed version of 7
commits.  While convenient, this trick has failed before due to subtle
changes in GitHub machinery, eventually leading to a hash mismatch[1].

Taking 7 patches directly is no fun either, so I wonder if we can just
stick with OpenSSL 1.0 for now?

[0] https://github.com/networkupstools/nut/pull/504
[1] commit 7670efefe4fb4aca12cb19ea5d89ff37c48e3ea6

> +    (build-system gnu-build-system)
> +    (arguments
> +     `(#:configure-flags '("--with-all"
> +                           "--with-openssl"
> +                           ;; powerman is required (where is it hosted?)
> +                           "--without-powerman"

Can you elaborate on powerman being required?  Is the package not usable
without it?

> +                           ;; cgi requires libgd
> +                           "--without-cgi")

We do have libgd (in the 'gd' package).

> +       #:phases
> +       (modify-phases %standard-phases
> +         (add-before 'configure 'update-configure
> +           ;; required to apply the patch to the ./configure script
> +           (lambda _
> +             (invoke "autoconf"))))))

This may not be necessary due to the 'bootstrap' phase (but I haven't
verified).

> +    (native-inputs `(("autoconf" ,autoconf)
> +                     ("cppunit" ,cppunit)
> +                     ("pkg-config" ,pkg-config)))
> +    (inputs `(("avahi" ,avahi)
> +              ("libusb" ,libusb-compat)
> +              ("libltdl" ,libltdl)
> +              ("freeipmi" ,freeipmi)
> +              ("neon" ,neon)
> +              ("openssl" ,openssl)
> +              ("net-snmp" ,net-snmp)))

The indentation of these are a bit weird.  :-)

> +    (home-page "https://networkupstools.org")
> +    (license
> +     (list
> +      ;; - most files under gpl2+
> +      license:gpl2+
> +      ;; - scripts/python/ under gpl3+
> +      license:gpl3+
> +      ;; - scripts/perl/Nut.pm same as perl (either gpl1+ or artistic)
> +      license:gpl1+ license:artistic2.0))

Great job at listing licenses!

> +    (synopsis "Collection of programs for monitoring and administering UPS")
> +    (description "Network UPS Tools is a collection of programs which provide
> +a common interface for monitoring and administering UPS, PDU and SCD hardware.
> +It uses a layered approach to connect all of the parts.  Drivers are provided
> +for a wide assortment of equipment.")))

Maybe add @acronym{UPS, Uninterruptible Power Supply},
@acronym{PDU, ...} etc, but no strong opinion.

Can you send an updated patch?