Package: guix-patches;
Reported by: Tomas Volf <~@wolfsden.cz>
Date: Sun, 12 Jan 2025 23:04:01 UTC
Severity: normal
Tags: patch
Done: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Bug is archived. No further changes may be made.
Message #41 received at 75528 <at> debbugs.gnu.org (full text, mbox):
From: Tomas Volf <~@wolfsden.cz> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 75528 <at> debbugs.gnu.org, Ludovic Courtès <ludo <at> gnu.org> Subject: Re: [bug#75528] [PATCH 2/2] services: Add power. Date: Sat, 15 Feb 2025 22:59:57 +0100
[Message part 1 (text/plain, inline)]
Maxim Cournoyer <maxim.cournoyer <at> gmail.com> writes:
> 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?
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 <~@wolfsden.cz>
>
> 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 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/
Same here.
>> + (err "Please report to bug-guix <at> gnu.org.~%")
>> + (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.
[signature.asc (application/pgp-signature, inline)]
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.