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: Sun, 16 Feb 2025 14:33:05 +0900
Hi Tomas,
Tomas Volf <~@wolfsden.cz> writes:
[...]
>> 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.
OK, that's reasonable, let's keep it (gnu services power).
> 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.
Haha, yes, the last point is a funny 'loophole' of the change log
format. Change a couple things to a package? Needs a lot of
description. Add a new package? Two words suffice :-). It's
reasonable though (but still funny).
[...]
>>> +
>>> +@item @code{auto-start?} (default: @code{#t}) (type: boolean)
>>> +Should the shepherd service auto-start?
>>> +
>>> +@item @code{pid-file} (default: @code{"/var/run/apcupsd.pid"}) (type: string)
>>> +Path to the pid file.
>>
>> All file names becoming /run/ prefixed instead of /var/run, based on a
>> previous comment.
>
> I will just quote myself from the package's patch, since it seems I did
> not get an answer there:
>
> It seems pretty much all programs on my system are using /var/run,
> including Shepherd. Only programs that store information in /run seem
> to be elogind and dbus.
>
> Is deprecation of /var/run listed somewhere in the manual? I am
> surprised Shepherd (since it is actively maintained and core component)
> is still in /var/run. I looked and found 42 references to /var/run, but
> none with the deprecation notice.
>
> Assuming you will insist on moving it to /run, is there some structure
> to follow in that directory? Or just s~/var/run~/run~? What about /var
> and /var/log? Should those be moved somewhere as well?
Deprecating /var/run to /run is not a Guix thing, but a File Hiearchy
Standard (FHS) thing [0]. Since we already use /run, there's no good
reason to have /var/run something else than a symlink to /run, and
there's an actual change pending merge that would do that, along making
/run mounted as a tmpfs (bug#73494).
[0] https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch05s13.html
>
> 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.".
nitpick: s/pid/PID/
> 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."?
Since a directory is implicitly a file, you can just say '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.
Opinions seem to vary on which style is best, but in Guix I'm pretty
sure we've stuck to what I've described for the most part until now, so
we should continue, for consistency.
[...]
> 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.
That's funny; the 'standards' Texinfo manual comes from autoconf, and is
the one documenting one. But autoconf is old, perhaps that guide line was
clarified later.
[...]
>> 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.
The 'slave' word always carry a negative connotation. I think it's a
good idea to be proactive on modernizing the terminology where we can.
[...]
>> 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.
I guess that could be explained somewhere along those lines:
- λ is used in code; newcomers could be confused as to what it means, or
have to copy-paste it around thinking it's required by the syntax to
define anonymous procedures (while it's not -- they can just type
'lambda').
- © is just a widely understood symbol used in a text comment.
>>> +
>>> +;;;; 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, ...)
That's similar to what I'd do in C or C++, except I'd reverse it so that
I can catch missing includes from my own files.
In Guix I've never read we do that. Perhaps it'd make sense. But
currently we typically globally sort lexicographically, I think (but
many modules imports are in a messy state).
> Given your requirement for sorting everything lexicographically (with no
> grouping), the following:
>
> (use-modules (gnu services)
> (srfi srfi-1))
>
> (when #f
> (modify-services '()
> (delete 'x)))
>
>
> Prints a warning when compiled:
>
> WARNING: (guile-user): imported module (gnu services) overrides core binding `delete'
>
> However, when done "my" way (putting srfi-1 first, since "stdlib"), the
> warning goes away.
That's an interesting workaround, but I think we should look into that
problem and fix it definitely I often used #:hidden (delete) without any
issue it seems, so I'm not even sure if its existence still solves a
true problem in current Guile.
[...]
>>> +(define-configuration/no-serialization apcupsd-event-handlers
>>> + (killpower
>>> + (gexp
>>> + #~((wall "Apccontrol doing: apcupsd --killpower on UPS ~a" name)
>>> + (sleep 10)
>>> + (apcupsd "--killpower")
>>> + (wall "Apccontrol has done: apcupsd --killpower on UPS ~a" name)))
>>> + "Handler for killpower event.")
>>
By the way, why do we need to sleep 10 seconds here? It seems unnecessary.
[...]
>> Sounds fun :-). I was wondering if the handlers could be of the
>> maybe-gexp type, and when not provided the handler would not be called
>> at all? Or does the design of apcupsd mandates that all handlers be
>> defined?
>
> The control script is called for all events. So I *could* use
> maybe-gexp, and change
>
> ("battattach"
> #$@(apcupsd-event-handlers-battattach
> (apcupsd-configuration-event-handlers cfg)))
>
>
> into
>
> ("battattach"
> #$@(let ((h (apcupsd-event-handlers-battattach
> (apcupsd-configuration-event-handlers cfg))))
> (if (maybe-value-set? h)
> h
> #~(#t))))
>
>
> and do that for all the events. But I have to admit I do not consider
> that an improvement. There probably is a better way to express that,
> but the current approach (of always having, possibly NOP, handler
> defined) helps keep the code straight forward.
OK; we could probably script the control script differently, but let's
keep what we have so far, it's simple and I doubt performance is an
issue ;-).
>>> +(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.
I'd stick to describe what they really are, e.g. for the enum-$name?
symbol, 'predicate-name' or 'predicate-variable-name' instead of
d/predicate. When deeply nested, it's OK to abbreviate the names a bit
if it becomes difficult to fit in our 80 columns width, so it could be
'pred-var-name' or the likes.
[...]
>>> + (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.
Which I've now covered in my reply above (sorry for missing in in my
first replies).
[...]
> 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.
I've replied above. I'd suggest to stick to lexicographically sort the
whole imports block to stick with current conventions (although I don't
think this particular one is documented).
>>> + ;; Script dir depends on these, and the configuration depends on the script
>>> + ;; dir. To sever the cyclic dependency, pass the paths via environment
>>> + ;; variables.
>>> + (define conf (getenv "GUIX_APCUPSD_CONF"))
>>> + (define powerfail-file (getenv "GUIX_APCUPSD_POWERFAIL_FILE"))
>>> +
>>> + (define (err . args)
>>> + (apply format (current-error-port) args))
>>> + (define (wall . args)
>>> + (system* #$(file-append util-linux "/bin/wall") (apply format #f args)))
>>> + (define (apcupsd . args)
>>> + (apply system* #$(file-append apcupsd "/sbin/apcupsd") "-f" conf args))
>>> + (define (mail-to-root subject body)
>>> + (let ((port (open-pipe* OPEN_WRITE
>>> + "/run/privileged/bin/sendmail"
>>> + "-F" "apcupsd"
>>> + "root")))
>>> + (format port "Subject: ~a~%~%~a~&" subject body)
>>> + (close-pipe port)))
>>> + (match (cdr (command-line))
>>> + (((? string? cmd) name connected powered)
>>> + (match cmd
>>> + ;; I am sure this could be done by macro, but meh. Last release of
>>> + ;; apcupsd was in 2016, so maintaining this will not be much work.
>>> + ("killpower"
>>> + #$@(apcupsd-event-handlers-killpower
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("commfailure"
>>> + #$@(apcupsd-event-handlers-commfailure
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("commok"
>>> + #$@(apcupsd-event-handlers-commok
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("powerout"
>>> + #$@(apcupsd-event-handlers-powerout
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("onbattery"
>>> + #$@(apcupsd-event-handlers-onbattery
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("offbattery"
>>> + #$@(apcupsd-event-handlers-offbattery
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("mainsback"
>>> + #$@(apcupsd-event-handlers-mainsback
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("failing"
>>> + #$@(apcupsd-event-handlers-failing
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("timeout"
>>> + #$@(apcupsd-event-handlers-timeout
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("loadlimit"
>>> + #$@(apcupsd-event-handlers-loadlimit
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("runlimit"
>>> + #$@(apcupsd-event-handlers-runlimit
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("doreboot"
>>> + #$@(apcupsd-event-handlers-doreboot
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("doshutdown"
>>> + #$@(apcupsd-event-handlers-doshutdown
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("annoyme"
>>> + #$@(apcupsd-event-handlers-annoyme
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("emergency"
>>> + #$@(apcupsd-event-handlers-emergency
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("changeme"
>>> + #$@(apcupsd-event-handlers-changeme
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("remotedown"
>>> + #$@(apcupsd-event-handlers-remotedown
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("startselftest"
>>> + #$@(apcupsd-event-handlers-startselftest
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("endselftest"
>>> + #$@(apcupsd-event-handlers-endselftest
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("battdetach"
>>> + #$@(apcupsd-event-handlers-battdetach
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + ("battattach"
>>> + #$@(apcupsd-event-handlers-battattach
>>> + (apcupsd-configuration-event-handlers cfg)))
>>> + (_
>>> + (err "Unknown event: ~a~%" cmd)
>>> + (err "Iff the event was passed by apcupsd, this is a bug.~%")
I just thought about it, perhaps it could be more concise to extract the
field values using match-record (these are bound to shorter names, since
they omit the 'apcupsd-configuration-' prefix).
[...]
>> I don't understand, in which scenario would an event *not* be emitted
>> by apcupsd?
>
> If you call the script manually from command line. ^_^
I see!
[...]
>>> + (let ((run-dir (apcupsd-configuration-run-dir cfg)))
>>> + (mixed-text-file
>>> + "apcupsd.conf"
>>> + "\
>>> +## apcupsd.conf v1.1 ##
>>> +#
>>> +# for apcupsd release 3.14.14 (31 May 2016) - GNU Guix
>>
>> Is this config really bound to a version like above? Unless they change
>> things in backward incompatible ways, it should work for newer ones too,
>> no?
>
> This file header is taken from the official configuration file
> distributed with the apcupsd (except the GNU Guix part). So while I do
> expect that this configuration file should work with newer version, the
> upstream decided to put the version into the configuration file.
>
> I have no strong opinion either way here, should I just remove the file
> header completely?
I think so, since we're going the path of producing our own, we don't
need to keep things that are not deemed useful.
>>> +#
>>> +# \"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.
Eheh. Good!
>>> +"
>>> + (serialize-configuration cfg apcupsd-configuration-fields)
>>> + ;; This one is confusing. The manual page states:
>>> + ;;
>>> + ;; > It must be changed when running more than one copy of apcupsd on the
>>> + ;; > same computer to control multiple UPSes.
>>> + ;;
>>> + ;; However would you not want the lock to be per-device, not per-process?
>>> + ;; I decided to follow the documentation, but I do not understand why it
>>> + ;; should be like this. I do not have multiple UPSes to try.
>>> + (serialize-string 'lock-dir (string-append run-dir "/lock"))
>>> + (serialize-string 'power-fail-dir run-dir)
>>> + (serialize-string 'no-login-dir run-dir)
>>> + (serialize-string 'stat-file (string-append run-dir "/apcupsd.status"))
>>> + "SCRIPTDIR " (apcupsd-script-dir cfg) "\n")))
>>> +
>>> +(define (apcupsd-shepherd-services cfg)
>>
>> s/cfg/config/
>
> And done.
>
>>
>>> + (match-record cfg <apcupsd-configuration>
>>> + ( package pid-file debug-level run-dir
>> ^ extraneous space
>
> This is intentional, it is neat little trick I have learned recently.
> Without the extra leading space, Emacs indents it in this way:
>
> (match-record config <apcupsd-configuration>
> (apcupsd pid-file debug-level run-dir
> shepherd-base-name auto-start?)
>
>
> However, with the extra leading space, the indentation changes to:
>
> (match-record config <apcupsd-configuration>
> ( apcupsd pid-file debug-level run-dir
> shepherd-base-name auto-start?)
>
>
> Since in this case it is not a procedure call, but a list, first member
> (apcupsd) is not special in any way. So it feels weird for it to have a
> special priority regarding the indentation. With the extra leading
> space, all list members are equal, as they should be.
I see; that's an issue with our .dir-locals.el indentation rules. It
should be fixed there, not worked around in actual code, though I can
that workaround documented in 'info "(emacs) Lisp Indent"', for lists.
In theory, we could define a function and assign it to the
lisp-indent-function property (see .dir-locals.el), per 'C-h f
scheme-indent-function' in Emacs to do whatever we like. While an
interesting side project, I think using the workaround you propose is
fine until then :-).
[...]
>> If you want, margin comments are ok without space or full sentence
>> (punctuation), e.g.:
>>
>> ;do not daemonize
>
> I have no strong preference. What is the convention in Guix/Guile
> community? Or is this down to each individual programmer?
Our coding style in general comes from
https://mumble.net/~campbell/scheme/style.txt (info '(guix) Formatting
Code'), which mentions the above but leaves the choice. Since Ludo uses
the above style, and authored most of the original code, it's the
prevalent style in the code base and I suggest we stick to it, for
consistency.
[...]
> I have added:
>
> ;; The apcupsd can be configured to prevent users from logging in on certain
> ;; conditions. This is implemented by creation of a "nologin" file, and
> ;; using a pam nologin module to prevent the login (if the file exists).
Thanks, that helps!
I'll let you take care of the small remaining things, and probably the
next revision will be good to go.
--
Thanks,
Maxim
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.