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

Groups > linux.debian.maint.python > #14611

Re: Review of Debian package pystray

Show key headers only | View raw

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

Back to linux.debian.maint.python | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

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

csiph-web