GNU bug report logs - #75528
[PATCH 0/2] Add apcupsd

Previous Next

Full log

View this message in rfc822 format

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: [bug#75528] [PATCH 2/2] services: Add power.
Date: Sun, 16 Feb 2025 16:16:29 +0100

[Message part 1 (text/plain, inline)]

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> writes:

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

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

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

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

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.

Anyway, I have sorted the modules as requested.  I am not sure whether
to sort

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

Under `a' or `m' though.  Opinion?

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

> 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

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

into

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

I am not sure it is increase in readability, but maybe I misunderstood
the suggestion.

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

I have removed the version, but kept the header, so that there is
reference to apcupsd-service-type left in the file.

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

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

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

Sure, adjusted.

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

One additional change I have made is to change

--8<---------------cut here---------------start------------->8---
        (requirement '())
--8<---------------cut here---------------end--------------->8---

into

--8<---------------cut here---------------start------------->8---
        (requirement '(user-processes))
--8<---------------cut here---------------end--------------->8---

Without the dependency, the daemon got respawned by shepherd on shutdown
and prevented the machine from going offline, which sucked a bit. ^_^

I thought I would mention it so that you can keep it in mind for future
code reviews.

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.

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)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.