Subject: Re: [bug#75528] [PATCH 1/2] gnu: Add apcupsd.

From: Tomas Volf <[email-masked]>
Date: Sun, 09 Feb 2025 17:10:51 +0100

Hello Maxim,

thank you for the review, replies below.

Maxim Cournoyer <[email-masked]> writes:

> Hi Tomas,
>
> Tomas Volf <[email-masked]> writes:
>
>> * gnu/packages/power.scm (apcupsd): New variable.
>
> 'guix lint' says:
>
> gnu/packages/power.scm:120:14: [email-masked]: no article allowed at the beginning of the synopsis
> gnu/packages/power.scm:119:15: [email-masked]: TLS certificate error: X.509 server certificate for 'www.apcupsd.org' does not match: CN=sf.net
>
>
> Please fix these.

Both fixed.

>
>> Change-Id: I74f59cd1fa2a13954117ff1683a10a84576cc839
>> ---
>>  gnu/local.mk           |   1 +
>>  gnu/packages/power.scm | 125 +++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 126 insertions(+)
>>  create mode 100644 gnu/packages/power.scm
>>
>> diff --git a/gnu/local.mk b/gnu/local.mk
>> index 855f2acccc..6ca7bf68ac 100644
>> --- a/gnu/local.mk
>> +++ b/gnu/local.mk
>> @@ -557,6 +557,7 @@ GNU_SYSTEM_MODULES =				\
>>    %D%/packages/polkit.scm			\
>>    %D%/packages/popt.scm				\
>>    %D%/packages/potassco.scm				\
>> +  %D%/packages/power.scm			\
>
> This change should be mentioned in the change log as well, e.g.:
>
>  * gnu/local.mk (GNU_SYSTEM_MODULES): Register it.
>
> [...]

I used slightly different message, since the previous line mentions "new
variable", but the object being registered is whole file.

>
>> +(define-public apcupsd
>> +  (package
>> +    (name "apcupsd")
>> +    (version "3.14.14")
>> +    (source (origin
>> +              (method url-fetch)
>> +              (uri
>> +               (string-append
>> +                "mirror://sourceforge/" name "/" name " - Stable/" version
>> +                "/" name "-" version ".tar.gz"))
>> +              (sha256
>> +               (base32
>> +                "0rwqiyzlg9p0szf3x6q1ppvrw6f6dbpn2rc5z623fk3bkdalhxyv"))))
>> +    (native-inputs
>> +     (list pkg-config python-docutils))
>> +    (inputs
>> +     (list libusb libusb-compat))
>
> nitpick: If there are less than 5 and they fit on the same line, I
> prefer to list them on the same line as the field name itself.  That's
> how 'guix style' does it, although it has a tendency to protrude past
> our max column width of 80 guideline in other places (that's
> bug#62303).

Fair enough.  I do not have strong preference, so I have made it into a
single line.

I have to admit I do not run `guix style' (yes, yes, I know contributing
part of the manual mandates it), since the code produced is often subpar
compared to the hand-crafted variant.

>
> Also, we conventionally list the input fields below the arguments field
> of a package, although technically it's unimportant.

Moved.

>
>> +    (outputs '("out" "doc"))
>> +    (build-system gnu-build-system)
>> +    (arguments
>> +     (list
>> +      #:configure-flags
>> +      #~(list
>> +         ;; The configure script ignores --prefix for most of the paths.
>
> Well done with the comment.
>
>> +         (string-append "--exec-prefix=" #$output)
>> +         (string-append "--mandir=" #$output "/share/man")
>> +         (string-append "--sbindir=" #$output "/sbin")
>> +         (string-append "--sysconfdir=" #$output "/etc/apcupsd")
>> +         (string-append "--with-halpolicydir=" #$output "/share/halpolicy")
>> +
>> +         ;; Put us into the version string.
>> +         "--with-distname=GNU/Guix"
>
> The name is 'GNU Guix'.

I was afraid of the space. :)  But seems to work fine, changed.

>
>> +         "--disable-install-distdir"
>> +
>> +         ;; State directories.
>> +         "--localstatedir=/var"
>> +         "--with-log-dir=/var/log"
>> +         "--with-pid-dir=/var/run"
>> +         "--with-lock-dir=/var/run/apcupsd/lock"
>> +         "--with-nologin=/var/run/apcupsd"
>> +         "--with-pwrfail-dir=/var/run/apcupsd"
>
> I think /var/run is deprecated in favor of just /run, so we should
> configure the package to use this, I think.

It seems pretty much all programs on my system are using /var/run,
including Shepherd.  Only programs that store information in /run seem
to be elogind and dbus.

Is deprecation of /var/run listed somewhere in the manual?  I am
surprised Shepherd (since it is actively maintained and core component)
is still in /var/run.  I looked and found 42 references to /var/run, but
none with the deprecation notice.

Assuming you will insist on moving it to /run, is there some structure
to follow in that directory?  Or just s~/var/run~/run~?  What about /var
and /var/log?  Should those be moved somewhere as well?

>
>> +         (string-append "ac_cv_path_SHUTDOWN=/nope")
>> +         (string-append "ac_cv_path_APCUPSD_MAIL=/nope")
>> +         ;; While `wall' is not expanded anywhere, it still is searched for.
>> +         (string-append "ac_cv_path_WALL=/nope")
>
> I'm not sure if this package is actively developed, but that last issue
> should ideally be reported (then cross-referenced here).

I did not found a way to report a bug.  Nothing is listed in the manual,
and their apcupsd-users mailing list requires JavaScript to sign up (and
does not tell me the email address otherwise).

I did try to blindly send an email, will see if it will be accepted
without being subscribed.  If that happens, I will add the link here.

>
>> +         ;; Enable additional drivers.
>> +         "--enable-test"
>
> Is '--enable-test' useful?  What does it do?

According to the manual

--8<---------------cut here---------------start------------->8---
This turns on a test driver that is used only for debugging.
--8<---------------cut here---------------end--------------->8---

Since there does not seem to be any harm in having it on, I enabled it,
since Guix seems to aim for feature complete packages (I assume that is
the reason for everything being so large).  But I have no use for it, so
I can turn it off if you would prefer.

>
>> +         "--enable-usb"
>> +         "--enable-modbus-usb")
>> +      #:tests? #f                       ; There are no tests.
>> +      #:modules (cons '(ice-9 ftw) %default-gnu-modules)
>> +      #:phases
>> +      #~(modify-phases %standard-phases
>> +          ;; These are not installed, so trick Make into thinking they were
>> +          ;; already generated.  Allows us not to depend on mandoc, util-linux.
>> +          (add-before 'configure 'touch-txt-docs
>> +            (lambda _
>> +              (for-each (lambda (f)
>> +                          (call-with-output-file f close-port))
>> +                        '("doc/apcupsd.man.txt"
>> +                          "doc/apcaccess.man.txt"
>> +                          "doc/apctest.man.txt"
>> +                          "doc/apccontrol.man.txt"
>> +                          "doc/apcupsd.conf.man.txt"))))
>
> I think I'd rather depend on these than introduce hacks like below.

The "hacks" *below* would still be required.  The HTML manual is not
built nor installed by default, and there is no target to invoke to
install them.  So both 'build-manual and 'move-doc phases would still be
required.

We could get rid of the "hack" *above* ('touch-txt-docs), true.  It is
worth those two additional inputs?

> These are only needed as native inputs anyway, right?

Yes.

>
>> +          (add-after 'build 'build-manual
>> +            (lambda _
>> +              (invoke "make" "-C" "doc/manual" "manual.html")))
>> +          (add-after 'install-license-files 'move-doc
>> +            (lambda _
>> +              (let ((target (string-append #$output:doc
>> +                                           "/share/doc/"
>> +                                           (strip-store-file-name #$output))))
>> +                (mkdir-p target)
>> +                (for-each (lambda (f)
>> +                            (copy-file (string-append "doc/manual/" f)
>> +                                       (string-append target "/" f)))
>> +                          (scandir "doc/manual"
>> +                                   (lambda (f)
>> +                                     (or (string-suffix? ".png" f)
>> +                                         (string-suffix? ".html" f))))))))
>> +          ;; If sending mails is required, use proper mail program.
>> +          (add-after 'install 'remove-smtp
>> +            (lambda _
>> +              (delete-file (string-append #$output "/sbin/smtp"))))
>> +          ;; The configuration files and scripts are not really suitable for
>> +          ;; Guix, and our service provides its own version anyway.  So nuke
>
> I'd use the more peaceful 'remove' or 'delete' instead of 'nuke'.

Uh, sure.  Done.

>
>> +          ;; these to make sure `apcupsd' and `apctest' executed without any
>> +          ;; arguments fail.  `apctest' actually segfaults, but only after
>> +          ;; printing an error ¯\_(ツ)_/¯.
>
> Please don't embed emojis in the source :-).

Technically kaomoji (顔文字), not emoji (絵文字), but point taken. :)

>
>> +          (add-after 'install 'remove-etc-apcupsd
>> +            (lambda _
>> +              (delete-file-recursively (string-append #$output "/etc/apcupsd")))))))
>
> Break this long line so it fits under 80 columns (our code style
> guideline).

I thought (well, still do), that going 4 characters over the limit was
worth the increase in readability.  Nevertheless, I have split it into
two lines.

>
>> +    (home-page "https://www.apcupsd.org")
>
> I'd use 'http', since their TLS cert is now invalid.

Done as part of resolving the ling warning.

>
>> +    (synopsis "A daemon for controlling APC UPSes")
>
> s/A //  (as hinted by 'guix lint').

Same.

>
>> +    (description "Apcupsd can be used for power mangement and controlling most
>
> s/mangement/management/

Nice catch, fixed.

>
>> +of APC’s UPS models on Unix and Windows machines.  Apcupsd works with most of
>> +APC’s Smart-UPS models as well as most simple signalling models such a Back-UPS,
>> +and BackUPS-Office.")
>> +    (license license:gpl2)))
>
> I think it's actually license:gpl2+, according to
> apcupsd-3.14.14/COPYING, which says:
>
> Each version is given a distinguishing version number.  If the Program
> specifies a version number of this License which applies to it and "any
> later version", you have the option of following the terms and conditions
> either of that version or of any later version published by the Free
> Software Foundation.  If the Program does not specify a version number of
> this License, you may choose any version ever published by the Free Software
> Foundation.
>
>
> In this case for most file the last sentence applies, some other
> explicitly mention 'or any later version'.  That's also supported by the
> debian/copyright file.

I do not think this is correct.  When I check
apcupsd-3.14.14/src/apcupsd.c file, I see this in its header:

--8<---------------cut here---------------start------------->8---
 * Copyright (C) 1999-2005 Kern Sibbald
 *
 * This program is free software; you can redistribute it and/or
 * modify it under the terms of version 2 of the GNU General
 * Public License as published by the Free Software Foundation.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
 * General Public License for more details.
 *
 * You should have received a copy of the GNU General Public
 * License along with this program; if not, write to the Free
 * Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
 * MA 02110-1335, USA.
--8<---------------cut here---------------end--------------->8---

There is no mention of "any later version".  Does that not make the
combined work GPL-2.0-only, even if other files would be under
GPL-2.0-or-later (I did no check whether they are)?

I did not find debian/copyright file in the source code.

>
> Another thing found:  the doc output doesn't build reproducibly, as
> shown by:
>
> $ ./pre-inst-env guix build --no-grafts --check -K
> guix build: error: derivation `/gnu/store/r62j72bd3an8k2fbmaiil5hma32syxdy-apcupsd-3.14.14.drv' may not be deterministic: output `/gnu/store/bhwkrgkqahgs5h8crcfyacvnwp7rj390-apcupsd-3.14.14-doc' differs from `/gnu/store/bhwkrgkqahgs5h8crcfyacvnwp7rj390-apcupsd-3.14.14-doc-check'
>
> Let's see why:
>
> $ diff -r /gnu/store/bhwkrgkqahgs5h8crcfyacvnwp7rj390-apcupsd-3.14.14-doc{,-check}

This is great, I did not know that it names the store item -check.  Very
useful, thank you.

> diff -r
> /gnu/store/bhwkrgkqahgs5h8crcfyacvnwp7rj390-apcupsd-3.14.14-doc/share/doc/apcupsd-3.14.14/manual.html
> /gnu/store/bhwkrgkqahgs5h8crcfyacvnwp7rj390-apcupsd-3.14.14-doc-check/share/doc/apcupsd-3.14.14/manual.html
> 376c376
> < <div class="line">February  5, 2025 06:54:22</div>
> ---
>> <div class="line">February  5, 2025 06:54:50</div>
>
> Ah, the classic date time stamp.  You'll want to neuter it in the source
> or in the built html file (with a preference for the former).

Nice find, I have missed this.  I have patched the source code of the
manual to not embed the date and time, seems to work now.

>
> Could you please send a v2?

Definitely, I will go through your review for the service file as well,
and once I have all the answers (both here and there), will send v2.

Once more thanks for the review,
Tomas

-- 
There are only two hard things in Computer Science:
cache invalidation, naming things and off-by-one errors.