[bug#75528,1/2] gnu: Add apcupsd.

* gnu/packages/power.scm (apcupsd): New variable.

Change-Id: I74f59cd1fa2a13954117ff1683a10a84576cc839
---
 gnu/local.mk           |   1 +
 gnu/packages/power.scm | 125 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 126 insertions(+)
 create mode 100644 gnu/packages/power.scm
  

Hi Tomas,

Tomas Volf <~@wolfsden.cz> writes:

> * gnu/packages/power.scm (apcupsd): New variable.

'guix lint' says:

--8<---------------cut here---------------start------------->8---
gnu/packages/power.scm:120:14: apcupsd@3.14.14: no article allowed at the beginning of the synopsis
gnu/packages/power.scm:119:15: apcupsd@3.14.14: TLS certificate error: X.509 server certificate for 'www.apcupsd.org' does not match: CN=sf.net
--8<---------------cut here---------------end--------------->8---

Please fix these.

> Change-Id: I74f59cd1fa2a13954117ff1683a10a84576cc839
> ---
>  gnu/local.mk           |   1 +
>  gnu/packages/power.scm | 125 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 126 insertions(+)
>  create mode 100644 gnu/packages/power.scm
>
> diff --git a/gnu/local.mk b/gnu/local.mk
> index 855f2acccc..6ca7bf68ac 100644
> --- a/gnu/local.mk
> +++ b/gnu/local.mk
> @@ -557,6 +557,7 @@ GNU_SYSTEM_MODULES =				\
>    %D%/packages/polkit.scm			\
>    %D%/packages/popt.scm				\
>    %D%/packages/potassco.scm				\
> +  %D%/packages/power.scm			\

This change should be mentioned in the change log as well, e.g.:

 * gnu/local.mk (GNU_SYSTEM_MODULES): Register it.

[...]

> +(define-public apcupsd
> +  (package
> +    (name "apcupsd")
> +    (version "3.14.14")
> +    (source (origin
> +              (method url-fetch)
> +              (uri
> +               (string-append
> +                "mirror://sourceforge/" name "/" name " - Stable/" version
> +                "/" name "-" version ".tar.gz"))
> +              (sha256
> +               (base32
> +                "0rwqiyzlg9p0szf3x6q1ppvrw6f6dbpn2rc5z623fk3bkdalhxyv"))))
> +    (native-inputs
> +     (list pkg-config python-docutils))
> +    (inputs
> +     (list libusb libusb-compat))

nitpick: If there are less than 5 and they fit on the same line, I
prefer to list them on the same line as the field name itself.  That's
how 'guix style' does it, although it has a tendency to protrude past
our max column width of 80 guideline in other places (that's bug#62303).

Also, we conventionally list the input fields below the arguments field
of a package, although technically it's unimportant.

> +    (outputs '("out" "doc"))
> +    (build-system gnu-build-system)
> +    (arguments
> +     (list
> +      #:configure-flags
> +      #~(list
> +         ;; The configure script ignores --prefix for most of the paths.

Well done with the comment.

> +         (string-append "--exec-prefix=" #$output)
> +         (string-append "--mandir=" #$output "/share/man")
> +         (string-append "--sbindir=" #$output "/sbin")
> +         (string-append "--sysconfdir=" #$output "/etc/apcupsd")
> +         (string-append "--with-halpolicydir=" #$output "/share/halpolicy")
> +
> +         ;; Put us into the version string.
> +         "--with-distname=GNU/Guix"

The name is 'GNU Guix'.

> +         "--disable-install-distdir"
> +
> +         ;; State directories.
> +         "--localstatedir=/var"
> +         "--with-log-dir=/var/log"
> +         "--with-pid-dir=/var/run"
> +         "--with-lock-dir=/var/run/apcupsd/lock"
> +         "--with-nologin=/var/run/apcupsd"
> +         "--with-pwrfail-dir=/var/run/apcupsd"

I think /var/run is deprecated in favor of just /run, so we should
configure the package to use this, I think.

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

> +         ;; Enable additional drivers.
> +         "--enable-test"

Is '--enable-test' useful?  What does it do?

> +         "--enable-usb"
> +         "--enable-modbus-usb")
> +      #:tests? #f                       ; There are no tests.
> +      #:modules (cons '(ice-9 ftw) %default-gnu-modules)
> +      #:phases
> +      #~(modify-phases %standard-phases
> +          ;; These are not installed, so trick Make into thinking they were
> +          ;; already generated.  Allows us not to depend on mandoc, util-linux.
> +          (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.
These are only needed as native inputs anyway, right?

> +          (add-after 'build 'build-manual
> +            (lambda _
> +              (invoke "make" "-C" "doc/manual" "manual.html")))
> +          (add-after 'install-license-files 'move-doc
> +            (lambda _
> +              (let ((target (string-append #$output:doc
> +                                           "/share/doc/"
> +                                           (strip-store-file-name #$output))))
> +                (mkdir-p target)
> +                (for-each (lambda (f)
> +                            (copy-file (string-append "doc/manual/" f)
> +                                       (string-append target "/" f)))
> +                          (scandir "doc/manual"
> +                                   (lambda (f)
> +                                     (or (string-suffix? ".png" f)
> +                                         (string-suffix? ".html" f))))))))
> +          ;; If sending mails is required, use proper mail program.
> +          (add-after 'install 'remove-smtp
> +            (lambda _
> +              (delete-file (string-append #$output "/sbin/smtp"))))
> +          ;; The configuration files and scripts are not really suitable for
> +          ;; Guix, and our service provides its own version anyway.  So nuke

I'd use the more peaceful 'remove' or 'delete' instead of 'nuke'.

> +          ;; these to make sure `apcupsd' and `apctest' executed without any
> +          ;; arguments fail.  `apctest' actually segfaults, but only after
> +          ;; printing an error ¯\_(ツ)_/¯.

Please don't embed emojis in the source :-).

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

> +    (home-page "https://www.apcupsd.org")

I'd use 'http', since their TLS cert is now invalid.

> +    (synopsis "A daemon for controlling APC UPSes")

s/A //  (as hinted by 'guix lint').

> +    (description "Apcupsd can be used for power mangement and controlling most

s/mangement/management/

> +of APC’s UPS models on Unix and Windows machines.  Apcupsd works with most of
> +APC’s Smart-UPS models as well as most simple signalling models such a Back-UPS,
> +and BackUPS-Office.")
> +    (license license:gpl2)))

I think it's actually license:gpl2+, according to
apcupsd-3.14.14/COPYING, which says:

--8<---------------cut here---------------start------------->8---
Each version is given a distinguishing version number.  If the Program
specifies a version number of this License which applies to it and "any
later version", you have the option of following the terms and conditions
either of that version or of any later version published by the Free
Software Foundation.  If the Program does not specify a version number of
this License, you may choose any version ever published by the Free Software
Foundation.
--8<---------------cut here---------------end--------------->8---

In this case for most file the last sentence applies, some other
explicitly mention 'or any later version'.  That's also supported by the
debian/copyright file.

Another thing found:  the doc output doesn't build reproducibly, as
shown by:

--8<---------------cut here---------------start------------->8---
$ ./pre-inst-env guix build --no-grafts --check -K
guix build: error: derivation `/gnu/store/r62j72bd3an8k2fbmaiil5hma32syxdy-apcupsd-3.14.14.drv' may not be deterministic: output `/gnu/store/bhwkrgkqahgs5h8crcfyacvnwp7rj390-apcupsd-3.14.14-doc' differs from `/gnu/store/bhwkrgkqahgs5h8crcfyacvnwp7rj390-apcupsd-3.14.14-doc-check'
--8<---------------cut here---------------end--------------->8---

Let's see why:

$ diff -r /gnu/store/bhwkrgkqahgs5h8crcfyacvnwp7rj390-apcupsd-3.14.14-doc{,-check}
diff -r /gnu/store/bhwkrgkqahgs5h8crcfyacvnwp7rj390-apcupsd-3.14.14-doc/share/doc/apcupsd-3.14.14/manual.html /gnu/store/bhwkrgkqahgs5h8crcfyacvnwp7rj390-apcupsd-3.14.14-doc-check/share/doc/apcupsd-3.14.14/manual.html
376c376
< <div class="line">February  5, 2025 06:54:22</div>
---
> <div class="line">February  5, 2025 06:54:50</div>

Ah, the classic date time stamp.  You'll want to neuter it in the source
or in the built html file (with a preference for the former).

Could you please send a v2?
  

Hi,

Tomas Volf <~@wolfsden.cz> writes:

> --- /dev/null
> +++ b/gnu/packages/power.scm
> @@ -0,0 +1,125 @@
> +;;; Copyright (C) 2025 Tomas Volf <~@wolfsden.cz>
> +;;; Copyright (C) 2023 Raven Hallsby <karl@hallsby.com>

One last thing; I don't see Raven listed in a 'Co-authored-by:' git
trailer in the commit message; should they?  Or otherwise mention
plainly this work was based on their previous work, made available
'$where'.
  

Hello Maxim,

thank you for the review, replies below.

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

> Hi Tomas,
>
> Tomas Volf <~@wolfsden.cz> writes:
>
>> * gnu/packages/power.scm (apcupsd): New variable.
>
> 'guix lint' says:
>
> gnu/packages/power.scm:120:14: apcupsd@3.14.14: no article allowed at the beginning of the synopsis
> gnu/packages/power.scm:119:15: apcupsd@3.14.14: TLS certificate error: X.509 server certificate for 'www.apcupsd.org' does not match: CN=sf.net
>
>
> Please fix these.

Both fixed.

>
>> Change-Id: I74f59cd1fa2a13954117ff1683a10a84576cc839
>> ---
>>  gnu/local.mk           |   1 +
>>  gnu/packages/power.scm | 125 +++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 126 insertions(+)
>>  create mode 100644 gnu/packages/power.scm
>>
>> diff --git a/gnu/local.mk b/gnu/local.mk
>> index 855f2acccc..6ca7bf68ac 100644
>> --- a/gnu/local.mk
>> +++ b/gnu/local.mk
>> @@ -557,6 +557,7 @@ GNU_SYSTEM_MODULES =				\
>>    %D%/packages/polkit.scm			\
>>    %D%/packages/popt.scm				\
>>    %D%/packages/potassco.scm				\
>> +  %D%/packages/power.scm			\
>
> This change should be mentioned in the change log as well, e.g.:
>
>  * gnu/local.mk (GNU_SYSTEM_MODULES): Register it.
>
> [...]

I used slightly different message, since the previous line mentions "new
variable", but the object being registered is whole file.

>
>> +(define-public apcupsd
>> +  (package
>> +    (name "apcupsd")
>> +    (version "3.14.14")
>> +    (source (origin
>> +              (method url-fetch)
>> +              (uri
>> +               (string-append
>> +                "mirror://sourceforge/" name "/" name " - Stable/" version
>> +                "/" name "-" version ".tar.gz"))
>> +              (sha256
>> +               (base32
>> +                "0rwqiyzlg9p0szf3x6q1ppvrw6f6dbpn2rc5z623fk3bkdalhxyv"))))
>> +    (native-inputs
>> +     (list pkg-config python-docutils))
>> +    (inputs
>> +     (list libusb libusb-compat))
>
> nitpick: If there are less than 5 and they fit on the same line, I
> prefer to list them on the same line as the field name itself.  That's
> how 'guix style' does it, although it has a tendency to protrude past
> our max column width of 80 guideline in other places (that's
> bug#62303).

Fair enough.  I do not have strong preference, so I have made it into a
single line.

I have to admit I do not run `guix style' (yes, yes, I know contributing
part of the manual mandates it), since the code produced is often subpar
compared to the hand-crafted variant.

>
> Also, we conventionally list the input fields below the arguments field
> of a package, although technically it's unimportant.

Moved.

>
>> +    (outputs '("out" "doc"))
>> +    (build-system gnu-build-system)
>> +    (arguments
>> +     (list
>> +      #:configure-flags
>> +      #~(list
>> +         ;; The configure script ignores --prefix for most of the paths.
>
> Well done with the comment.
>
>> +         (string-append "--exec-prefix=" #$output)
>> +         (string-append "--mandir=" #$output "/share/man")
>> +         (string-append "--sbindir=" #$output "/sbin")
>> +         (string-append "--sysconfdir=" #$output "/etc/apcupsd")
>> +         (string-append "--with-halpolicydir=" #$output "/share/halpolicy")
>> +
>> +         ;; Put us into the version string.
>> +         "--with-distname=GNU/Guix"
>
> The name is 'GNU Guix'.

I was afraid of the space. :)  But seems to work fine, changed.

>
>> +         "--disable-install-distdir"
>> +
>> +         ;; State directories.
>> +         "--localstatedir=/var"
>> +         "--with-log-dir=/var/log"
>> +         "--with-pid-dir=/var/run"
>> +         "--with-lock-dir=/var/run/apcupsd/lock"
>> +         "--with-nologin=/var/run/apcupsd"
>> +         "--with-pwrfail-dir=/var/run/apcupsd"
>
> 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.

>
>> +         ;; Enable additional drivers.
>> +         "--enable-test"
>
> Is '--enable-test' useful?  What does it do?

According to the manual

--8<---------------cut here---------------start------------->8---
This turns on a test driver that is used only for debugging.
--8<---------------cut here---------------end--------------->8---

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.

>
>> +         "--enable-usb"
>> +         "--enable-modbus-usb")
>> +      #:tests? #f                       ; There are no tests.
>> +      #:modules (cons '(ice-9 ftw) %default-gnu-modules)
>> +      #:phases
>> +      #~(modify-phases %standard-phases
>> +          ;; These are not installed, so trick Make into thinking they were
>> +          ;; already generated.  Allows us not to depend on mandoc, util-linux.
>> +          (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?

> These are only needed as native inputs anyway, right?

Yes.

>
>> +          (add-after 'build 'build-manual
>> +            (lambda _
>> +              (invoke "make" "-C" "doc/manual" "manual.html")))
>> +          (add-after 'install-license-files 'move-doc
>> +            (lambda _
>> +              (let ((target (string-append #$output:doc
>> +                                           "/share/doc/"
>> +                                           (strip-store-file-name #$output))))
>> +                (mkdir-p target)
>> +                (for-each (lambda (f)
>> +                            (copy-file (string-append "doc/manual/" f)
>> +                                       (string-append target "/" f)))
>> +                          (scandir "doc/manual"
>> +                                   (lambda (f)
>> +                                     (or (string-suffix? ".png" f)
>> +                                         (string-suffix? ".html" f))))))))
>> +          ;; If sending mails is required, use proper mail program.
>> +          (add-after 'install 'remove-smtp
>> +            (lambda _
>> +              (delete-file (string-append #$output "/sbin/smtp"))))
>> +          ;; The configuration files and scripts are not really suitable for
>> +          ;; Guix, and our service provides its own version anyway.  So nuke
>
> I'd use the more peaceful 'remove' or 'delete' instead of 'nuke'.

Uh, sure.  Done.

>
>> +          ;; these to make sure `apcupsd' and `apctest' executed without any
>> +          ;; arguments fail.  `apctest' actually segfaults, but only after
>> +          ;; printing an error ¯\_(ツ)_/¯.
>
> Please don't embed emojis in the source :-).

Technically kaomoji (顔文字), not emoji (絵文字), but point taken. :)

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

>
>> +    (home-page "https://www.apcupsd.org")
>
> I'd use 'http', since their TLS cert is now invalid.

Done as part of resolving the ling warning.

>
>> +    (synopsis "A daemon for controlling APC UPSes")
>
> s/A //  (as hinted by 'guix lint').

Same.

>
>> +    (description "Apcupsd can be used for power mangement and controlling most
>
> s/mangement/management/

Nice catch, fixed.

>
>> +of APC’s UPS models on Unix and Windows machines.  Apcupsd works with most of
>> +APC’s Smart-UPS models as well as most simple signalling models such a Back-UPS,
>> +and BackUPS-Office.")
>> +    (license license:gpl2)))
>
> I think it's actually license:gpl2+, according to
> apcupsd-3.14.14/COPYING, which says:
>
> Each version is given a distinguishing version number.  If the Program
> specifies a version number of this License which applies to it and "any
> later version", you have the option of following the terms and conditions
> either of that version or of any later version published by the Free
> Software Foundation.  If the Program does not specify a version number of
> this License, you may choose any version ever published by the Free Software
> Foundation.
>
>
> In this case for most file the last sentence applies, some other
> explicitly mention 'or any later version'.  That's also supported by the
> debian/copyright file.

I do not think this is correct.  When I check
apcupsd-3.14.14/src/apcupsd.c file, I see this in its header:

--8<---------------cut here---------------start------------->8---
 * Copyright (C) 1999-2005 Kern Sibbald
 *
 * This program is free software; you can redistribute it and/or
 * modify it under the terms of version 2 of the GNU General
 * Public License as published by the Free Software Foundation.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
 * General Public License for more details.
 *
 * You should have received a copy of the GNU General Public
 * License along with this program; if not, write to the Free
 * Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
 * MA 02110-1335, USA.
--8<---------------cut here---------------end--------------->8---

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

I did not find debian/copyright file in the source code.

>
> Another thing found:  the doc output doesn't build reproducibly, as
> shown by:
>
> $ ./pre-inst-env guix build --no-grafts --check -K
> guix build: error: derivation `/gnu/store/r62j72bd3an8k2fbmaiil5hma32syxdy-apcupsd-3.14.14.drv' may not be deterministic: output `/gnu/store/bhwkrgkqahgs5h8crcfyacvnwp7rj390-apcupsd-3.14.14-doc' differs from `/gnu/store/bhwkrgkqahgs5h8crcfyacvnwp7rj390-apcupsd-3.14.14-doc-check'
>
> Let's see why:
>
> $ diff -r /gnu/store/bhwkrgkqahgs5h8crcfyacvnwp7rj390-apcupsd-3.14.14-doc{,-check}

This is great, I did not know that it names the store item -check.  Very
useful, thank you.

> diff -r
> /gnu/store/bhwkrgkqahgs5h8crcfyacvnwp7rj390-apcupsd-3.14.14-doc/share/doc/apcupsd-3.14.14/manual.html
> /gnu/store/bhwkrgkqahgs5h8crcfyacvnwp7rj390-apcupsd-3.14.14-doc-check/share/doc/apcupsd-3.14.14/manual.html
> 376c376
> < <div class="line">February  5, 2025 06:54:22</div>
> ---
>> <div class="line">February  5, 2025 06:54:50</div>
>
> Ah, the classic date time stamp.  You'll want to neuter it in the source
> or in the built html file (with a preference for the former).

Nice find, I have missed this.  I have patched the source code of the
manual to not embed the date and time, seems to work now.

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

Once more thanks for the review,
Tomas
  

Hi,

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

> Hi,
>
> Tomas Volf <~@wolfsden.cz> writes:
>
>> --- /dev/null
>> +++ b/gnu/packages/power.scm
>> @@ -0,0 +1,125 @@
>> +;;; Copyright (C) 2025 Tomas Volf <~@wolfsden.cz>
>> +;;; Copyright (C) 2023 Raven Hallsby <karl@hallsby.com>
>
> One last thing; I don't see Raven listed in a 'Co-authored-by:' git
> trailer in the commit message; should they?  Or otherwise mention
> plainly this work was based on their previous work, made available
> '$where'.

I did not know about co-authored trailer.  Should it be used even in
situations where I just took a look at his version online without any
active involvement?  Currently I have modified the commit message to:

--8<---------------cut here---------------start------------->8---
gnu: Add apcupsd.

Some parts were taken or inspired by the work of Raven Hallsby available
here[0].  For that reason I have added his copyright as well (I asked for the
permission).

0: https://raw.githubusercontent.com/KarlJoad/guix/9013b5ac3fadb48fad2e7ef1fbfaa4848dcb922a/gnu/packages/power.scm

* gnu/packages/power.scm (apcupsd): New variable.
* gnu/local.mk (GNU_SYSTEM_MODULES): Register the new file.

Change-Id: I0e4b2f50c8adf0f96d140e2be0f79e3740f4955c
--8<---------------cut here---------------end--------------->8---

Is that sufficient?

Tomas
  

Hi Tomas,

Tomas Volf <~@wolfsden.cz> writes:

> Hi,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:
>
>> Hi,
>>
>> Tomas Volf <~@wolfsden.cz> writes:
>>
>>> --- /dev/null
>>> +++ b/gnu/packages/power.scm
>>> @@ -0,0 +1,125 @@
>>> +;;; Copyright (C) 2025 Tomas Volf <~@wolfsden.cz>
>>> +;;; Copyright (C) 2023 Raven Hallsby <karl@hallsby.com>
>>
>> One last thing; I don't see Raven listed in a 'Co-authored-by:' git
>> trailer in the commit message; should they?  Or otherwise mention
>> plainly this work was based on their previous work, made available
>> '$where'.
>
> I did not know about co-authored trailer.  Should it be used even in
> situations where I just took a look at his version online without any
> active involvement?  Currently I have modified the commit message to:

I believe if you reused enough of the code to make it necessary to add
their name to the copyright notice, then a Co-authored-by git trailer
would make sense (whether they were actively involved in your version or
not).

> gnu: Add apcupsd.
>
> Some parts were taken or inspired by the work of Raven Hallsby available
> here[0].  For that reason I have added his copyright as well (I asked for the
> permission).
>
> 0: https://raw.githubusercontent.com/KarlJoad/guix/9013b5ac3fadb48fad2e7ef1fbfaa4848dcb922a/gnu/packages/power.scm
>
> * gnu/packages/power.scm (apcupsd): New variable.
> * gnu/local.mk (GNU_SYSTEM_MODULES): Register the new file.
>
> Change-Id: I0e4b2f50c8adf0f96d140e2be0f79e3740f4955c
>
> Is that sufficient?

Co-authored-by is not mandatory, but I see it as a nice etiquette to
have, similar to how it's important to preserve the proper commit
authorship information, for due credits.
  

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?
  

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

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

I have now added a link to my message to the mailing list.

>>>
>>>> +         ;; 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 :-).
>
> [...]

Makes sense, I took it out.

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

Makes sense, I will remove the phase and add the dependencies into
native-inputs.

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

I did not get to the service part yet (life has sadly been somewhat
busy), will get to it during this weekend.  But I was also waiting for
your response here, there was no reason to send v2 since I was pretty
sure I will need to do additional changes, so v3 would required anyway.

Tomas
  

Hi Tomas,

Thank you for having taken my suggestions into consideration :-).

[...]

>> I haven't seen v2 yet, I guess you are still working out the service
>> part?
>
> I did not get to the service part yet (life has sadly been somewhat
> busy), will get to it during this weekend.  But I was also waiting for
> your response here, there was no reason to send v2 since I was pretty
> sure I will need to do additional changes, so v3 would required anyway.

No worries.  Just keep me in CC when you get around ot it, and it's
probably going to be fine to go then.