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 #56 received at 75528 <at> debbugs.gnu.org (full text, mbox):
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: Re: [bug#75528] [PATCH 2/2] services: Add power. Date: Tue, 18 Feb 2025 15:51:06 +0900
[Message part 1 (text/plain, inline)]
Hi Tomas,
Tomas Volf <~@wolfsden.cz> writes:
[...]
>> Deprecating /var/run to /run is not a Guix thing, but a File Hiearchy
>> Standard (FHS) thing [0]. Since we already use /run, there's no good
>> reason to have /var/run something else than a symlink to /run, and
>> there's an actual change pending merge that would do that, along making
>> /run mounted as a tmpfs (bug#73494).
>>
>> [0] https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch05s13.html
>
> I did not know we actually care about FHS (for example our /usr/bin
> definitely does not look like it should in that case :) ).
>
> I did the s|/var/run|/run|.
We typically do not follow FHS much due to each of our packages being
installed under their own prefix, but for the few things where we do,
like here, I think it makes sense to stick to it, as it brings
familiarity and consistency to our users.
>>>> s/Slave/slave/ ? Could be nicer to use a neutral alternative like
>>>> 'follower machine'.
>>>
>>> The "Slave machine" (including the capitalization) is a term used in the
>>> manual. I can lower-case it if you would prefer, seems that manual
>>> actually uses both casings now when I looked for it.
>>>
>>> I do not think inventing new terminology is a good idea, since users
>>> will not be able to find it in the manual distributed with the program.
>>
>> The 'slave' word always carry a negative connotation. I think it's a
>> good idea to be proactive on modernizing the terminology where we can.
>
> In that case I would suggest to contact the upstream and convince them
> regarding this topic. Once that happens, we will be able to adjust the
> documentation with the next release (which would be free of offensive
> terminology).
>
> In any case, this is not a first occurrence of the word "slave" in the
> Guix repository nor manual.
That there are currently other occurrences is not a good reason to add
more, in my opinion :-). That said, I've looked at upstream sources and
it's true that the old master/slave terminology is currently deeply
interwined in code and doc... so I did an experiment and came up with
<https://codeberg.org/apteryx/acupsd/pulls/1>. I also forwarded the
svn diff to upstream via their mailing list. We'll see what they think.
[...]
>>> WARNING: (guile-user): imported module (gnu services) overrides core binding `delete'
>>>
>>> However, when done "my" way (putting srfi-1 first, since "stdlib"), the
>>> warning goes away.
>>
>> That's an interesting workaround, but I think we should look into that
>> problem and fix it definitely I often used #:hidden (delete) without any
>> issue it seems, so I'm not even sure if its existence still solves a
>> true problem in current Guile.
>
> I have recently (~few months back) tried to remove the `delete' and it a
> wall quickly, so I assume for reasons I do not understand it is still
> required.
>
> Having to use #:hidden (delete) on every single relevant import is in my
> opinion much worse solution than just ordering imports with srfi-1 at
> the top.
>
> But, I concur, it would be best it this just worked in any order.
OK, good to know, but let's keep this out of scope for the context of
this change.
> Anyway, I have sorted the modules as requested. I am not sure whether
> to sort
>
> #$@(apcupsd-event-handlers-modules
> (apcupsd-configuration-event-handlers cfg))
>
>
> Under `a' or `m' though. Opinion?
I'm not sure I understood the question, but under `a' makes more sense
to me (which `m' are you referring to?).
>>>>> +(define-configuration/no-serialization apcupsd-event-handlers
>>>>> + (killpower
>>>>> + (gexp
>>>>> + #~((wall "Apccontrol doing: apcupsd --killpower on UPS ~a" name)
>>>>> + (sleep 10)
>>>>> + (apcupsd "--killpower")
>>>>> + (wall "Apccontrol has done: apcupsd --killpower on UPS ~a" name)))
>>>>> + "Handler for killpower event.")
>>>>
>>
>> By the way, why do we need to sleep 10 seconds here? It seems
>> unnecessary.
>
> All default handlers are just conversions of the official apccontrol
> script, and it has the sleep in there. I am not sure why, so I have
> decided it would be better to leave it as it was.
OK, I guess it's safer to keep it.
>> I just thought about it, perhaps it could be more concise to extract the
>> field values using match-record (these are bound to shorter names, since
>> they omit the 'apcupsd-configuration-' prefix).
>
> If I got the idea right, you suggest converting
>
> ("emergency"
> #$@(apcupsd-event-handlers-emergency
> (apcupsd-configuration-event-handlers cfg)))
>
>
> into
>
> ("emergency"
> #$@(match-record cfg <apcupsd-configuration> (handlers)
> (match-record handlers <apcupsd-event-handlers> (emergency)
> emergency)))
>
>
> I am not sure it is increase in readability, but maybe I misunderstood
> the suggestion.
I think something like this should work:
--8<---------------cut here---------------start------------->8---
modified gnu/services/power.scm
@@ -494,119 +494,67 @@ (define-configuration apcupsd-configuration
empty-serializer))
(define (%apccontrol config)
- (program-file
- "apccontrol"
- #~(begin
- (use-modules (ice-9 format)
- (ice-9 match)
- (ice-9 popen)
- (srfi srfi-9)
- #$@(apcupsd-event-handlers-modules
- (apcupsd-configuration-event-handlers config)))
- ;; Script dir depends on these, and the configuration depends on the
- ;; script dir. To sever the cyclic dependency, pass the file names via
- ;; environment variables.
- (define conf (getenv "GUIX_APCUPSD_CONF"))
- (define powerfail-file (getenv "GUIX_APCUPSD_POWERFAIL_FILE"))
-
- (define (err . args)
- (apply format (current-error-port) args))
- (define (wall . args)
- (system* #$(file-append util-linux "/bin/wall") (apply format #f args)))
- (define (apcupsd . args)
- (apply system* #$(file-append apcupsd "/sbin/apcupsd") "-f" conf args))
- (define (mail-to-root subject body)
- (let ((port (open-pipe* OPEN_WRITE
- "/run/privileged/bin/sendmail"
- "-F" "apcupsd"
- "root")))
- (format port "Subject: ~a~%~%~a~&" subject body)
- (close-pipe port)))
- (match (cdr (command-line))
- (((? string? cmd) name connected powered)
- (let ((connected? (match connected
+ (define event-handlers (apcupsd-configuration-event-handlers config))
+ (match-record event-handlers <apcupsd-event-handlers>
+ ( modules killpower commfailure commok powerout
+ onbattery offbattery ...)
+ (program-file
+ "apccontrol"
+ #~(begin
+ (use-modules (ice-9 format)
+ (ice-9 match)
+ (ice-9 popen)
+ (srfi srfi-9)
+ #$@modules)
+ ;; Script dir depends on these, and the configuration depends on the
+ ;; script dir. To sever the cyclic dependency, pass the file names via
+ ;; environment variables.
+ (define conf (getenv "GUIX_APCUPSD_CONF"))
+ (define powerfail-file (getenv "GUIX_APCUPSD_POWERFAIL_FILE"))
+
+ (define (err . args)
+ (apply format (current-error-port) args))
+
+ (define (wall . args)
+ (system* #$(file-append util-linux "/bin/wall") (apply format #f args)))
+
+ (define (apcupsd . args)
+ (apply system* #$(file-append apcupsd "/sbin/apcupsd") "-f" conf args))
+
+ (define (mail-to-root subject body)
+ (let ((port (open-pipe* OPEN_WRITE
+ "/run/privileged/bin/sendmail"
+ "-F" "apcupsd"
+ "root")))
+ (format port "Subject: ~a~%~%~a~&" subject body)
+ (close-pipe port)))
+
+ (match (cdr (command-line))
+ (((? string? cmd) name connected powered)
+ (let ((connected? (match connected
+ ("1" #t)
+ ("0" #f)))
+ (powered? (match powered
("1" #t)
- ("0" #f)))
- (powered? (match powered
- ("1" #t)
- ("0" #f))))
- (match cmd
- ;; I am sure this could be done by macro, but meh. Last release
- ;; of apcupsd was in 2016, so maintaining this will not be much
- ;; work.
- ("killpower"
- #$@(apcupsd-event-handlers-killpower
- (apcupsd-configuration-event-handlers config)))
- ("commfailure"
- #$@(apcupsd-event-handlers-commfailure
- (apcupsd-configuration-event-handlers config)))
- ("commok"
- #$@(apcupsd-event-handlers-commok
- (apcupsd-configuration-event-handlers config)))
- ("powerout"
- #$@(apcupsd-event-handlers-powerout
- (apcupsd-configuration-event-handlers config)))
- ("onbattery"
- #$@(apcupsd-event-handlers-onbattery
- (apcupsd-configuration-event-handlers config)))
- ("offbattery"
- #$@(apcupsd-event-handlers-offbattery
- (apcupsd-configuration-event-handlers config)))
- ("mainsback"
- #$@(apcupsd-event-handlers-mainsback
- (apcupsd-configuration-event-handlers config)))
- ("failing"
- #$@(apcupsd-event-handlers-failing
- (apcupsd-configuration-event-handlers config)))
- ("timeout"
- #$@(apcupsd-event-handlers-timeout
- (apcupsd-configuration-event-handlers config)))
- ("loadlimit"
- #$@(apcupsd-event-handlers-loadlimit
- (apcupsd-configuration-event-handlers config)))
- ("runlimit"
- #$@(apcupsd-event-handlers-runlimit
- (apcupsd-configuration-event-handlers config)))
- ("doreboot"
- #$@(apcupsd-event-handlers-doreboot
- (apcupsd-configuration-event-handlers config)))
- ("doshutdown"
- #$@(apcupsd-event-handlers-doshutdown
- (apcupsd-configuration-event-handlers config)))
- ("annoyme"
- #$@(apcupsd-event-handlers-annoyme
- (apcupsd-configuration-event-handlers config)))
- ("emergency"
- #$@(apcupsd-event-handlers-emergency
- (apcupsd-configuration-event-handlers config)))
- ("changeme"
- #$@(apcupsd-event-handlers-changeme
- (apcupsd-configuration-event-handlers config)))
- ("remotedown"
- #$@(apcupsd-event-handlers-remotedown
- (apcupsd-configuration-event-handlers config)))
- ("startselftest"
- #$@(apcupsd-event-handlers-startselftest
- (apcupsd-configuration-event-handlers config)))
- ("endselftest"
- #$@(apcupsd-event-handlers-endselftest
- (apcupsd-configuration-event-handlers config)))
- ("battdetach"
- #$@(apcupsd-event-handlers-battdetach
- (apcupsd-configuration-event-handlers config)))
- ("battattach"
- #$@(apcupsd-event-handlers-battattach
- (apcupsd-configuration-event-handlers config)))
- (_
- (err "Unknown event: ~a~%" cmd)
- (err "Iff the event was emitted by apcupsd, this is a bug.~%")
- (err "Please report to bug-guix <at> gnu.org.~%")
- (exit #f)))))
- (args
- (err "Unknown arguments: ~a~%" args)
- (err "Iff the arguments were passed by apcupsd, this is a bug.~%")
- (err "Please report to bug-guix <at> gnu.org.~%")
- (exit #f))))))
+ ("0" #f))))
+ (match cmd
+ ("killpower" #$killpower)
+ ("commfailure" #$commfailure)
+ ("commok" #$commok)
+ ("powerout" #$powerout)
+ ("onbattery" #$onbattery)
+ ("offbattery" #$offbattery)
+ [...]
+ (_
+ (err "Unknown event: ~a~%" cmd)
+ (err "If the event was emitted by apcupsd, this is a bug.~%")
+ (err "Please report to bug-guix <at> gnu.org.~%")
+ (exit #f)))))
+ (args
+ (err "Unknown arguments: ~a~%" args)
+ (err "If the arguments were passed by apcupsd, this is a bug.~%")
+ (err "Please report to bug-guix <at> gnu.org.~%")
+ (exit #f))))))))
(define (apcupsd-script-dir config)
(computed-file
--8<---------------cut here---------------end--------------->8---
[...]
>>> This file header is taken from the official configuration file
>>> distributed with the apcupsd (except the GNU Guix part). So while I do
>>> expect that this configuration file should work with newer version, the
>>> upstream decided to put the version into the configuration file.
>>>
>>> I have no strong opinion either way here, should I just remove the file
>>> header completely?
>>
>> I think so, since we're going the path of producing our own, we don't
>> need to keep things that are not deemed useful.
>
> I have removed the version, but kept the header, so that there is
> reference to apcupsd-service-type left in the file.
Sounds reasonable.
[...]
> Getting bit off topic, but I am not sure how that would work. Between
> various macros, and Scheme being LISP-1, I have no idea how to write
> robust code that would just magically always indent "right".
>
> But I would be glad to be proven wrong. :)
I guess you could have a procedure that calls the existing indenting
Elisp functions, and then have a post-processing pass on top to fix it
the way wa want. It's probably a bit over the top :-). I like
your/Emacs' down to Earth solution.
[...]
> One additional change I have made is to change
>
> (requirement '())
>
>
> into
>
> (requirement '(user-processes))
>
> Without the dependency, the daemon got respawned by shepherd on shutdown
> and prevented the machine from going offline, which sucked a bit. ^_^
Oh wow; good catch. That seems a rather large foot gun to have for
service writers. I wonder if something could be done about it in the
Shepherd.
> I thought I would mention it so that you can keep it in mind for future
> code reviews.
Indeed, thanks for bringing this to my attention.
> With that I think I am out of things that I want to change or would
> appreciate feedback on, so I am sending a v2. Thank you for working
> with me through this, the code is much better than in v1.
I'll let you work through my last match-record suggestion above, if you
want and send a v3.
I'm also attaching a fixup with the terminology changes I've submitted
upstream if you'd like to validate it still works (I don't have any
UPS to try it with myself).
[0001-fixup-services-Add-power.patch (text/x-patch, attachment)]
[Message part 3 (text/plain, inline)]
Thank you for your patience! -- Thanks, Maxim
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.