Hi,

Maxim Cournoyer <maxim.cournoyer@gmail.com> [2023-01-16T18:01:59+0100]:
> Sergiu Ivanov <sivanov@colimite.fr> writes:
>
>>>From b92cdb4ce99bc7ad45e0caba7f863db5931741db Mon Sep 17 00:00:00 2001
>>  
>> +(define-public python-pulsectl
>> +  (package
>> +    (name "python-pulsectl")
>> +    (version "22.3.2")
>> +    (source (origin
>> +              (method url-fetch)
>> +              (uri (pypi-uri "pulsectl" version))
>> +              (sha256
>> +               (base32
>> +                "115ha1cwpd2r84ssnxdbr59hgs0jbx0lz3xpqli64kmxxqf4w5yc"))))
>> +    (build-system python-build-system)
>> +    (inputs (list pulseaudio))
>> +    (arguments
>> +     `(#:tests? #f
>
> Tests are typically stripped from the pypi source archive (sdist).  If
> you look into the source repository, there are tests under
> pulsectl/tests, so it'd be better to fetch the source from git.

In fact, pulsectl's tests fail because they seem to want to start
a dummy PulseAudio instance, which I expect to fail because of the
restrictions of the build environment.  Here's my post on the mailing
list with some more details:

https://lists.gnu.org/archive/html/help-guix/2023-01/msg00038.html

I added a comment briefly explaining this, but maybe there is
a better way.

> Also note that for the cases where using #:tests? #f is actually needed
> (when there really are no test suite), a short explanatory comment is
> expected (;no test suite).
>
>> +       #:phases
>> +       (modify-phases %standard-phases
>> +         (add-after 'unpack 'patch-path
>> +           (lambda* (#:key inputs #:allow-other-keys)
>> +             (let ((pulse (assoc-ref inputs "pulseaudio")))
>> +               (substitute* "pulsectl/_pulsectl.py"
>> +                 (("libpulse.so.0")
>> +                  (string-append pulse "/lib/libpulse.so.0")))
>> +               #t))))))
>
> Please do not include trailing #t in phases or snippets anymore; they
> are not needed.

Fixed, thank you.

> Also prefer using a plain list for arguments and g-expressions
> (gexps).

I spent some time squinting at this remark and reading the manuals, but
I can't figure out what you mean.

Could you please give some more hints about the parts I should change
and how?

>> +    (home-page "https://github.com/mk-fg/python-pulse-control")
>> +    (synopsis
>> +     "Python bindings for mixer-like controls in PulseAudio")
>> +    (description
>> +     "Python high-level interface and ctypes-based bindings for
>> +PulseAudio (libpulse), to use in simple synchronous code.  This wrapper is
>> +mostly for mixer-like controls and introspection-related operations, as
>> +opposed to e.g. submitting sound samples to play and player-like
>> client.")
>
> I'd start the description with "This package provides a Python
> high-level interface [...]", to make it a complete sentence.
>
> I'd use plural for the last word (player-like clientS), as there could
> be more than one client available.

Done, thank you.

> Don't forget to CC my email when sending a revised v2 version with the
> above :-).

Done as well :D


Maxim Cournoyer <maxim.cournoyer@gmail.com> [2023-01-16T18:06:19+0100]:
> Hi again,
>
> Sergiu Ivanov <sivanov@colimite.fr> writes:
[...]
>> +    (arguments
>> +     `(#:tests? #f
>> +       #:phases
>> +       (modify-phases %standard-phases
>> +         (add-after 'unpack 'patch-path
>> +           (lambda* (#:key inputs #:allow-other-keys)
>> +             (let ((pulse (assoc-ref inputs "pulseaudio")))
>> +               (substitute* "pulsectl/_pulsectl.py"
>> +                 (("libpulse.so.0")
>> +                  (string-append pulse "/lib/libpulse.so.0")))
>
> Sorry, I forgot to mention in my previous reply: here, you could use
> (search-input-file inputs "lib/libpulse.so.0"), which has the added
> benefit of failing if the file cannot be found in the inputs arguments.

Oh, good to know, thank you for the suggestion!  search-input-file
actually simplified the code and allowed me to drop the let (which
I copied from the previous version of volctl in fact).

I updated patches 2 and 3 to use search-input-file and attach both to
these E-mails.

By the way, I'd be happy to know whether with debbugs it is better to
attach the updated patches to E-mails with comments, or rather sending
the patches as separate E-mails.

-
Sergiu