Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.debian.maint.python > #14548 > unrolled thread

Review of Debian package pystray

Back to article view | Back to linux.debian.maint.python

Contents

  Review of Debian package pystray Louis-Philippe Véronneau <pollo@debian.org> - 2022-10-22 05:30 +0200
    Re: Review of Debian package pystray Louis-Philippe Véronneau <pollo@debian.org> - 2022-11-07 18:50 +0100
      Re: Review of Debian package pystray Claudius Heine <ch@denx.de> - 2022-11-08 15:50 +0100
        Re: Review of Debian package pystray Jeroen Ploemen <jcfp@debian.org> - 2022-12-03 09:40 +0100

#14548 — Review of Debian package pystray

[Multipart message — attachments visible in raw view] — view raw

Hello,

This is my review of the pystray package you asked the Debian Python 
Team to sponsor in the Debian archive.

1. I doubt d/README.source is needed, as this is a team-maintained 
package and most of that info is in the Policy :)

2. in d/rules, you are using "override_dh_sphinxdoc" and then calling 
dh_sphinxdoc.

You should instead use "execute_before_dh_sphinxdoc".

3. You are not running any upstream tests, neither at build not as 
autopkgtests.

Tests are very important. How do you know your package isn't broken, or 
has not been broken by a change in the archive? Only tests can tell you 
that.

I see you've noted that some tests require user confirmations and it 
indeed seems to be the case for most of the ones in icon_tests.py.

* is there a way to bypass this or are those tests simply not relevant 
in a automated environment?

* what about menu_descriptor_tests.py? I gave it a very cursory look, 
but it doesn't seem to use the confirm() function. Trying to run it 
gives me an  "Xlib.error.DisplayNameError: Bad display name" error 
though, so chances are you'd need to run tests with xvfb to mock an X 
session.

Note that it is my personal policy not to sponsor packages that do not 
run upstream tests. I make some exceptions for unusual cases (this might 
be one?), but rarely.

Some other people might not be as rigid as I am on this, but I thought 
you should know.

-------------------

That's it! I've removed your package from the sponsor queue for now, but 
feel free to re-add it when you feel like you've dealt with my review.

Apart from the test problem, this is a pretty good package!

Thanks for you contribution to Debian.

-- 
   ⢀⣴⠾⠻⢶⣦⠀
   ⣾⠁⢠⠒⠀⣿⡁  Louis-Philippe Véronneau
   ⢿⡄⠘⠷⠚⠋   pollo@debian.org / veronneau.org
   ⠈⠳⣄

[toc] | [next] | [standalone]

#14609

[Multipart message — attachments visible in raw view] — view raw

On 2022-10-21 23 h 28, Louis-Philippe Véronneau wrote:
> Hello,
> 
> This is my review of the pystray package you asked the Debian Python 
> Team to sponsor in the Debian archive.
> 
> 1. I doubt d/README.source is needed, as this is a team-maintained 
> package and most of that info is in the Policy :)
> 
> 2. in d/rules, you are using "override_dh_sphinxdoc" and then calling 
> dh_sphinxdoc.
> 
> You should instead use "execute_before_dh_sphinxdoc".
> 
> 3. You are not running any upstream tests, neither at build not as 
> autopkgtests.
> 
> Tests are very important. How do you know your package isn't broken, or 
> has not been broken by a change in the archive? Only tests can tell you 
> that.
> 
> I see you've noted that some tests require user confirmations and it 
> indeed seems to be the case for most of the ones in icon_tests.py.
> 
> * is there a way to bypass this or are those tests simply not relevant 
> in a automated environment?
> 
> * what about menu_descriptor_tests.py? I gave it a very cursory look, 
> but it doesn't seem to use the confirm() function. Trying to run it 
> gives me an  "Xlib.error.DisplayNameError: Bad display name" error 
> though, so chances are you'd need to run tests with xvfb to mock an X 
> session.
> 
> Note that it is my personal policy not to sponsor packages that do not 
> run upstream tests. I make some exceptions for unusual cases (this might 
> be one?), but rarely.
> 
> Some other people might not be as rigid as I am on this, but I thought 
> you should know.
> 
> -------------------
> 
> That's it! I've removed your package from the sponsor queue for now, but 
> feel free to re-add it when you feel like you've dealt with my review.
> 
> Apart from the test problem, this is a pretty good package!
> 
> Thanks for you contribution to Debian.
> 

Hello,

I've taken another look at pystray.

First of all, next time, please don't force push, it made it harder to 
know what had changed since my last review and I had to start from 
scratch :(

The autopkgtests are currently failing. I suspect you are not running 
those locally when you are building the package? It's fairly easy to do 
on sbuild [1] and I would highly recommend you add this to your standard 
build process.

In any case, I get the following error:

E   Xlib.error.DisplayNameError: Bad display name ""

I see you patched the upstream code to be able to run the tests at build 
(kudos). I suspect either dependencies are missing in the autopkgtest, 
or you aren't passing your TEST_SKIP_INTERACTIVE var there.

In either case, those tests need to pass, otherwise the package won't 
migrate to testing.

Note that:

1. You have duplicate build-dep entries in d/control
2. Your d/salsa-ci.yml file is currently skipping autopkgtests

I would've have fixed those before uploading, but with the failing 
autopkgtests it seems we'll need another back-and-forth round anyway.

Cheers,

[1]: Look for "run_autopkgtest = 1" in /etc/sbuild/sbuild.conf

-- 
   ⢀⣴⠾⠻⢶⣦⠀
   ⣾⠁⢠⠒⠀⣿⡁  Louis-Philippe Véronneau
   ⢿⡄⠘⠷⠚⠋   pollo@debian.org / veronneau.org
   ⠈⠳⣄

[toc] | [prev] | [next] | [standalone]

#14611

Hi,

On 2022-11-07 18:45, Louis-Philippe Véronneau wrote:
> On 2022-10-21 23 h 28, Louis-Philippe Véronneau wrote:
>> Hello,
>>
>> This is my review of the pystray package you asked the Debian Python 
>> Team to sponsor in the Debian archive.
>>
>> 1. I doubt d/README.source is needed, as this is a team-maintained 
>> package and most of that info is in the Policy :)
>>
>> 2. in d/rules, you are using "override_dh_sphinxdoc" and then calling 
>> dh_sphinxdoc.
>>
>> You should instead use "execute_before_dh_sphinxdoc".
>>
>> 3. You are not running any upstream tests, neither at build not as 
>> autopkgtests.
>>
>> Tests are very important. How do you know your package isn't broken, 
>> or has not been broken by a change in the archive? Only tests can tell 
>> you that.
>>
>> I see you've noted that some tests require user confirmations and it 
>> indeed seems to be the case for most of the ones in icon_tests.py.
>>
>> * is there a way to bypass this or are those tests simply not relevant 
>> in a automated environment?
>>
>> * what about menu_descriptor_tests.py? I gave it a very cursory look, 
>> but it doesn't seem to use the confirm() function. Trying to run it 
>> gives me an  "Xlib.error.DisplayNameError: Bad display name" error 
>> though, so chances are you'd need to run tests with xvfb to mock an X 
>> session.
>>
>> Note that it is my personal policy not to sponsor packages that do not 
>> run upstream tests. I make some exceptions for unusual cases (this 
>> might be one?), but rarely.
>>
>> Some other people might not be as rigid as I am on this, but I thought 
>> you should know.
>>
>> -------------------
>>
>> That's it! I've removed your package from the sponsor queue for now, 
>> but feel free to re-add it when you feel like you've dealt with my 
>> review.
>>
>> Apart from the test problem, this is a pretty good package!
>>
>> Thanks for you contribution to Debian.
>>
> 
> Hello,
> 
> I've taken another look at pystray.
> 
> First of all, next time, please don't force push, it made it harder to 
> know what had changed since my last review and I had to start from 
> scratch :(

Sorry about that.

> The autopkgtests are currently failing. I suspect you are not running 
> those locally when you are building the package? It's fairly easy to do 
> on sbuild [1] and I would highly recommend you add this to your standard 
> build process.
> 
> In any case, I get the following error:
> 
> E   Xlib.error.DisplayNameError: Bad display name ""
> 
> I see you patched the upstream code to be able to run the tests at build 
> (kudos). I suspect either dependencies are missing in the autopkgtest, 
> or you aren't passing your TEST_SKIP_INTERACTIVE var there.
> 
> In either case, those tests need to pass, otherwise the package won't 
> migrate to testing.
> 
> Note that:
> 
> 1. You have duplicate build-dep entries in d/control
> 2. Your d/salsa-ci.yml file is currently skipping autopkgtests
> 
> I would've have fixed those before uploading, but with the failing 
> autopkgtests it seems we'll need another back-and-forth round anyway.

Thanks, I fixed those and autopkgtest on salsa now seems to work.

I wasn't really sure how to set the environment variable for autodep8, 
so I just wrote the output of autodep8 into d/tests/control and added 
the environment variable as well as xvfb and xauth call and depends. I 
am not sure if that is the correct way, though. I guess that means that 
autodep8 is no longer used and instead autopkgtest is used directly...

> Cheers,
> 
> [1]: Look for "run_autopkgtest = 1" in /etc/sbuild/sbuild.conf

Well, I have issues building with sbuild [1] and could not yet figure 
out why. So I just build the packages directly on my machine and relied 
on the salsa-ci for the rest.

regards,
Claudius


[1] It fails with:
```
Setup apt archive
-----------------

Merged Build-Depends: debhelper-compat (= 13), dh-python, dunst, 
gir1.2-ayatanaappindicator3-0.1, gir1.2-glib-2.0, gir1.2-gtk-3.0, 
libglib2.0-tests, python3-all, python3-pil, python3-setuptools, 
python3-six, python3-sphinx, python3-xlib, xauth, xvfb, build-essential, 
fakeroot
Filtered Build-Depends: debhelper-compat (= 13), dh-python, dunst, 
gir1.2-ayatanaappindicator3-0.1, gir1.2-glib-2.0, gir1.2-gtk-3.0, 
libglib2.0-tests, python3-all, python3-pil, python3-setuptools, 
python3-six, python3-sphinx, python3-xlib, xauth, xvfb, build-essential, 
fakeroot
dpkg-deb: building package 'sbuild-build-depends-main-dummy' in 
'/<<RESOLVERDIR>>/apt_archive/sbuild-build-depends-main-dummy.deb'.
dpkg-deb: error: failed to make temporary file (control member): No such 
file or directory
Dummy package creation failed
E: Setting up apt archive failed
```

[toc] | [prev] | [next] | [standalone]

#14660

[Multipart message — attachments visible in raw view] — view raw

hi Claudius,

took a look at the pystray package up for sponsorship in the Python
team. Overall it's in really good shape, still a few comments left
though:

* control: the long description is really short and mentions neither
supported environment nor any other pystray features.

* tests:
 + please loop over py3versions -s rather than -r;
 + both tests have the same test-name;
 + the upstream testsuite autopkgtest doesn't actually run any tests
   according to ci logs [1];
 + for the upstream testsuite, you want to copy the test files to the
   $AUTOPKGTEST_TMP dir and run from there, to avoid testing the
   extracted sources rather than the installed package;
 + keeping the test commands/script in a separate file (rather than
   d/tests/control) tends to greatly increase readability for all but
   the smallest and most trivial autopkgtests.


[1]https://salsa.debian.org/python-team/packages/pystray/-/jobs/3596337#L411

[toc] | [prev] | [standalone]

Back to top | Article view | linux.debian.maint.python

csiph-web