[PATCH 0/2] Add apcupsd

> * gnu/packages/power.scm (apcupsd): New variable.

> 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 \

> +(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))

> + (outputs '("out" "doc"))

> + (build-system gnu-build-system)

> + (arguments

> + (list

> + #:configure-flags

> + #~(list

> + ;; The configure script ignores --prefix for most of the paths.

> + (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"

> + "--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"

> + (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")

> + ;; Enable additional drivers.

> + "--enable-test"

> + "--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"))))

> + (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

> + ;; these to make sure `apcupsd' and `apctest' executed without any

> + ;; arguments fail. `apctest' actually segfaults, but only after

> + ;; printing an error ¯\_(ツ)_/¯.

> + (add-after 'install 'remove-etc-apcupsd

> + (lambda _

> + (delete-file-recursively (string-append #$output "/etc/apcupsd")))))))

> + (home-page "https://www.apcupsd.org")

> + (synopsis "A daemon for controlling APC UPSes")

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

> +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)))

> <div class="line">February 5, 2025 06:54:50</div>

> --- /dev/null

> +++ b/gnu/packages/power.scm

> @@ -0,0 +1,125 @@

> +;;; Copyright (C) 2025 Tomas Volf <~@wolfsden.cz>

> +;;; Copyright (C) 2023 Raven Hallsby <karl@hallsby.com>

> * gnu/services/power.scm: New file.

> +The @code{(gnu services power)} module provides a service definition for

> +@uref{http://www.apcupsd.org/, apcupsd}, a utility to interact with APC

> +UPSes. Apcupsd also works with some OEM-branded products manufactured

> +by APC.

> +@defvar apcupsd-service-type

> +The service type for apcupsd. For USB UPSes no configuration is

> +necessary, however tweaking some fields to better suit your needs might

> +be desirable. The defaults are taken from the upstream configuration

> +and they are not very conservative (@code{battery-level} of 5% is too

> +low in my opinion).

> +The default event handlers do send emails, read more in

> +@ref{apcupsd-event-handlers}.

> +

> +@lisp

> +(service apcupsd-service-type)

> +@end lisp

> +@end defvar

> +

> +@deftp {Data Type} apcupsd-configuration

> +

> +Available @code{apcupsd-configuration} fields are:

> +

> +@table @asis

> +@item @code{package} (default: @code{apcupsd}) (type: package)

> +Package to use.

> +@item @code{shepherd-base-name} (default: @code{apcupsd}) (type: symbol)

> +Base name of the shepherd service. You can add the service multiple

> +times with different prefix to manage multiple UPSes.

> +

> +@item @code{auto-start?} (default: @code{#t}) (type: boolean)

> +Should the shepherd service auto-start?

> +

> +@item @code{pid-file} (default: @code{"/var/run/apcupsd.pid"}) (type: string)

> +Path to the pid file.

> +

> +@item @code{debug-level} (default: @code{0}) (type: integer)

> +Logging verbosity. Bigger number means more logs. In the source code I

> +saw up to @code{300}, so for all logs, @code{999} seems reasonable.

> +

> +@item @code{run-dir} (default: @code{"/var/run/apcupsd"}) (type: string)

> +Directory containing runtime information. You need to change this if

> +you desire to run multiple instances of the daemon.

> +

> +@item @code{name} (type: maybe-string)

> +Use this to give your UPS a name in log files and such. This is

> +particularly useful if you have multiple UPSes. This does not set the

> +EEPROM. It should be 8 characters or less.

> +

> +@item @code{cable} (default: @code{usb}) (type: enum-cable)

> +The type of cable connecting the UPS to your computer. Possible generic

> +choices are @code{'simple}, @code{'smart}, @code{'ether} and

> +@code{'usb}. Or a specific cable model number may be used:

> +@item usb

> +A null string setting enables autodetection, which is the best choice

> +for most installations.

> +@anchor{apcupsd-event-handlers}

> +@deftp {Data Type} apcupsd-event-handlers

> +

> +For description of the events please refer to the @command{apcupsd}'s

> +manual, which can be found in the @samp{apcupsd-doc} package.

> +

> +Each handler shall be a gexp. It is spliced into the control script for

> +the daemon. In addition to the standard Guile programming environment,

> +these procedures and variables are also available.

> +

> +@table @code

> +@item conf

> +Variable containing path to the configuration file.

> +

> +@item powerfail-file

> +Variable containing path to the powerfail file.

> +@item cmd

> +The event currently being handled.

> +

> +@item name

> +The name of the UPS as specified in the configuration file.

> +

> +@item connected

> +Is @code{"1"} if apcupsd is connected to the UPS via a serial port (or a

> +USB port). In most configurations, this will be the case. In the case

> +of a Slave machine where apcupsd is not directly connected to the UPS,

> +this value will be @code{"0"}.

> +

> +@item powered

> +Is @code{"1"} if the computer on which @command{apcupsd} is running is

> +powered by the UPS and @code{"0"} if not. At the moment, this value is

> +unimplemented and always @code{"0"}.

> +

> +@item (err @var{fmt} @var{args...})

> +Wrapper around @code{format} outputting to @code{(current-error-port)}.

> +

> +@item (wall @var{fmt} @var{args...})

> +Wrapper around @code{format} outputting via @command{wall}.

> +

> +@item (apcupsd @var{args...})

> +Call @command{apcupsd} while passing the correct configuration file and

> +all the arguments.

> +

> +@item (mail-to-root @var{subject} @var{body})

> +Send an email to the local administrator. This procedure assumes the

> +@command{sendmail} is located at @command{/run/privileged/bin/sendmail}

> +(as would be the case with @code{opensmtpd-service-type}).

> +

> +@end table

> +

> +Available @code{apcupsd-event-handlers} fields are:

> +

> +@table @asis

> +@item @code{killpower} (type: gexp)

> +Handler for killpower event.

> +

> +@item @code{commfailure} (type: gexp)

> +Handler for commfailure event.

> +

> +@item @code{commok} (type: gexp)

> +Handler for commfailure event.

> +

> +@item @code{powerout} (type: gexp)

> +Handler for powerout event.

> +

> +@item @code{onbattery} (type: gexp)

> +Handler for onbattery event.

> +

> +@item @code{offbattery} (type: gexp)

> +Handler for offbattery event.

> +

> +@item @code{mainsback} (type: gexp)

> +Handler for mainsback event.

> +

> +@item @code{failing} (type: gexp)

> +Handler for failing event.

> +

> +@item @code{timeout} (type: gexp)

> +Handler for timeout event.

> +

> +@item @code{loadlimit} (type: gexp)

> +Handler for loadlimit event.

> +

> +@item @code{runlimit} (type: gexp)

> +Handler for runlimit event.

> +

> +@item @code{doreboot} (type: gexp)

> +Handler for doreboot event.

> +

> +@item @code{doshutdown} (type: gexp)

> +Handler for doshutdown event.

> +

> +@item @code{annoyme} (type: gexp)

> +Handler for annoyme event.

> +

> +@item @code{emergency} (type: gexp)

> +Handler for emergency event.

> +

> +@item @code{changeme} (type: gexp)

> +Handler for changeme event.

> +

> +@item @code{remotedown} (type: gexp)

> +Handler for remotedown event.

> +

> +@item @code{startselftest} (type: gexp)

> +Handler for startselftest event.

> +

> +@item @code{endselftest} (type: gexp)

> +Handler for endselftest event.

> +

> +@item @code{battdetach} (type: gexp)

> +Handler for battdetach event.

> +

> +@item @code{battattach} (type: gexp)

> +Handler for battattach event.

> +

> +@end table

> +

> +@end deftp

> +;;; Copyright (C) 2025 Tomas Volf <~@wolfsden.cz>

> +

> +;;;; Commentary:

> +

> +;;; Power-related services.

> +

> +;;;; Code:

> +

> +(define-module (gnu services power)

> + #:use-module (srfi srfi-1)

> + #:use-module (srfi srfi-26)

> + #:use-module (ice-9 match)

> + #:use-module (gnu)

> + #:use-module (gnu packages admin)

> + #:use-module (gnu packages linux)

> + #:use-module (gnu packages power)

> + #:use-module (gnu services)

> + #:use-module (gnu services configuration)

> + #:use-module (gnu services shepherd)

> + #:use-module (guix packages)

> + #:use-module (guix records)

> + #:export (apcupsd-service-type

> +

> + apcupsd-configuration

> + apcupsd-configuration-package

> + apcupsd-configuration-shepherd-base-name

> + apcupsd-configuration-auto-start?

> + apcupsd-configuration-pid-file

> + apcupsd-configuration-debug-level

> + apcupsd-configuration-run-dir

> + apcupsd-configuration-name

> + apcupsd-configuration-cable

> + apcupsd-configuration-type

> + apcupsd-configuration-device

> + apcupsd-configuration-poll-time

> + apcupsd-configuration-on-batery-delay

> + apcupsd-configuration-battery-level

> + apcupsd-configuration-remaining-minutes

> + apcupsd-configuration-timeout

> + apcupsd-configuration-annoy-interval

> + apcupsd-configuration-annoy-delay

> + apcupsd-configuration-no-logon

> + apcupsd-configuration-kill-delay

> + apcupsd-configuration-net-server

> + apcupsd-configuration-net-server-ip

> + apcupsd-configuration-net-server-port

> + apcupsd-configuration-net-server-events-file

> + apcupsd-configuration-net-server-events-file-max-size

> + apcupsd-configuration-class

> + apcupsd-configuration-mode

> + apcupsd-configuration-stat-time

> + apcupsd-configuration-log-stats

> + apcupsd-configuration-data-time

> + apcupsd-configuration-facility

> + apcupsd-configuration-event-handlers

> +

> + apcupsd-event-handlers-annoyme

> + apcupsd-event-handlers-battattach

> + apcupsd-event-handlers-battdetach

> + apcupsd-event-handlers-changeme

> + apcupsd-event-handlers-commfailure

> + apcupsd-event-handlers-commok

> + apcupsd-event-handlers-doreboot

> + apcupsd-event-handlers-doshutdown

> + apcupsd-event-handlers-emergency

> + apcupsd-event-handlers-endselftest

> + apcupsd-event-handlers-failing

> + apcupsd-event-handlers-killpower

> + apcupsd-event-handlers-loadlimit

> + apcupsd-event-handlers-mainsback

> + apcupsd-event-handlers-offbattery

> + apcupsd-event-handlers-onbattery

> + apcupsd-event-handlers-powerout

> + apcupsd-event-handlers-remotedown

> + apcupsd-event-handlers-runlimit

> + apcupsd-event-handlers-startselftest

> + apcupsd-event-handlers-timeout))

> +

> +(define-configuration/no-serialization apcupsd-event-handlers

> + (killpower

> + (gexp

> + #~((wall "Apccontrol doing: apcupsd --killpower on UPS ~a" name)

> + (sleep 10)

> + (apcupsd "--killpower")

> + (wall "Apccontrol has done: apcupsd --killpower on UPS ~a" name)))

> + "Handler for killpower event.")

> + (commfailure

> + (gexp

> + #~((let ((msg (format #f "~a Communications with UPS ~a lost."

> + (gethostname) name)))

> + (mail-to-root msg msg))

> + (wall "Warning: communications lost with UPS ~a" name)))

> + "Handler for commfailure event.")

> + (commok

> + (gexp

> + #~((let ((msg (format #f "~a Communications with UPS ~a restored."

> + (gethostname) name)))

> + (mail-to-root msg msg))

> + (wall "Communications restored with UPS ~a" name)))

> + "Handler for commfailure event.")

> + (powerout

> + (gexp

> + #~(#t))

> + "Handler for powerout event.")

> + (onbattery

> + (gexp

> + #~((let ((msg (format #f "~a UPS ~a Power Failure !!!"

> + (gethostname) name)))

> + (mail-to-root msg msg))

> + (wall "Power failure on UPS ~a. Running on batteries." name)))

> + "Handler for onbattery event.")

> + (offbattery

> + (gexp

> + #~((let ((msg (format #f "~a UPS ~a Power has returned."

> + (gethostname) name)))

> + (mail-to-root msg msg))

> + (wall "Power has returned on UPS ~a..." name)))

> + "Handler for offbattery event.")

> + (mainsback

> + (gexp

> + #~((when (file-exists? powerfail-file)

> + (wall "Continuing with shutdown."))))

> + "Handler for mainsback event.")

> + (failing

> + (gexp

> + #~((wall "Battery power exhausted on UPS ~a. Doing shutdown." name)))

> + "Handler for failing event.")

> + (timeout

> + (gexp

> + #~((wall "Battery time limit exceeded on UPS ~a. Doing shutdown." name)))

> + "Handler for timeout event.")

> + (loadlimit

> + (gexp

> + #~((wall "Remaining battery charge below limit on UPS ~a. Doing shutdown." name)))

> + "Handler for loadlimit event.")

> + (runlimit

> + (gexp

> + #~((wall "Remaining battery runtime below limit on UPS ~a. Doing shutdown." name)))

> + "Handler for runlimit event.")

> + (doreboot

> + (gexp

> + #~((wall "UPS ~a initiating Reboot Sequence" name)

> + (system* #$(file-append shepherd "/sbin/reboot"))))

> + "Handler for doreboot event.")

> + (doshutdown

> + (gexp

> + #~((wall "UPS ~a initiated Shutdown Sequence" name)

> + (system* #$(file-append shepherd "/sbin/halt"))))

> + "Handler for doshutdown event.")

> + (annoyme

> + (gexp

> + #~((wall "Power problems with UPS ~a. Please logoff." name)))

> + "Handler for annoyme event.")

> + (emergency

> + (gexp

> + #~((wall "Emergency Shutdown. Possible battery failure on UPS ~a." name)))

> + "Handler for emergency event.")

> + (changeme

> + (gexp

> + #~((let ((msg (format #f "~a UPS ~a battery needs changing NOW."

> + (gethostname) name)))

> + (mail-to-root msg msg))

> + (wall "Emergency! Batteries have failed on UPS ~a. Change them NOW." name)))

> + "Handler for changeme event.")

> + (remotedown

> + (gexp

> + #~((wall "Remote Shutdown. Beginning Shutdown Sequence.")))

> + "Handler for remotedown event.")

> + (startselftest

> + (gexp

> + #~(#t))

> + "Handler for startselftest event.")

> + (endselftest

> + (gexp

> + #~(#t))

> + "Handler for endselftest event.")

> + (battdetach

> + (gexp

> + #~(#t))

> + "Handler for battdetach event.")

> + (battattach

> + (gexp

> + #~(#t))

> + "Handler for battattach event."))

> +(define-syntax define-enum

> + (lambda (x)

> + (syntax-case x ()

> + ((_ name values)

> + (let* ((d/n (syntax->datum #'name))

> + (d/predicate (string->symbol

> + (format #f "enum-~a?" d/n)))

> + (d/serialize (string->symbol

> + (format #f "serialize-enum-~a" d/n))))

> + (with-syntax

> + ((predicate (datum->syntax x d/predicate))

> + (serialize (datum->syntax x d/serialize)))

> + #'(begin

> + (define (predicate value)

> + (memq value values))

> + (define serialize serialize-symbol))))))))

> +(define mangle-field-name

> + (match-lambda

> + ('name "UPSNAME")

> + ('cable "UPSCABLE")

> + ('type "UPSTYPE")

> + ('device "DEVICE")

> + ('poll-time "POLLTIME")

> + ('lock-dir "LOCKFILE")

> + ('power-fail-dir "PWRFAILDIR")

> + ('no-login-dir "NOLOGINDIR")

> + ('on-batery-delay "ONBATTERYDELAY")

> + ('battery-level "BATTERYLEVEL")

> + ('remaining-minutes "MINUTES")

> + ('timeout "TIMEOUT")

> + ('annoy-interval "ANNOY")

> + ('annoy-delay "ANNOYDELAY")

> + ('no-logon "NOLOGON")

> + ('kill-delay "KILLDELAY")

> + ('net-server "NETSERVER")

> + ('net-server-ip "NISIP")

> + ('net-server-port "NISPORT")

> + ('net-server-events-file "EVENTSFILE")

> + ('net-server-events-file-max-size "EVENTSFILEMAX")

> + ('class "UPSCLASS")

> + ('mode "UPSMODE")

> + ('stat-time "STATTIME")

> + ('stat-file "STATFILE")

> + ('log-stats "LOGSTATS")

> + ('data-time "DATATIME")

> + ('facility "FACILITY")))

> +

> +(define (serialize-string field-name value)

> + #~(format #f "~a ~a\n" #$(mangle-field-name field-name) '#$value))

> +(define serialize-symbol serialize-string)

> +(define serialize-integer serialize-string)

> +(define (serialize-boolean field-name value)

> + #~(format #f "~a ~a\n" #$(mangle-field-name field-name) #$(if value "on" "off")))

> +

> +(define-maybe string)

> +

> +(define-enum cable '( simple smart ether usb

> + 940-0119A 940-0127A 940-0128A 940-0020B 940-0020C

> + 940-0023A 940-0024B 940-0024C 940-1524C 940-0024G

> + 940-0095A 940-0095B 940-0095C 940-0625A MAM-04-02-2000))

> +(define-enum type '(apcsmart usb net snmp netsnmp dumb pcnet modbus test))

> +(define-enum no-logon '(disable timeout percent minutes always))

> +(define-enum class '(standalone shareslave sharemaster))

> +(define-enum mode '(disable share))

> +

> +(define-configuration apcupsd-configuration

> + (package (package apcupsd) "Package to use.")

> Hi Tomas,

>

> Tomas Volf <~@wolfsden.cz> writes:

>

>> * gnu/packages/power.scm (apcupsd): New variable.

>

> 'guix lint' says:

>

> gnu/packages/power.scm:120:14: apcupsd@3.14.14: no article allowed at the beginning of the synopsis

> gnu/packages/power.scm:119:15: apcupsd@3.14.14: TLS certificate error: X.509 server certificate for 'www.apcupsd.org' does not match: CN=sf.net

>

>

> 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:

>

> 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.

>

> 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}

> 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?

> Hi,

>

> Tomas Volf <~@wolfsden.cz> writes:

>

>> --- /dev/null

>> +++ b/gnu/packages/power.scm

>> @@ -0,0 +1,125 @@

>> +;;; Copyright (C) 2025 Tomas Volf <~@wolfsden.cz>

>> +;;; Copyright (C) 2023 Raven Hallsby <karl@hallsby.com>

>

> One last thing; I don't see Raven listed in a 'Co-authored-by:' git

> trailer in the commit message; should they? Or otherwise mention

> plainly this work was based on their previous work, made available

> '$where'.

> Hi,

>

> Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

>

>> Hi,

>>

>> Tomas Volf <~@wolfsden.cz> writes:

>>

>>> --- /dev/null

>>> +++ b/gnu/packages/power.scm

>>> @@ -0,0 +1,125 @@

>>> +;;; Copyright (C) 2025 Tomas Volf <~@wolfsden.cz>

>>> +;;; Copyright (C) 2023 Raven Hallsby <karl@hallsby.com>

>>

>> One last thing; I don't see Raven listed in a 'Co-authored-by:' git

>> trailer in the commit message; should they? Or otherwise mention

>> plainly this work was based on their previous work, made available

>> '$where'.

>

> I did not know about co-authored trailer. Should it be used even in

> situations where I just took a look at his version online without any

> active involvement? Currently I have modified the commit message to:

> gnu: Add apcupsd.

>

> Some parts were taken or inspired by the work of Raven Hallsby available

> here[0]. For that reason I have added his copyright as well (I asked for the

> permission).

>

> 0: https://raw.githubusercontent.com/KarlJoad/guix/9013b5ac3fadb48fad2e7ef1fbfaa4848dcb922a/gnu/packages/power.scm

>

> * gnu/packages/power.scm (apcupsd): New variable.

> * gnu/local.mk (GNU_SYSTEM_MODULES): Register the new file.

>

> Change-Id: I0e4b2f50c8adf0f96d140e2be0f79e3740f4955c

>

> Is that sufficient?

>> 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

>

> This turns on a test driver that is used only for debugging.

>

>

> 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.

>>> + (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?

>>

>>> + (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.

> I do not think this is correct. When I check

> apcupsd-3.14.14/src/apcupsd.c file, I see this in its header:

> 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.

>> 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.

>> 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.

>

> OK, thank you.

>>>

>>>> + ;; Enable additional drivers.

>>>> + "--enable-test"

>>>

>>> Is '--enable-test' useful? What does it do?

>>

>> According to the manual

>>

>> This turns on a test driver that is used only for debugging.

>>

>>

>> 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.

>

> For this kind of very edge case that is not enabled by default, I'd

> leave it off. Our maximalist take on things doesn't including switching

> on every non-default option, but if extra dependencies are automatically

> picked as part of the configure script, then we would typically add

> them, unless they grow the 'guix size' of the package unreasonably (like

> adding libreoffice to emacs-org-mode because it has support for it :-)).

>

> It's one of the reason. Other reasons are references kept erroneously,

> static libraries, large documentation, wrappers keeping references to

> native inputs that shouldn't be captured, etc. There's a lot of work to

> do to reduce the size of the distribution :-).

>

> [...]

>>>> + (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?

>

> Yes, I meant above, sorry. I don't see mandoc and util-linux as large

> dependencies enough to justify working around these, especially if they

> are only used at build time. And, the would actually be useful when

> working on the source of acupsd in a 'guix shell -D acupsd' environment,

> so I'd just add them.

>

> [...]

>

>>> 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.

>

> I haven't seen v2 yet, I guess you are still working out the service

> part?

>> I haven't seen v2 yet, I guess you are still working out the service

>> part?

>

> I did not get to the service part yet (life has sadly been somewhat

> busy), will get to it during this weekend. But I was also waiting for

> your response here, there was no reason to send v2 since I was pretty

> sure I will need to do additional changes, so v3 would required anyway.

> Hi Tomas,

>

> Tomas Volf <~@wolfsden.cz> writes:

>

>> * gnu/services/power.scm: New file.

>

> I see there's already a gnu/services/pm.scm file; it seems we don't need

> another one? Or was it a conscious decision?

>

>> +The @code{(gnu services power)} module provides a service definition for

>> +@uref{http://www.apcupsd.org/, apcupsd}, a utility to interact with APC

>> +UPSes. Apcupsd also works with some OEM-branded products manufactured

>> +by APC.

>

> I'd introduce a few @acronym here to help readers, e.g. for OEM and

> UPS.

>

>> +@defvar apcupsd-service-type

>> +The service type for apcupsd. For USB UPSes no configuration is

>> +necessary, however tweaking some fields to better suit your needs might

>> +be desirable. The defaults are taken from the upstream configuration

>> +and they are not very conservative (@code{battery-level} of 5% is too

>> +low in my opinion).

>

> I'd say, (for example, the default @code{battery-level} of 5% may be

> considered too low by some), to keep it neutral.

>

>> +The default event handlers do send emails, read more in

>> +@ref{apcupsd-event-handlers}.

>> +

>> +@lisp

>> +(service apcupsd-service-type)

>> +@end lisp

>> +@end defvar

>> +

>> +@deftp {Data Type} apcupsd-configuration

>> +

>> +Available @code{apcupsd-configuration} fields are:

>> +

>> +@table @asis

>> +@item @code{package} (default: @code{apcupsd}) (type: package)

>> +Package to use.

>

> That's more conventionally named with the same of the package,

> e.g. 'acpupsd', not 'package'. I'd word the description as 'The

> @code{apcupsd} package to use'.

>

>> +@item @code{shepherd-base-name} (default: @code{apcupsd}) (type: symbol)

>> +Base name of the shepherd service. You can add the service multiple

>> +times with different prefix to manage multiple UPSes.

>

> s/prefix/prefixes/

>> +

>> +@item @code{auto-start?} (default: @code{#t}) (type: boolean)

>> +Should the shepherd service auto-start?

>> +

>> +@item @code{pid-file} (default: @code{"/var/run/apcupsd.pid"}) (type: string)

>> +Path to the pid file.

>

> All file names becoming /run/ prefixed instead of /var/run, based on a

> previous comment.

>

> We don't use path in GNU to denote file names, we use 'file names'.

> That's mentioned in info '(standards) GNU Manuals':

>

> Please do not use the term "pathname" that is used in Unix

> documentation; use "file name" (two words) instead. We use the term

> "path" only for search paths, which are lists of directory names.

>

>> +

>> +@item @code{debug-level} (default: @code{0}) (type: integer)

>> +Logging verbosity. Bigger number means more logs. In the source code I

>> +saw up to @code{300}, so for all logs, @code{999} seems reasonable.

>

> I think it's best to not use first person pronoun in the manual to keep

> it a bit more formal. So instead of 'I saw', this could be reworded to:

> 'The source code uses up to @code{300} as debug level value, so a value

> of @code{999} seems reasonable to enable all logs.' or similar.

>

>> +

>> +@item @code{run-dir} (default: @code{"/var/run/apcupsd"}) (type: string)

>> +Directory containing runtime information. You need to change this if

>> +you desire to run multiple instances of the daemon.

>> +

>> +@item @code{name} (type: maybe-string)

>> +Use this to give your UPS a name in log files and such. This is

>> +particularly useful if you have multiple UPSes. This does not set the

>> +EEPROM. It should be 8 characters or less.

>> +

>> +@item @code{cable} (default: @code{usb}) (type: enum-cable)

>> +The type of cable connecting the UPS to your computer. Possible generic

>> +choices are @code{'simple}, @code{'smart}, @code{'ether} and

>> +@code{'usb}. Or a specific cable model number may be used:

>

> nitpick: I'd reword to 'Alternatively, a specific [...]', which reads

> better to me.

>

>> +@item usb

>> +A null string setting enables autodetection, which is the best choice

>> +for most installations.

>

> nitpick: My dictionary (aspell) prefers auto-detection.

>

> [...]

>

>> +@anchor{apcupsd-event-handlers}

>> +@deftp {Data Type} apcupsd-event-handlers

>> +

>> +For description of the events please refer to the @command{apcupsd}'s

>> +manual, which can be found in the @samp{apcupsd-doc} package.

>

> I think you also meant it is in the @samp{doc} output, not in a

> different package, right?

> In any case, I'd refer them to the man page of apccontrol, which

> details the events. Also, to avoid the odd rendering as "the

> ‘apcupsd’’s manual" in info, I'd instead rephrase to:

>

> For a description of the events please refer to @samp{man 8

> apccontrol}.

>

>> +

>> +Each handler shall be a gexp. It is spliced into the control script for

>> +the daemon. In addition to the standard Guile programming environment,

>> +these procedures and variables are also available.

>

> s/these/the following/ [...] s/available./available:/

> Since these are spliced in the control script and thus not at the top

> level, users won't be able to import Guile modules they may want to use,

> so it seems there should be a field to define which Guile modules should

> be imported and used by default in the configuration.

>

>> +

>> +@table @code

>> +@item conf

>> +Variable containing path to the configuration file.

>

> s/path/file name/, anywhere this appears.

>

>> +

>> +@item powerfail-file

>> +Variable containing path to the powerfail file.

>

> Ditto.

>

>> +@item cmd

>> +The event currently being handled.

>> +

>> +@item name

>> +The name of the UPS as specified in the configuration file.

>> +

>> +@item connected

>> +Is @code{"1"} if apcupsd is connected to the UPS via a serial port (or a

>

> @command{apcupsd}

>

> Also, any reason why these integer values are expressed as strings?

>

>> +USB port). In most configurations, this will be the case. In the case

>> +of a Slave machine where apcupsd is not directly connected to the UPS,

>

> s/Slave/slave/ ? Could be nicer to use a neutral alternative like

> 'follower machine'.

>

>> +this value will be @code{"0"}.

>> +

>> +@item powered

>> +Is @code{"1"} if the computer on which @command{apcupsd} is running is

>> +powered by the UPS and @code{"0"} if not. At the moment, this value is

>> +unimplemented and always @code{"0"}.

>> +

>> +@item (err @var{fmt} @var{args...})

>> +Wrapper around @code{format} outputting to @code{(current-error-port)}.

>> +

>> +@item (wall @var{fmt} @var{args...})

>> +Wrapper around @code{format} outputting via @command{wall}.

>> +

>> +@item (apcupsd @var{args...})

>> +Call @command{apcupsd} while passing the correct configuration file and

>> +all the arguments.

>> +

>> +@item (mail-to-root @var{subject} @var{body})

>> +Send an email to the local administrator. This procedure assumes the

>> +@command{sendmail} is located at @command{/run/privileged/bin/sendmail}

>> +(as would be the case with @code{opensmtpd-service-type}).

>> +

>> +@end table

>> +

>> +Available @code{apcupsd-event-handlers} fields are:

>> +

>> +@table @asis

>> +@item @code{killpower} (type: gexp)

>> +Handler for killpower event.

>> +

>> +@item @code{commfailure} (type: gexp)

>> +Handler for commfailure event.

>> +

>> +@item @code{commok} (type: gexp)

>> +Handler for commfailure event.

>> +

>> +@item @code{powerout} (type: gexp)

>> +Handler for powerout event.

>> +

>> +@item @code{onbattery} (type: gexp)

>> +Handler for onbattery event.

>> +

>> +@item @code{offbattery} (type: gexp)

>> +Handler for offbattery event.

>> +

>> +@item @code{mainsback} (type: gexp)

>> +Handler for mainsback event.

>> +

>> +@item @code{failing} (type: gexp)

>> +Handler for failing event.

>> +

>> +@item @code{timeout} (type: gexp)

>> +Handler for timeout event.

>> +

>> +@item @code{loadlimit} (type: gexp)

>> +Handler for loadlimit event.

>> +

>> +@item @code{runlimit} (type: gexp)

>> +Handler for runlimit event.

>> +

>> +@item @code{doreboot} (type: gexp)

>> +Handler for doreboot event.

>> +

>> +@item @code{doshutdown} (type: gexp)

>> +Handler for doshutdown event.

>> +

>> +@item @code{annoyme} (type: gexp)

>> +Handler for annoyme event.

>> +

>> +@item @code{emergency} (type: gexp)

>> +Handler for emergency event.

>> +

>> +@item @code{changeme} (type: gexp)

>> +Handler for changeme event.

>> +

>> +@item @code{remotedown} (type: gexp)

>> +Handler for remotedown event.

>> +

>> +@item @code{startselftest} (type: gexp)

>> +Handler for startselftest event.

>> +

>> +@item @code{endselftest} (type: gexp)

>> +Handler for endselftest event.

>> +

>> +@item @code{battdetach} (type: gexp)

>> +Handler for battdetach event.

>> +

>> +@item @code{battattach} (type: gexp)

>> +Handler for battattach event.

>> +

>> +@end table

>> +

>> +@end deftp

>

> nitpick: I'd remove the newline between the two @end above (I know, it

> gets generated this way...).

>

>> +;;; Copyright (C) 2025 Tomas Volf <~@wolfsden.cz>

>

> We use the unicode symbol (©) for copyright throughout Guix; please use

> it too, for uniformity.

>

>> +

>> +;;;; Commentary:

>> +

>> +;;; Power-related services.

>> +

>> +;;;; Code:

>> +

>> +(define-module (gnu services power)

>> + #:use-module (srfi srfi-1)

>> + #:use-module (srfi srfi-26)

>> + #:use-module (ice-9 match)

>> + #:use-module (gnu)

>> + #:use-module (gnu packages admin)

>> + #:use-module (gnu packages linux)

>> + #:use-module (gnu packages power)

>> + #:use-module (gnu services)

>> + #:use-module (gnu services configuration)

>> + #:use-module (gnu services shepherd)

>> + #:use-module (guix packages)

>> + #:use-module (guix records)

>

> Please order lexicographically (that is, alphabetically but on things

> which are not necessarily words :-)).

>

>> + #:export (apcupsd-service-type

>> +

>> + apcupsd-configuration

>> + apcupsd-configuration-package

>> + apcupsd-configuration-shepherd-base-name

>> + apcupsd-configuration-auto-start?

>> + apcupsd-configuration-pid-file

>> + apcupsd-configuration-debug-level

>> + apcupsd-configuration-run-dir

>> + apcupsd-configuration-name

>> + apcupsd-configuration-cable

>> + apcupsd-configuration-type

>> + apcupsd-configuration-device

>> + apcupsd-configuration-poll-time

>> + apcupsd-configuration-on-batery-delay

>> + apcupsd-configuration-battery-level

>> + apcupsd-configuration-remaining-minutes

>> + apcupsd-configuration-timeout

>> + apcupsd-configuration-annoy-interval

>> + apcupsd-configuration-annoy-delay

>> + apcupsd-configuration-no-logon

>> + apcupsd-configuration-kill-delay

>> + apcupsd-configuration-net-server

>> + apcupsd-configuration-net-server-ip

>> + apcupsd-configuration-net-server-port

>> + apcupsd-configuration-net-server-events-file

>> + apcupsd-configuration-net-server-events-file-max-size

>> + apcupsd-configuration-class

>> + apcupsd-configuration-mode

>> + apcupsd-configuration-stat-time

>> + apcupsd-configuration-log-stats

>> + apcupsd-configuration-data-time

>> + apcupsd-configuration-facility

>> + apcupsd-configuration-event-handlers

>> +

>> + apcupsd-event-handlers-annoyme

>> + apcupsd-event-handlers-battattach

>> + apcupsd-event-handlers-battdetach

>> + apcupsd-event-handlers-changeme

>> + apcupsd-event-handlers-commfailure

>> + apcupsd-event-handlers-commok

>> + apcupsd-event-handlers-doreboot

>> + apcupsd-event-handlers-doshutdown

>> + apcupsd-event-handlers-emergency

>> + apcupsd-event-handlers-endselftest

>> + apcupsd-event-handlers-failing

>> + apcupsd-event-handlers-killpower

>> + apcupsd-event-handlers-loadlimit

>> + apcupsd-event-handlers-mainsback

>> + apcupsd-event-handlers-offbattery

>> + apcupsd-event-handlers-onbattery

>> + apcupsd-event-handlers-powerout

>> + apcupsd-event-handlers-remotedown

>> + apcupsd-event-handlers-runlimit

>> + apcupsd-event-handlers-startselftest

>> + apcupsd-event-handlers-timeout))

>> +

>> +(define-configuration/no-serialization apcupsd-event-handlers

>> + (killpower

>> + (gexp

>> + #~((wall "Apccontrol doing: apcupsd --killpower on UPS ~a" name)

>> + (sleep 10)

>> + (apcupsd "--killpower")

>> + (wall "Apccontrol has done: apcupsd --killpower on UPS ~a" name)))

>> + "Handler for killpower event.")

>

> In many places in your writing, I find that it skips adding 'the'

> (definite article), which I've learned should be use when describing

> something specific, such as here with the specific handlers:

>

> "Handler for the @code{killpower} event."

>

> In case it helps, I've found this page seems useful in briefly

> explaining when to use 'the' [0].

>

> [0] https://owl.purdue.edu/owl/general_writing/grammar/using_articles.html

>

>> + (commfailure

>> + (gexp

>> + #~((let ((msg (format #f "~a Communications with UPS ~a lost."

>> + (gethostname) name)))

>> + (mail-to-root msg msg))

>> + (wall "Warning: communications lost with UPS ~a" name)))

>> + "Handler for commfailure event.")

>> + (commok

>> + (gexp

>> + #~((let ((msg (format #f "~a Communications with UPS ~a restored."

>> + (gethostname) name)))

>> + (mail-to-root msg msg))

>> + (wall "Communications restored with UPS ~a" name)))

>> + "Handler for commfailure event.")

>> + (powerout

>> + (gexp

>> + #~(#t))

>> + "Handler for powerout event.")

>> + (onbattery

>> + (gexp

>> + #~((let ((msg (format #f "~a UPS ~a Power Failure !!!"

>> + (gethostname) name)))

>> + (mail-to-root msg msg))

>> + (wall "Power failure on UPS ~a. Running on batteries." name)))

>> + "Handler for onbattery event.")

>> + (offbattery

>> + (gexp

>> + #~((let ((msg (format #f "~a UPS ~a Power has returned."

>> + (gethostname) name)))

>> + (mail-to-root msg msg))

>> + (wall "Power has returned on UPS ~a..." name)))

>> + "Handler for offbattery event.")

>> + (mainsback

>> + (gexp

>> + #~((when (file-exists? powerfail-file)

>> + (wall "Continuing with shutdown."))))

>> + "Handler for mainsback event.")

>> + (failing

>> + (gexp

>> + #~((wall "Battery power exhausted on UPS ~a. Doing shutdown." name)))

>> + "Handler for failing event.")

>> + (timeout

>> + (gexp

>> + #~((wall "Battery time limit exceeded on UPS ~a. Doing shutdown." name)))

>> + "Handler for timeout event.")

>> + (loadlimit

>> + (gexp

>> + #~((wall "Remaining battery charge below limit on UPS ~a. Doing shutdown." name)))

>> + "Handler for loadlimit event.")

>> + (runlimit

>> + (gexp

>> + #~((wall "Rema

>> I see there's already a gnu/services/pm.scm file; it seems we don't need

>> another one? Or was it a conscious decision?

>

> Yes, it was intentional, for couple of reasons (in no particular order).

>

> I like the symmetry between gnu/{packages,services}/power.scm. It makes

> finding the service-type easy without even opening the manual.

>

> The "pm" is for "power management", and I have never heard anyone call

> apcupsd "a power management daemon". It reacts to power outage, true,

> but it does not "manage" the power in any way. So it seemed like a poor

> fit.

>

> The current pm.scm exports 8 bindings, the new power.scm 54. So it

> would feel like taking over the file.

> Shorter files are faster to compile (especially when

> define-configuration is used), so having two files that can be built in

> parallel seemed better.

>

> And like this I can also write the changelog in to the commit message in

> a reasonably short form. If I put it into pm.scm, I would need to list

> every single binding, in power.scm I mention just a new file.

>>> +

>>> +@item @code{auto-start?} (default: @code{#t}) (type: boolean)

>>> +Should the shepherd service auto-start?

>>> +

>>> +@item @code{pid-file} (default: @code{"/var/run/apcupsd.pid"}) (type: string)

>>> +Path to the pid file.

>>

>> All file names becoming /run/ prefixed instead of /var/run, based on a

>> previous comment.

>

> I will just quote myself from the package's patch, since it seems I did

> not get an answer there:

>

> 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?

>

> Thanks.

>

>>

>> We don't use path in GNU to denote file names, we use 'file names'.

>> That's mentioned in info '(standards) GNU Manuals':

>>

>> Please do not use the term "pathname" that is used in Unix

>> documentation; use "file name" (two words) instead. We use the term

>> "path" only for search paths, which are lists of directory names.

>>

>

> Changed into "File name of the pid file.".

> Out of curiosity, does this apply to directories as well? So,

> hypothetically, I should instead of "Path to the directory storing XY."

> use "File name of the directory storing XY."?

>>> +

>>> +@item @code{debug-level} (default: @code{0}) (type: integer)

>>> +Logging verbosity. Bigger number means more logs. In the source code I

>>> +saw up to @code{300}, so for all logs, @code{999} seems reasonable.

>>

>> I think it's best to not use first person pronoun in the manual to keep

>> it a bit more formal. So instead of 'I saw', this could be reworded to:

>> 'The source code uses up to @code{300} as debug level value, so a value

>> of @code{999} seems reasonable to enable all logs.' or similar.

>

> Sorry, I have probably gotten too influenced by (gnus) and its personal

> style I have enjoyed a lot.

>

> I have used your suggestion verbatim.

> Done. I went back to the package and replaced it there as well. It is

> bit unfortunate that autotools call the variables `ac_cv_path_', instead

> of `ac_cv_file_name_', but nothing I can do about that.

>> Also, any reason why these integer values are expressed as strings?

>

> No. I have just adhered to the original calling convention of the shell

> script, and argv is all char*. These are even not really an integer

> values, but bools. So I changed this one to `connected?', and the other

> one to `powered?', with #t/#f values.

>

>>

>>> +USB port). In most configurations, this will be the case. In the case

>>> +of a Slave machine where apcupsd is not directly connected to the UPS,

>>

>> s/Slave/slave/ ? Could be nicer to use a neutral alternative like

>> 'follower machine'.

>

> The "Slave machine" (including the capitalization) is a term used in the

> manual. I can lower-case it if you would prefer, seems that manual

> actually uses both casings now when I looked for it.

>

> I do not think inventing new terminology is a good idea, since users

> will not be able to find it in the manual distributed with the program.

>> We use the unicode symbol (©) for copyright throughout Guix; please use

>> it too, for uniformity.

>

> So © is fine, but λ is not. I fail to see the consistency.

>>> +

>>> +;;;; Commentary:

>>> +

>>> +;;; Power-related services.

>>> +

>>> +;;;; Code:

>>> +

>>> +(define-module (gnu services power)

>>> + #:use-module (srfi srfi-1)

>>> + #:use-module (srfi srfi-26)

>>> + #:use-module (ice-9 match)

>>> + #:use-module (gnu)

>>> + #:use-module (gnu packages admin)

>>> + #:use-module (gnu packages linux)

>>> + #:use-module (gnu packages power)

>>> + #:use-module (gnu services)

>>> + #:use-module (gnu services configuration)

>>> + #:use-module (gnu services shepherd)

>>> + #:use-module (guix packages)

>>> + #:use-module (guix records)

>>

>> Please order lexicographically (that is, alphabetically but on things

>> which are not necessarily words :-)).

>

> Interesting. I personally order imports lexicographically, but within

> groups of:

>

> 1. stdlib (srfi, rnrs, ...)

> 2. guile extensions (ice-9, ...)

> 3. 3rd-party dependencies (guile-lib, guile-json, ...)

> 4. modules from the program itself (gnu, guix, ...)

> Given your requirement for sorting everything lexicographically (with no

> grouping), the following:

>

> (use-modules (gnu services)

> (srfi srfi-1))

>

> (when #f

> (modify-services '()

> (delete 'x)))

>

>

> Prints a warning when compiled:

>

> WARNING: (guile-user): imported module (gnu services) overrides core binding `delete'

>

> However, when done "my" way (putting srfi-1 first, since "stdlib"), the

> warning goes away.

>>> +(define-configuration/no-serialization apcupsd-event-handlers

>>> + (killpower

>>> + (gexp

>>> + #~((wall "Apccontrol doing: apcupsd --killpower on UPS ~a" name)

>>> + (sleep 10)

>>> + (apcupsd "--killpower")

>>> + (wall "Apccontrol has done: apcupsd --killpower on UPS ~a" name)))

>>> + "Handler for killpower event.")

>>

>> Sounds fun :-). I was wondering if the handlers could be of the

>> maybe-gexp type, and when not provided the handler would not be called

>> at all? Or does the design of apcupsd mandates that all handlers be

>> defined?

>

> The control script is called for all events. So I *could* use

> maybe-gexp, and change

>

> ("battattach"

> #$@(apcupsd-event-handlers-battattach

> (apcupsd-configuration-event-handlers cfg)))

>

>

> into

>

> ("battattach"

> #$@(let ((h (apcupsd-event-handlers-battattach

> (apcupsd-configuration-event-handlers cfg))))

> (if (maybe-value-set? h)

> h

> #~(#t))))

>

>

> and do that for all the events. But I have to admit I do not consider

> that an improvement. There probably is a better way to express that,

> but the current approach (of always having, possibly NOP, handler

> defined) helps keep the code straight forward.

>>> +(define-syntax define-enum

>>> + (lambda (x)

>>> + (syntax-case x ()

>>> + ((_ name values)

>>> + (let* ((d/n (syntax->datum #'name))

>>> + (d/predicate (string->symbol

>>> + (format #f "enum-~a?" d/n)))

>>> + (d/serialize (string->symbol

>>> + (format #f "serialize-enum-~a" d/n))))

>>

>> Even for internal bindings, better variable names would help reading the

>> code.

>>

>

> When I am writing syntax-case, I always struggle with the naming, since

> I effectively have to have two variables for a single thing. One for

> datum, and one for syntax.

>

> I have expanded the d/ info full datum/, however if can suggest better

> naming scheme for situations like this, I am all ears.

>>> + (shepherd-base-name

>>> + (symbol 'apcupsd)

>>> + "Base name of the shepherd service. You can add the service multiple times

>>> +with different prefix to manage multiple UPSes."

>>> + empty-serializer)

>>> + (auto-start?

>>> + (boolean #t)

>>> + "Should the shepherd service auto-start?"

>>> + empty-serializer)

>>> + (pid-file

>>> + (string "/var/run/apcupsd.pid")

>>

>> /var/run -> /run (adjust everywhere)

>

> See the reaction above regarding the /var/run.

> The s/ was supposed to stand for script/, I originally expected I will

> have more than one. I have renamed it to %apccontrol.

>

>>

>>> + (program-file

>>> + "apccontrol"

>>> + #~(begin

>>> + (use-modules (srfi srfi-9)

>>> + (ice-9 format)

>>> + (ice-9 match)

>>> + (ice-9 popen))

>>

>> Please sort modules lexicographically.

>

> See the reaction above.

>>> + ;; Script dir depends on these, and the configuration depends on the script

>>> + ;; dir. To sever the cyclic dependency, pass the paths via environment

>>> + ;; variables.

>>> + (define conf (getenv "GUIX_APCUPSD_CONF"))

>>> + (define powerfail-file (getenv "GUIX_APCUPSD_POWERFAIL_FILE"))

>>> +

>>> + (define (err . args)

>>> + (apply format (current-error-port) args))

>>> + (define (wall . args)

>>> + (system* #$(file-append util-linux "/bin/wall") (apply format #f args)))

>>> + (define (apcupsd . args)

>>> + (apply system* #$(file-append apcupsd "/sbin/apcupsd") "-f" conf args))

>>> + (define (mail-to-root subject body)

>>> + (let ((port (open-pipe* OPEN_WRITE

>>> + "/run/privileged/bin/sendmail"

>>> + "-F" "apcupsd"

>>> + "root")))

>>> + (format port "Subject: ~a~%~%~a~&" subject body)

>>> + (close-pipe port)))

>>> + (match (cdr (command-line))

>>> + (((? string? cmd) name connected powered)

>>> + (match cmd

>>> + ;; I am sure this could be done by macro, but meh. Last release of

>>> + ;; apcupsd was in 2016, so maintaining this will not be much work.

>>> + ("killpower"

>>> + #$@(apcupsd-event-handlers-killpower

>>> + (apcupsd-configuration-event-handlers cfg)))

>>> + ("commfailure"

>>> + #$@(apcupsd-event-handlers-commfailure

>>> + (apcupsd-configuration-event-handlers cfg)))

>>> + ("commok"

>>> + #$@(apcupsd-event-handlers-commok

>>> + (apcupsd-configuration-event-handlers cfg)))

>>> + ("powerout"

>>> + #$@(apcupsd-event-handlers-powerout

>>> + (apcupsd-configuration-event-handlers cfg)))

>>> + ("onbattery"

>>> + #$@(apcupsd-event-handlers-onbattery

>>> + (apcupsd-configuration-event-handlers cfg)))

>>> + ("offbattery"

>>> + #$@(apcupsd-event-handlers-offbattery

>>> + (apcupsd-configuration-event-handlers cfg)))

>>> + ("mainsback"

>>> + #$@(apcupsd-event-handlers-mainsback

>>> + (apcupsd-configuration-event-handlers cfg)))

>>> + ("failing"

>>> + #$@(apcupsd-event-handlers-failing

>>> + (apcupsd-configuration-event-handlers cfg)))

>>> + ("timeout"

>>> + #$@(apcupsd-event-handlers-timeout

>>> + (apcupsd-configuration-event-handlers cfg)))

>>> + ("loadlimit"

>>> + #$@(apcupsd-event-handlers-loadlimit

>>> + (apcupsd-configuration-event-handlers cfg)))

>>> + ("runlimit"

>>> + #$@(apcupsd-event-handlers-runlimit

>>> + (apcupsd-configuration-event-handlers cfg)))

>>> + ("doreboot"

>>> + #$@(apcupsd-event-handlers-doreboot

>>> + (apcupsd-configuration-event-handlers cfg)))

>>> + ("doshutdown"

>>> + #$@(apcupsd-event-handlers-doshutdown

>>> + (apcupsd-configuration-event-handlers cfg)))

>>> + ("annoyme"

>>> + #$@(apcupsd-event-handlers-annoyme

>>> + (apcupsd-configuration-event-handlers cfg)))

>>> + ("emergency"

>>> + #$@(apcupsd-event-handlers-emergency

>>> + (apcupsd-configuration-event-handlers cfg)))

>>> + ("changeme"

>>> + #$@(apcupsd-event-handlers-changeme

>>> + (apcupsd-configuration-event-handlers cfg)))

>>> + ("remotedown"

>>> + #$@(apcupsd-event-handlers-remotedown

>>> + (apcupsd-configuration-event-handlers cfg)))

>>> + ("startselftest"

>>> + #$@(apcupsd-event-handlers-startselftest

>>> + (apcupsd-configuration-event-handlers cfg)))

>>> + ("endselftest"

>>> + #$@(apcupsd-event-handlers-endselftest

>>> + (apcupsd-configuration-event-handlers cfg)))

>>> + ("battdetach"

>>> + #$@(apcupsd-event-handlers-battdetach

>>> + (apcupsd-configuration-event-handlers cfg)))

>>> + ("battattach"

>>> + #$@(apcupsd-event-handlers-battattach

>>> + (apcupsd-configuration-event-handlers cfg)))

>>> + (_

>>> + (err "Unknown event: ~a~%" cmd)

>>> + (err "Iff the event was passed by apcupsd, this is a bug.~%")

>> I don't understand, in which scenario would an event *not* be emitted

>> by apcupsd?

>

> If you call the script manually from command line. ^_^

>>> + (let ((run-dir (apcupsd-configuration-run-dir cfg)))

>>> + (mixed-text-file

>>> + "apcupsd.conf"

>>> + "\

>>> +## apcupsd.conf v1.1 ##

>>> +#

>>> +# for apcupsd release 3.14.14 (31 May 2016) - GNU Guix

>>

>> Is this config really bound to a version like above? Unless they change

>> things in backward incompatible ways, it should work for newer ones too,

>> no?

>

> This file header is taken from the official configuration file

> distributed with the apcupsd (except the GNU Guix part). So while I do

> expect that this configuration file should work with newer version, the

> upstream decided to put the version into the configuration file.

>

> I have no strong opinion either way here, should I just remove the file

> header completely?

>>> +#

>>> +# \"apcupsd\" POSIX config file (crafted via apcupsd-service-type)

>>

>> s/crafted via/auto-generated by/

>

> But, but, "crafted" sound way more fancy. :) Well, replaced anyway.

> However I have used just "generated by", the "auto-" part felt bit off.

>>> +"

>>> + (serialize-configuration cfg apcupsd-configuration-fields)

>>> + ;; This one is confusing. The manual page states:

>>> + ;;

>>> + ;; > It must be changed when running more than one copy of apcupsd on the

>>> + ;; > same computer to control multiple UPSes.

>>> + ;;

>>> + ;; However would you not want the lock to be per-device, not per-process?

>>> + ;; I decided to follow the documentation, but I do not understand why it

>>> + ;; should be like this. I do not have multiple UPSes to try.

>>> + (serialize-string 'lock-dir (string-append run-dir "/lock"))

>>> + (serialize-string 'power-fail-dir run-dir)

>>> + (serialize-string 'no-login-dir run-dir)

>>> + (serialize-string 'stat-file (string-append run-dir "/apcupsd.status"))

>>> + "SCRIPTDIR " (apcupsd-script-dir cfg) "\n")))

>>> +

>>> +(define (apcupsd-shepherd-services cfg)

>>

>> s/cfg/config/

>

> And done.

>

>>

>>> + (match-record cfg <apcupsd-configuration>

>>> + ( package pid-file debug-level run-dir

>> ^ extraneous space

>

> This is intentional, it is neat little trick I have learned recently.

> Without the extra leading space, Emacs indents it in this way:

>

> (match-record config <apcupsd-configuration>

> (apcupsd pid-file debug-level run-dir

> shepherd-base-name auto-start?)

>

>

> However, with the extra leading space, the indentation changes to:

>

> (match-record config <apcupsd-configuration>

> ( apcupsd pid-file debug-level run-dir

> shepherd-base-name auto-start?)

>

>

> Since in this case it is not a procedure call, but a list, first member

> (apcupsd) is not special in any way. So it feels weird for it to have a

> special priority regarding the indentation. With the extra leading

> space, all list members are equal, as they should be.

>>>> +

>>>> +@item @code{auto-start?} (default: @code{#t}) (type: boolean)

>>>> +Should the shepherd service auto-start?

>>>> +

>>>> +@item @code{pid-file} (default: @code{"/var/run/apcupsd.pid"}) (type: string)

>>>> +Path to the pid file.

>>>

>>> All file names becoming /run/ prefixed instead of /var/run, based on a

>>> previous comment.

>>

>> I will just quote myself from the package's patch, since it seems I did

>> not get an answer there:

>>

>> 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?

>

> Deprecating /var/run to /run is not a Guix thing, but a File Hiearchy

> Standard (FHS) thing [0]. Since we already use /run, there's no good

> reason to have /var/run something else than a symlink to /run, and

> there's an actual change pending merge that would do that, along making

> /run mounted as a tmpfs (bug#73494).

>

> [0] https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch05s13.html

>

> [..]

>

>>>

>>> s/Slave/slave/ ? Could be nicer to use a neutral alternative like

>>> 'follower machine'.

>>

>> The "Slave machine" (including the capitalization) is a term used in the

>> manual. I can lower-case it if you would prefer, seems that manual

>> actually uses both casings now when I looked for it.

>>

>> I do not think inventing new terminology is a good idea, since users

>> will not be able to find it in the manual distributed with the program.

>

> The 'slave' word always carry a negative connotation. I think it's a

> good idea to be proactive on modernizing the terminology where we can.

>> Given your requirement for sorting everything lexicographically (with no

>> grouping), the following:

>>

>> (use-modules (gnu services)

>> (srfi srfi-1))

>>

>> (when #f

>> (modify-services '()

>> (delete 'x)))

>>

>>

>> Prints a warning when compiled:

>>

>> WARNING: (guile-user): imported module (gnu services) overrides core binding `delete'

>>

>> However, when done "my" way (putting srfi-1 first, since "stdlib"), the

>> warning goes away.

>

> That's an interesting workaround, but I think we should look into that

> problem and fix it definitely I often used #:hidden (delete) without any

> issue it seems, so I'm not even sure if its existence still solves a

> true problem in current Guile.

>

> [...]

>

>>>> +(define-configuration/no-serialization apcupsd-event-handlers

>>>> + (killpower

>>>> + (gexp

>>>> + #~((wall "Apccontrol doing: apcupsd --killpower on UPS ~a" name)

>>>> + (sleep 10)

>>>> + (apcupsd "--killpower")

>>>> + (wall "Apccontrol has done: apcupsd --killpower on UPS ~a" name)))

>>>> + "Handler for killpower event.")

>>>

>

> By the way, why do we need to sleep 10 seconds here? It seems

> unnecessary.

> I just thought about it, perhaps it could be more concise to extract the

> field values using match-record (these are bound to shorter names, since

> they omit the 'apcupsd-configuration-' prefix).

>>>> + (let ((run-dir (apcupsd-configuration-run-dir cfg)))

>>>> + (mixed-text-file

>>>> + "apcupsd.conf"

>>>> + "\

>>>> +## apcupsd.conf v1.1 ##

>>>> +#

>>>> +# for apcupsd release 3.14.14 (31 May 2016) - GNU Guix

>>>

>>> Is this config really bound to a version like above? Unless they change

>>> things in backward incompatible ways, it should work for newer ones too,

>>> no?

>>

>> This file header is taken from the official configuration file

>> distributed with the apcupsd (except the GNU Guix part). So while I do

>> expect that this configuration file should work with newer version, the

>> upstream decided to put the version into the configuration file.

>>

>> I have no strong opinion either way here, should I just remove the file

>> header completely?

>

> I think so, since we're going the path of producing our own, we don't

> need to keep things that are not deemed useful.

>>>> + (match-record cfg <apcupsd-configuration>

>>>> + ( package pid-file debug-level run-dir

>>> ^ extraneous space

>>

>> This is intentional, it is neat little trick I have learned recently.

>> Without the extra leading space, Emacs indents it in this way:

>>

>> (match-record config <apcupsd-configuration>

>> (apcupsd pid-file debug-level run-dir

>> shepherd-base-name auto-start?)

>>

>>

>> However, with the extra leading space, the indentation changes to:

>>

>> (match-record config <apcupsd-configuration>

>> ( apcupsd pid-file debug-level run-dir

>> shepherd-base-name auto-start?)

>>

>>

>> Since in this case it is not a procedure call, but a list, first member

>> (apcupsd) is not special in any way. So it feels weird for it to have a

>> special priority regarding the indentation. With the extra leading

>> space, all list members are equal, as they should be.

>

> I see; that's an issue with our .dir-locals.el indentation rules. It

> should be fixed there, not worked around in actual code, though I can

> that workaround documented in 'info "(emacs) Lisp Indent"', for lists.

> In theory, we could define a function and assign it to the

> lisp-indent-function property (see .dir-locals.el), per 'C-h f

> scheme-indent-function' in Emacs to do whatever we like. While an

> interesting side project, I think using the workaround you propose is

> fine until then :-).

>

> [...]

>

>>> If you want, margin comments are ok without space or full sentence

>>> (punctuation), e.g.:

>>>

>>> ;do not daemonize

>>

>> I have no strong preference. What is the convention in Guix/Guile

>> community? Or is this down to each individual programmer?

>

> Our coding style in general comes from

> https://mumble.net/~campbell/scheme/style.txt (info '(guix) Formatting

> Code'), which mentions the above but leaves the choice. Since Ludo uses

> the above style, and authored most of the original code, it's the

> prevalent style in the code base and I suggest we stick to it, for

> consistency.

>

>> I have added:

>>

>> ;; The apcupsd can be configured to prevent users from logging in on certain

>> ;; conditions. This is implemented by creation of a "nologin" file, and

>> ;; using a pam nologin module to prevent the login (if the file exists).

>

> Thanks, that helps!

>

> I'll let you take care of the small remaining things, and probably the

> next revision will be good to go.

>> Deprecating /var/run to /run is not a Guix thing, but a File Hiearchy

>> Standard (FHS) thing [0]. Since we already use /run, there's no good

>> reason to have /var/run something else than a symlink to /run, and

>> there's an actual change pending merge that would do that, along making

>> /run mounted as a tmpfs (bug#73494).

>>

>> [0] https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch05s13.html

>

> I did not know we actually care about FHS (for example our /usr/bin

> definitely does not look like it should in that case :) ).

>

> I did the s|/var/run|/run|.

>>>> s/Slave/slave/ ? Could be nicer to use a neutral alternative like

>>>> 'follower machine'.

>>>

>>> The "Slave machine" (including the capitalization) is a term used in the

>>> manual. I can lower-case it if you would prefer, seems that manual

>>> actually uses both casings now when I looked for it.

>>>

>>> I do not think inventing new terminology is a good idea, since users

>>> will not be able to find it in the manual distributed with the program.

>>

>> The 'slave' word always carry a negative connotation. I think it's a

>> good idea to be proactive on modernizing the terminology where we can.

>

> In that case I would suggest to contact the upstream and convince them

> regarding this topic. Once that happens, we will be able to adjust the

> documentation with the next release (which would be free of offensive

> terminology).

>

> In any case, this is not a first occurrence of the word "slave" in the

> Guix repository nor manual.

>>> WARNING: (guile-user): imported module (gnu services) overrides core binding `delete'

>>>

>>> However, when done "my" way (putting srfi-1 first, since "stdlib"), the

>>> warning goes away.

>>

>> That's an interesting workaround, but I think we should look into that

>> problem and fix it definitely I often used #:hidden (delete) without any

>> issue it seems, so I'm not even sure if its existence still solves a

>> true problem in current Guile.

>

> I have recently (~few months back) tried to remove the `delete' and it a

> wall quickly, so I assume for reasons I do not understand it is still

> required.

>

> Having to use #:hidden (delete) on every single relevant import is in my

> opinion much worse solution than just ordering imports with srfi-1 at

> the top.

>

> But, I concur, it would be best it this just worked in any order.

> Anyway, I have sorted the modules as requested. I am not sure whether

> to sort

>

> #$@(apcupsd-event-handlers-modules

> (apcupsd-configuration-event-handlers cfg))

>

>

> Under `a' or `m' though. Opinion?

>>>>> +(define-configuration/no-serialization apcupsd-event-handlers

>>>>> + (killpower

>>>>> + (gexp

>>>>> + #~((wall "Apccontrol doing: apcupsd --killpower on UPS ~a" name)

>>>>> + (sleep 10)

>>>>> + (apcupsd "--killpower")

>>>>> + (wall "Apccontrol has done: apcupsd --killpower on UPS ~a" name)))

>>>>> + "Handler for killpower event.")

>>>>

>>

>> By the way, why do we need to sleep 10 seconds here? It seems

>> unnecessary.

>

> All default handlers are just conversions of the official apccontrol

> script, and it has the sleep in there. I am not sure why, so I have

> decided it would be better to leave it as it was.

>> I just thought about it, perhaps it could be more concise to extract the

>> field values using match-record (these are bound to shorter names, since

>> they omit the 'apcupsd-configuration-' prefix).

>

> If I got the idea right, you suggest converting

>

> ("emergency"

> #$@(apcupsd-event-handlers-emergency

> (apcupsd-configuration-event-handlers cfg)))

>

>

> into

>

> ("emergency"

> #$@(match-record cfg <apcupsd-configuration> (handlers)

> (match-record handlers <apcupsd-event-handlers> (emergency)

> emergency)))

>

>

> I am not sure it is increase in readability, but maybe I misunderstood

> the suggestion.

>>> This file header is taken from the official configuration file

>>> distributed with the apcupsd (except the GNU Guix part). So while I do

>>> expect that this configuration file should work with newer version, the

>>> upstream decided to put the version into the configuration file.

>>>

>>> I have no strong opinion either way here, should I just remove the file

>>> header completely?

>>

>> I think so, since we're going the path of producing our own, we don't

>> need to keep things that are not deemed useful.

>

> I have removed the version, but kept the header, so that there is

> reference to apcupsd-service-type left in the file.

> Getting bit off topic, but I am not sure how that would work. Between

> various macros, and Scheme being LISP-1, I have no idea how to write

> robust code that would just magically always indent "right".

>

> But I would be glad to be proven wrong. :)

> One additional change I have made is to change

>

> (requirement '())

>

>

> into

>

> (requirement '(user-processes))

>

> Without the dependency, the daemon got respawned by shepherd on shutdown

> and prevented the machine from going offline, which sucked a bit. ^_^

> I thought I would mention it so that you can keep it in mind for future

> code reviews.

> With that I think I am out of things that I want to change or would

> appreciate feedback on, so I am sending a v2. Thank you for working

> with me through this, the code is much better than in v1.

> * gnu/packages/power.scm (apcupsd): New variable.

> * gnu/local.mk (GNU_SYSTEM_MODULES): Register the new file.

> Hi,

>

> Tomas Volf <~@wolfsden.cz> writes:

>

>> * gnu/packages/power.scm (apcupsd): New variable.

>> * gnu/local.mk (GNU_SYSTEM_MODULES): Register the new file.

>

> Edited this one lightly:

>

> modified gnu/packages/power.scm

> @@ -115,8 +115,9 @@ (define-public apcupsd

> (inputs (list libusb libusb-compat))

> (home-page "http://www.apcupsd.org")

> (synopsis "Daemon for controlling APC UPSes")

> - (description "The apcupsd can be used for power management and controlling

> -most of APC’s UPS models on Unix and Windows machines. The apcupsd works with

> -most of APC’s Smart-UPS models as well as most simple signalling models such a

> + (description "@command{apcupsd} can be used for power management and

> +controlling most of @acronym{APC, American Power Conversion}’s @acronym{UPS,

> +Uninterruptible Power Supply} models. @command{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)))

>

> We do not mention Windows or other nonfree OS support in package

> descriptions as Guix doesn't run natively there anyway and we'd rather

> not advertise them.

>

> And pushed. I'm still waiting for a v3 for the service (if you can

> implement the match-record suggestion) to push 2/2.

> Will take a look (and hopefully send the patch) tomorrow, sorry for the

> delays. Other things got in the way.

> [..]

>>> I just thought about it, perhaps it could be more concise to extract the

>>> field values using match-record (these are bound to shorter names, since

>>> they omit the 'apcupsd-configuration-' prefix).

>>

>> If I got the idea right, you suggest converting

>>

>> ("emergency"

>> #$@(apcupsd-event-handlers-emergency

>> (apcupsd-configuration-event-handlers cfg)))

>>

>>

>> into

>>

>> ("emergency"

>> #$@(match-record cfg <apcupsd-configuration> (handlers)

>> (match-record handlers <apcupsd-event-handlers> (emergency)

>> emergency)))

>>

>>

>> I am not sure it is increase in readability, but maybe I misunderstood

>> the suggestion.

>

> I think something like this should work:

>

> modified gnu/services/power.scm

> @@ -494,119 +494,67 @@ (define-configuration apcupsd-configuration

> empty-serializer))

>

> (define (%apccontrol config)

> - (program-file

> - "apccontrol"

> - #~(begin

> - (use-modules (ice-9 format)

> - (ice-9 match)

> - (ice-9 popen)

> - (srfi srfi-9)

> - #$@(apcupsd-event-handlers-modules

> - (apcupsd-configuration-event-handlers config)))

> - ;; Script dir depends on these, and the configuration depends on the

> - ;; script dir. To sever the cyclic dependency, pass the file names via

> - ;; environment variables.

> - (define conf (getenv "GUIX_APCUPSD_CONF"))

> - (define powerfail-file (getenv "GUIX_APCUPSD_POWERFAIL_FILE"))

> -

> - (define (err . args)

> - (apply format (current-error-port) args))

> - (define (wall . args)

> - (system* #$(file-append util-linux "/bin/wall") (apply format #f args)))

> - (define (apcupsd . args)

> - (apply system* #$(file-append apcupsd "/sbin/apcupsd") "-f" conf args))

> - (define (mail-to-root subject body)

> - (let ((port (open-pipe* OPEN_WRITE

> - "/run/privileged/bin/sendmail"

> - "-F" "apcupsd"

> - "root")))

> - (format port "Subject: ~a~%~%~a~&" subject body)

> - (close-pipe port)))

> - (match (cdr (command-line))

> - (((? string? cmd) name connected powered)

> - (let ((connected? (match connected

> + (define event-handlers (apcupsd-configuration-event-handlers config))

> + (match-record event-handlers <apcupsd-event-handlers>

> + ( modules killpower commfailure commok powerout

> + onbattery offbattery ...)

> + (program-file

> + "apccontrol"

> + #~(begin

> + (use-modules (ice-9 format)

> + (ice-9 match)

> + (ice-9 popen)

> + (srfi srfi-9)

> + #$@modules)

> + ;; Script dir depends on these, and the configuration depends on the

> + ;; script dir. To sever the cyclic dependency, pass the file names via

> + ;; environment variables.

> + (define conf (getenv "GUIX_APCUPSD_CONF"))

> + (define powerfail-file (getenv "GUIX_APCUPSD_POWERFAIL_FILE"))

> +

> + (define (err . args)

> + (apply format (current-error-port) args))

> +

> + (define (wall . args)

> + (system* #$(file-append util-linux "/bin/wall") (apply format #f args)))

> +

> + (define (apcupsd . args)

> + (apply system* #$(file-append apcupsd "/sbin/apcupsd") "-f" conf args))

> +

> + (define (mail-to-root subject body)

> + (let ((port (open-pipe* OPEN_WRITE

> + "/run/privileged/bin/sendmail"

> + "-F" "apcupsd"

> + "root")))

> + (format port "Subject: ~a~%~%~a~&" subject body)

> + (close-pipe port)))

> +

> + (match (cdr (command-line))

> + (((? string? cmd) name connected powered)

> + (let ((connected? (match connected

> + ("1" #t)

> + ("0" #f)))

> + (powered? (match powered

> ("1" #t)

> - ("0" #f)))

> - (powered? (match powered

> - ("1" #t)

> - ("0" #f))))

> - (match cmd

> - ;; I am sure this could be done by macro, but meh. Last release

> - ;; of apcupsd was in 2016, so maintaining this will not be much

> - ;; work.

> - ("killpower"

> - #$@(apcupsd-event-handlers-killpower

> - (apcupsd-configuration-event-handlers config)))

> - ("commfailure"

> - #$@(apcupsd-event-handlers-commfailure

> - (apcupsd-configuration-event-handlers config)))

> - ("commok"

> - #$@(apcupsd-event-handlers-commok

> - (apcupsd-configuration-event-handlers config)))

> - ("powerout"

> - #$@(apcupsd-event-handlers-powerout

> - (apcupsd-configuration-event-handlers config)))

> - ("onbattery"

> - #$@(apcupsd-event-handlers-onbattery

> - (apcupsd-configuration-event-handlers config)))

> - ("offbattery"

> - #$@(apcupsd-event-handlers-offbattery

> - (apcupsd-configuration-event-handlers config)))

> - ("mainsback"

> - #$@(apcupsd-event-handlers-mainsback

> - (apcupsd-configuration-event-handlers config)))

> - ("failing"

> - #$@(apcupsd-event-handlers-failing

> - (apcupsd-configuration-event-handlers config)))

> - ("timeout"

> - #$@(apcupsd-event-handlers-timeout

> - (apcupsd-configuration-event-handlers config)))

> - ("loadlimit"

> - #$@(apcupsd-event-handlers-loadlimit

> - (apcupsd-configuration-event-handlers config)))

> - ("runlimit"

> - #$@(apcupsd-event-handlers-runlimit

> - (apcupsd-configuration-event-handlers config)))

> - ("doreboot"

> - #$@(apcupsd-event-handlers-doreboot

> - (apcupsd-configuration-event-handlers config)))

> - ("doshutdown"

> - #$@(apcupsd-event-handlers-doshutdown

> - (apcupsd-configuration-event-handlers config)))

> - ("annoyme"

> - #$@(apcupsd-event-handlers-annoyme

> - (apcupsd-configuration-event-handlers config)))

> - ("emergency"

> - #$@(apcupsd-event-handlers-emergency

> - (apcupsd-configuration-event-handlers config)))

> - ("changeme"

> - #$@(apcupsd-event-handlers-changeme

> - (apcupsd-configuration-event-handlers config)))

> - ("remotedown"

> - #$@(apcupsd-event-handlers-remotedown

> - (apcupsd-configuration-event-handlers config)))

> - ("startselftest"

> - #$@(apcupsd-event-handlers-startselftest

> - (apcupsd-configuration-event-handlers config)))

> - ("endselftest"

> - #$@(apcupsd-event-handlers-endselftest

> - (apcupsd-configuration-event-handlers config)))

> - ("battdetach"

> - #$@(apcupsd-event-handlers-battdetach

> - (apcupsd-configuration-event-handlers config)))

> - ("battattach"

> - #$@(apcupsd-event-handlers-battattach

> - (apcupsd-configuration-event-handlers config)))

> - (_

> - (err "Unknown event: ~a~%" cmd)

> - (err "Iff the event was emitted by apcupsd, this is a bug.~%")

> - (err "Please report to bug-guix@gnu.org.~%")

> - (exit #f)))))

> - (args

> - (err "Unknown arguments: ~a~%" args)

> - (err "Iff the arguments were passed by apcupsd, this is a bug.~%")

> - (err "Please report to bug-guix@gnu.org.~%")

> - (exit #f))))))

> + ("0" #f))))

> + (match cmd

> + ("killpower" #$killpower)

> + ("commfailure" #$commfailure)

> + ("commok" #$commok)

> + ("powerout" #$powerout)

> + ("onbattery" #$onbattery)

> + ("offbattery" #$offbattery)

> + [...]

> + (_

> + (err "Unknown event: ~a~%" cmd)

> + (err "If the event was emitted by apcupsd, this is a bug.~%")

> + (err "Please report to bug-guix@gnu.org.~%")

> + (exit #f)))))

> + (args

> + (err "Unknown arguments: ~a~%" args)

> + (err "If the arguments were passed by apcupsd, this is a bug.~%")

> + (err "Please report to bug-guix@gnu.org.~%")

> + (exit #f))))))))

>

> (define (apcupsd-script-dir config)

> (computed-file

>

> [...]

> [..]

> I'll let you work through my last match-record suggestion above, if you

> want and send a v3.

>

> I'm also attaching a fixup with the terminology changes I've submitted

> upstream if you'd like to validate it still works (I don't have any

> UPS to try it with myself).

> +(define apcupsd-service-type

> + (service-type

> + (name 'apcupsd)

> + (description "Configure and optionally start the apcupsd.")

> + (extensions (list (service-extension activation-service-type

> + apcupsd-activation)

> + (service-extension shepherd-root-service-type

> + apcupsd-shepherd-services)

> + (service-extension pam-root-service-type

> + apcupsd-pam-extensions)))

> + (compose identity)

> + (extend (lambda (cfg lst)

> + (fold (cut <> <>) cfg lst)))

> Hi Tomas,

>

> I was about to merge this, but I have one last (I promise!) question, see

> below.

>

> Tomas Volf <~@wolfsden.cz> writes:

>

> [...]

>

>> +(define apcupsd-service-type

>> + (service-type

>> + (name 'apcupsd)

>> + (description "Configure and optionally start the apcupsd.")

>> + (extensions (list (service-extension activation-service-type

>> + apcupsd-activation)

>> + (service-extension shepherd-root-service-type

>> + apcupsd-shepherd-services)

>> + (service-extension pam-root-service-type

>> + apcupsd-pam-extensions)))

>> + (compose identity)

>> + (extend (lambda (cfg lst)

>> + (fold (cut <> <>) cfg lst)))

>

> What does the above extend does? I find the (cut <> <>) particularly

> cryptic, as it's typically used to specialize procedures, which there

> are none above.

> Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

>

>> Hi Tomas,

>>

>> I was about to merge this, but I have one last (I promise!) question, see

>> below.

>

> No problem, I appreciate the review, it lead to higher code quality of

> this series.

>>

>> Tomas Volf <~@wolfsden.cz> writes:

>>

>> [...]

>>

>>> +(define apcupsd-service-type

>>> + (service-type

>>> + (name 'apcupsd)

>>> + (description "Configure and optionally start the apcupsd.")

>>> + (extensions (list (service-extension activation-service-type

>>> + apcupsd-activation)

>>> + (service-extension shepherd-root-service-type

>>> + apcupsd-shepherd-services)

>>> + (service-extension pam-root-service-type

>>> + apcupsd-pam-extensions)))

>>> + (compose identity)

>>> + (extend (lambda (cfg lst)

>>> + (fold (cut <> <>) cfg lst)))

>>

>> What does the above extend does? I find the (cut <> <>) particularly

>> cryptic, as it's typically used to specialize procedures, which there

>> are none above.

>

> I consider the behavior of the default #f value for extend field to be

> unfortunate, so I prefer to specify extend for all my services. Yet in

> this case there is no obvious thing to extend, so I opted for the

> extension to be a procedure, taking in the configuration and returning,

> possibly modified, configuration.

> The (cut <> <>) is just a handy, more concise way to write

> (lambda (proc arg) (proc arg)).

> What are your thoughts here? Should I just drop both compose and extend

> from here? Or is it fine like this?