Path: csiph.com!news.mixmin.net!aioe.org!bofh.it!news.nic.it!robomod From: Claudius Heine Newsgroups: linux.debian.maint.python Subject: Re: Review of Debian package pystray Date: Tue, 08 Nov 2022 15:50:02 +0100 Message-ID: References: X-Original-To: =?UTF-8?Q?Louis-Philippe_V=c3=a9ronneau?= X-Mailbox-Line: From debian-python-request@lists.debian.org Tue Nov 8 14:42:48 2022 Old-Return-Path: X-Amavis-Spam-Status: No, score=-8.801 tagged_above=-10000 required=5.3 tests=[BAYES_00=-2, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, LDO_WHITELIST=-5, MURPHY_DRUGS_REL8=0.02, NICE_REPLY_A=-0.001, RCVD_IN_DNSWL_MED=-2.3, SARE_ADLTOBFU=0.68] autolearn=ham autolearn_force=no X-Policyd-Weight: NOT_IN_SBL_XBL_SPAMHAUS=-1.5 CL_IP_EQ_FROM_MX=-3.1; rate: -4.6 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.3 Content-Language: en-US Organization: Denx Software Engineering Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Virus-Status: Clean X-Mailing-List: archive/latest/20464 List-ID: List-URL: List-Archive: https://lists.debian.org/msgid-search/d88d2a36-d461-41e5-2dfd-30052a9eb80d@denx.de Approved: robomod@news.nic.it Lines: 130 Sender: robomod@news.nic.it X-Original-Cc: debian-python X-Original-Date: Tue, 8 Nov 2022 15:41:59 +0100 X-Original-Message-ID: X-Original-References: <7657e6cf-8610-9f4b-a7be-5d718cc1d9ab@debian.org> <391a4500-f46c-fbc9-9418-e3c0069674de@debian.org> Xref: csiph.com linux.debian.maint.python: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 '/<>/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 ```