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

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

Re: Review of Debian package pystray

Show all headers | View raw

[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
   ⠈⠳⣄

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