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, Ludovic Courtès <ludo <at> gnu.org> Subject: [bug#75528] [PATCH 2/2] services: Add power. Date: Wed, 05 Feb 2025 22:39:33 +0900
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/
aaa
> +
> +@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 "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."))
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?
> +(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.
> + (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))))))))
That is a nice minimal enum for using with define-configuration. Note
that Guile has some kind of very limited enum from (rnrs enums), but I
don't think they'd be too useful here. See for example
'make-enumeration' from (rnrs enums), which I used in for the
ntp-server-types in (gnu services networking).
> +(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")))
You'll want to break the above line.
> +
> +(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.")
Please stick to the convention where the name of the package is the name
of the field used to specify such package (here: apcupsd).
> + (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)
> + "Path to the pid file."
I'm repeating myself because I reviewed the texi produced from this, but
yeah, s/path/file name/, and make sure to adjust the rest of the
comments I made for the .texi in the source (I'd just edit the source
and regenerate the doc from it).
[...]
> +(define (s/apccontrol cfg)
what is the 's/' prefix for in the procedure name? 'serialize'? Spell
this out in full to be clear rather than cryptic (that's a convention
followed at large in general in Scheme).
> + (program-file
> + "apccontrol"
> + #~(begin
> + (use-modules (srfi srfi-9)
> + (ice-9 format)
> + (ice-9 match)
> + (ice-9 popen))
Please sort modules lexicographically.
> + ;; 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.~%")
s/Iff/If/, perhaps s/passed/emitted/. I don't understand, in which scenario
would an event *not* be emitted by apcupsd?
> + (err "Please report to bug-guix <at> gnu.org.~%")
> + (exit #f))))
> + (args
> + (err "Unknown arguments: ~a~%" args)
> + (err "Iff the arguments were passed by apcupsd, this is a bug.~%")
s/Iff/If/
> + (err "Please report to bug-guix <at> gnu.org.~%")
> + (exit #f))))))
> +
> +(define (apcupsd-script-dir cfg)
s/cfg/config/
> + (computed-file
> + "apcupsd-script-dir"
> + #~(begin
> + (mkdir #$output)
> + (chdir #$output)
> + (symlink #$(s/apccontrol cfg) "apccontrol"))))
> +
> +(define (apcupsd-config-file cfg)
s/cfg/config/
> + (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?
> +#
> +# \"apcupsd\" POSIX config file (crafted via apcupsd-service-type)
s/crafted via/auto-generated by/
> +"
> + (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/
> + (match-record cfg <apcupsd-configuration>
> + ( package pid-file debug-level run-dir
^ extraneous space
> + shepherd-base-name auto-start?)
> + (let* ((config-file (apcupsd-config-file cfg))
> + (s/ shepherd-base-name)
Please improve the variables names a bit; what is 's/' ?
> + (s/run-dirs (string->symbol (format #f "~a-run-dirs" s/))))
> + (list
> + (shepherd-service
> + (documentation "Create the run directories.")
> + (provision (list s/run-dirs))
> + (one-shot? #t)
> + (auto-start? auto-start?)
> + (start #~(lambda _
> + ((@ (guix build utils) mkdir-p)
> + #$(string-append run-dir "/lock"))
> + #t)))
Is there a reason why this is not done in an activation script instead?
That's more conventional, I think.
> + (shepherd-service
> + (documentation "Run apcupsd daemon.")
Run *the* apcupsd daemon.
> + (requirement (list s/run-dirs))
> + (provision (list s/))
> + (auto-start? auto-start?)
> + (start #~(make-forkexec-constructor
> + '(#$(file-append package "/sbin/apcupsd")
> + "-b" ; Do not daemonize.
If you want, margin comments are ok without space or full sentence
(punctuation), e.g.:
--8<---------------cut here---------------start------------->8---
;do not daemonize
--8<---------------cut here---------------end--------------->8---
> + "-f" #$config-file
> + "-P" #$pid-file
> + "-d" #$(number->string debug-level))
> + #:log-file
> + #$(format #f "/var/log/~a.log" shepherd-base-name)
> + #:environment-variables
> + (cons* (string-append "GUIX_APCUPSD_CONF="
> + #$config-file)
> + #$(string-append "GUIX_APCUPSD_POWERFAIL_FILE="
> + run-dir "/powerfail")
> + (default-environment-variables))))
> + (stop #~(make-kill-destructor))
> + (actions (list (shepherd-configuration-action config-file))))))))
> +
> +(define (apcupsd-pam-extensions cfg)
s/cfg/config/
> + (define pam-nologin
A comment explaining why this PAM extention is required would be helpful
(I have no idea myself -- but I must confess: PAM is still a bit of a
mystery to me).
> + (pam-entry
> + (control "required")
> + (module "pam_nologin.so")
> + (arguments (list (string-append "file="
> + (apcupsd-configuration-run-dir cfg)
> + "/nologin")))))
> +
> + (list (pam-extension
> + (transformer
> + (lambda (pam)
> + (pam-service
> + (inherit pam)
> + (auth (cons pam-nologin (pam-service-auth pam)))))))))
> +
> +(define apcupsd-service-type
> + (service-type
> + (name 'apcupsd)
> + (description "Configure and optionally start apcupsd.")
> + (extensions (list (service-extension shepherd-root-service-type
> + apcupsd-shepherd-services)
> + (service-extension pam-root-service-type
> + apcupsd-pam-extensions)))
> + (compose identity)
> + (extend (λ (cfg lst)
nitpick: There was a stylistic choice to prefer lambda instead of λ in
the Guix code base to try to make it more accessible at some point.
> + (fold (cut <> <>) cfg lst)))
> + (default-value (apcupsd-configuration))))
Thanks for this very complete, carefully and thoughtfully crafted
contribution. As you can see, the only comments I had were mostly
nitpicks and small details. I look forward to the next revision.
--
Thanks,
Maxim
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.