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.
View this message in rfc822 format
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Tomas Volf <~@wolfsden.cz> Cc: 75528 <at> debbugs.gnu.org Subject: [bug#75528] [PATCH 1/2] gnu: Add apcupsd. Date: Wed, 05 Feb 2025 15:59:02 +0900
Hi Tomas,
Tomas Volf <~@wolfsden.cz> writes:
> * gnu/packages/power.scm (apcupsd): New variable.
'guix lint' says:
--8<---------------cut here---------------start------------->8---
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
--8<---------------cut here---------------end--------------->8---
Please fix these.
> 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.
[...]
> +(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).
Also, we conventionally list the input fields below the arguments field
of a package, although technically it's unimportant.
> + (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'.
> + "--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.
> + (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).
> + ;; Enable additional drivers.
> + "--enable-test"
Is '--enable-test' useful? What does it do?
> + "--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.
These are only needed as native inputs anyway, right?
> + (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'.
> + ;; 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 :-).
> + (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).
> + (home-page "https://www.apcupsd.org")
I'd use 'http', since their TLS cert is now invalid.
> + (synopsis "A daemon for controlling APC UPSes")
s/A // (as hinted by 'guix lint').
> + (description "Apcupsd can be used for power mangement and controlling most
s/mangement/management/
> +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:
--8<---------------cut here---------------start------------->8---
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.
--8<---------------cut here---------------end--------------->8---
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.
Another thing found: the doc output doesn't build reproducibly, as
shown by:
--8<---------------cut here---------------start------------->8---
$ ./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'
--8<---------------cut here---------------end--------------->8---
Let's see why:
$ diff -r /gnu/store/bhwkrgkqahgs5h8crcfyacvnwp7rj390-apcupsd-3.14.14-doc{,-check}
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).
Could you please send a v2?
--
Thanks,
Maxim
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.