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 #71 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: Sun, 23 Feb 2025 22:21:35 +0100
[Message part 1 (text/plain, inline)]
Maxim Cournoyer <maxim.cournoyer <at> gmail.com> writes:
> [..]
>>> 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:
>
> 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
>
> [...]
I get the idea now, thank you. Yes, that is *much* nicer. Updated.
> [..]
> I'll let you work through my last match-record suggestion above, if you
> want and send a v3.
Incorporated and v3 sent. This time a single patch, since the package
was already merged.
>
> 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).
Thank you, but I will pass on that. I respect your choice to invest
time into this effort, but for me this issue it not high enough on my
priority list to do the same.
Have a nice day,
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.