Package: guix-patches;
Reported by: Tomas Volf <~@wolfsden.cz>
Date: Sun, 12 Jan 2025 23:04:01 UTC
Severity: normal
Tags: patch
Done: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Bug is archived. No further changes may be made.
Message #23 received at 75528 <at> debbugs.gnu.org (full text, mbox):
From: Tomas Volf <~@wolfsden.cz> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 75528 <at> debbugs.gnu.org Subject: Re: [bug#75528] [PATCH 1/2] gnu: Add apcupsd. Date: Sun, 09 Feb 2025 17:10:51 +0100
[Message part 1 (text/plain, inline)]
Hello Maxim,
thank you for the review, replies below.
Maxim Cournoyer <maxim.cournoyer <at> gmail.com> writes:
> Hi Tomas,
>
> Tomas Volf <~@wolfsden.cz> writes:
>
>> * gnu/packages/power.scm (apcupsd): New variable.
>
> 'guix lint' says:
>
> gnu/packages/power.scm:120:14: apcupsd <at> 3.14.14: no article allowed at the beginning of the synopsis
> gnu/packages/power.scm:119:15: apcupsd <at> 3.14.14: 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.
[signature.asc (application/pgp-signature, inline)]
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.