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 #32 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 Subject: Re: [bug#75528] [PATCH 1/2] gnu: Add apcupsd. Date: Thu, 13 Feb 2025 15:01:12 +0900
Hi Tomas,
Tomas Volf <~@wolfsden.cz> writes:
[...]
>> I think /var/run is deprecated in favor of just /run, so we should
>> configure the package to use this, I think.
>
> 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?
>
>>
>>> + (string-append "ac_cv_path_SHUTDOWN=/nope")
>>> + (string-append "ac_cv_path_APCUPSD_MAIL=/nope")
>>> + ;; While `wall' is not expanded anywhere, it still is searched for.
>>> + (string-append "ac_cv_path_WALL=/nope")
>>
>> I'm not sure if this package is actively developed, but that last issue
>> should ideally be reported (then cross-referenced here).
>
> I did not found a way to report a bug. Nothing is listed in the manual,
> and their apcupsd-users mailing list requires JavaScript to sign up (and
> does not tell me the email address otherwise).
>
> I did try to blindly send an email, will see if it will be accepted
> without being subscribed. If that happens, I will add the link here.
OK, thank you.
>>
>>> + ;; Enable additional drivers.
>>> + "--enable-test"
>>
>> Is '--enable-test' useful? What does it do?
>
> According to the manual
>
> This turns on a test driver that is used only for debugging.
>
>
> Since there does not seem to be any harm in having it on, I enabled it,
> since Guix seems to aim for feature complete packages (I assume that is
> the reason for everything being so large). But I have no use for it, so
> I can turn it off if you would prefer.
For this kind of very edge case that is not enabled by default, I'd
leave it off. Our maximalist take on things doesn't including switching
on every non-default option, but if extra dependencies are automatically
picked as part of the configure script, then we would typically add
them, unless they grow the 'guix size' of the package unreasonably (like
adding libreoffice to emacs-org-mode because it has support for it :-)).
It's one of the reason. Other reasons are references kept erroneously,
static libraries, large documentation, wrappers keeping references to
native inputs that shouldn't be captured, etc. There's a lot of work to
do to reduce the size of the distribution :-).
[...]
>>> + (add-before 'configure 'touch-txt-docs
>>> + (lambda _
>>> + (for-each (lambda (f)
>>> + (call-with-output-file f close-port))
>>> + '("doc/apcupsd.man.txt"
>>> + "doc/apcaccess.man.txt"
>>> + "doc/apctest.man.txt"
>>> + "doc/apccontrol.man.txt"
>>> + "doc/apcupsd.conf.man.txt"))))
>>
>> I think I'd rather depend on these than introduce hacks like below.
>
> The "hacks" *below* would still be required. The HTML manual is not
> built nor installed by default, and there is no target to invoke to
> install them. So both 'build-manual and 'move-doc phases would still be
> required.
>
> We could get rid of the "hack" *above* ('touch-txt-docs), true. It is
> worth those two additional inputs?
Yes, I meant above, sorry. I don't see mandoc and util-linux as large
dependencies enough to justify working around these, especially if they
are only used at build time. And, the would actually be useful when
working on the source of acupsd in a 'guix shell -D acupsd' environment,
so I'd just add them.
[...]
>>
>>> + (add-after 'install 'remove-etc-apcupsd
>>> + (lambda _
>>> + (delete-file-recursively (string-append #$output "/etc/apcupsd")))))))
>>
>> Break this long line so it fits under 80 columns (our code style
>> guideline).
>
> I thought (well, still do), that going 4 characters over the limit was
> worth the increase in readability. Nevertheless, I have split it into
> two lines.
Thanks. In some cases where it really hurts readability, it may be fine
(it's a guideline after all), but the usual business of simply moving
one parentheses pair down is not one of these :-).
[...]
> I do not think this is correct. When I check
> apcupsd-3.14.14/src/apcupsd.c file, I see this in its header:
[...]
> There is no mention of "any later version". Does that not make the
> combined work GPL-2.0-only, even if other files would be under
> GPL-2.0-or-later (I did no check whether they are)?
You are right, and yes the combined work would be GPLv2 only then.
> I did not find debian/copyright file in the source code.
There's one at least in
https://sourceforge.net/projects/apcupsd/files/apcupsd%20-%20Stable/3.14.14/apcupsd-3.14.14.tar.gz/download
[...]
>> Could you please send a v2?
>
> Definitely, I will go through your review for the service file as well,
> and once I have all the answers (both here and there), will send v2.
I haven't seen v2 yet, I guess you are still working out the service part?
--
Thanks,
Maxim
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.