Subject: [bug#75528] [PATCH 2/2] services: Add power.

From: Tomas Volf <[email-masked]>
Date: Sat, 15 Feb 2025 22:59:57 +0100

Maxim Cournoyer <[email-masked]> writes:

> Hi Tomas,
>
> Tomas Volf <[email-masked]> 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?

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.

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

I added ones for UPS, OEM and 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).
>
> I'd say, (for example, the default @code{battery-level} of 5% may be
> considered too low by some), to keep it neutral.

Done.

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

I see, good to know, will try to adhere to that convention from now on.

>
>> +@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/

I will just take your word on this one. :)

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

--8<---------------cut here---------------start------------->8---
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?
--8<---------------cut here---------------end--------------->8---

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.

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

Done.

>
>> +@item usb
>> +A null string setting enables autodetection, which is the best choice
>> +for most installations.
>
> nitpick: My dictionary (aspell) prefers auto-detection.

Huh, so does hunspell.  Shame on me, I need to remember to enable
flyspell-prog-mode more often.

>
> [...]
>
>> +@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?

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

I like this version, thank you.

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

Done.

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

Hm, that is a good point.  I did not think of that.  I have added a
`modules' field to the apcupsd-event-handlers record.

>
>> +
>> +@table @code
>> +@item conf
>> +Variable containing path to the configuration file.
>
> s/path/file name/, anywhere this appears.

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.

>
>> +
>> +@item powerfail-file
>> +Variable containing path to the powerfail file.
>
> Ditto.

And done.

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

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

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

Done.

>
>> +;;; Copyright (C) 2025 Tomas Volf <[email-masked]>
>
> 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.

(But of course adjusted, it is just that I do not get the difference.)

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

--8<---------------cut here---------------start------------->8---
(use-modules (gnu services)
             (srfi srfi-1))

(when #f
  (modify-services '()
    (delete 'x)))
--8<---------------cut here---------------end--------------->8---

Prints a warning when compiled:

--8<---------------cut here---------------start------------->8---
WARNING: (guile-user): imported module (gnu services) overrides core binding `delete'
--8<---------------cut here---------------end--------------->8---

However, when done "my" way (putting srfi-1 first, since "stdlib"), the
warning goes away.

This is not really issue here, since I am not using modify-services, the
warning is not printed (I have no idea why).  But since in general order
of imports seem to matter, I wonder whether mandating global order is a
good idea.

What are your thoughts here?

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

Yeah I always have a problem with these.  My mother tongue does not have
this concept at all, so it is bit hard for me.  Thank you for the link,
it was useful.  I went through the texts and tried to add them where I
felt they were missing, you are right, there were a lot of them missing.

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

The control script is called for all events.  So I *could* use
maybe-gexp, and change

--8<---------------cut here---------------start------------->8---
	      ("battattach"
	       #$@(apcupsd-event-handlers-battattach
		   (apcupsd-configuration-event-handlers cfg)))
--8<---------------cut here---------------end--------------->8---

into

--8<---------------cut here---------------start------------->8---
	      ("battattach"
	       #$@(let ((h (apcupsd-event-handlers-battattach
		            (apcupsd-configuration-event-handlers cfg))))
                    (if (maybe-value-set? h)
                        h
                        #~(#t))))
--8<---------------cut here---------------end--------------->8---

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.

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

Done.

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

Done.

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

>
>> +   "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).
>
> [...]

Done (as part of the texi modification).

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

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.~%")
>
> s/Iff/If/,

I think `Iff' is used correctly here, no?

> perhaps s/passed/emitted/.

Sure, why not.

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

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

>
>> +             (err "Please report to [email-masked].~%")
>> +             (exit #f))))
>> +         (args
>> +          (err "Unknown arguments: ~a~%" args)
>> +          (err "Iff the arguments were passed by apcupsd, this is a bug.~%")
>
> s/Iff/If/

Same here.

>> +          (err "Please report to [email-masked].~%")
>> +          (exit #f))))))
>> +
>> +(define (apcupsd-script-dir cfg)
>
> s/cfg/config/

Done.

>
>> +  (computed-file
>> +   "apcupsd-script-dir"
>> +   #~(begin
>> +       (mkdir #$output)
>> +       (chdir #$output)
>> +       (symlink #$(s/apccontrol cfg) "apccontrol"))))
>> +
>> +(define (apcupsd-config-file cfg)
>
> s/cfg/config/

Done.

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

--8<---------------cut here---------------start------------->8---
  (match-record config <apcupsd-configuration>
                (apcupsd pid-file debug-level run-dir
                         shepherd-base-name auto-start?)
--8<---------------cut here---------------end--------------->8---

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

--8<---------------cut here---------------start------------->8---
  (match-record config <apcupsd-configuration>
                ( apcupsd pid-file debug-level run-dir
                  shepherd-base-name auto-start?)
--8<---------------cut here---------------end--------------->8---

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.

>
>> +                  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/' ?

I removed them completely, with activation-service-type they are not
needed at all now.

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

The reason is simple, I forgot that activation-service-type exists. ^_^
I have converted the code to use it.

>
>> +       (shepherd-service
>> +        (documentation "Run apcupsd daemon.")
>
> Run *the* apcupsd daemon.

Done.

>
>> +        (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.:
>
>                ;do not daemonize

I have no strong preference.  What is the convention in Guix/Guile
community?  Or is this down to each individual programmer?

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

Done.

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

I have added:

--8<---------------cut here---------------start------------->8---
  ;; 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).
--8<---------------cut here---------------end--------------->8---

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

Nice, thanks for catching it.  I am aware, but sometimes I miss one or
two when copying code from my channel.

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

And thank you for the very thorough review.  It is mostly details, but
it still surprised me how many I have managed to get wrong.

Thanks,
Tomas

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