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

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

review for gtg/0.6-1

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

Contents

  review for gtg/0.6-1 Jeroen Ploemen <jcfp@debian.org> - 2022-11-01 14:00 +0100
    Re: review for gtg/0.6-1 François Mazen <francois@mzf.fr> - 2022-11-04 17:10 +0100
      Re: review for gtg/0.6-1 Jeroen Ploemen <jcfp@debian.org> - 2022-11-07 14:40 +0100

#14576 — review for gtg/0.6-1

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

hi Francois,

I took a look at the gtg package put up for sponsorship in the Python
team:

* changelog:
 + multiple entries for revisions that did enter the archive (0.3.1-2
   through 4) appear to have gone missing?
 + there's about a dozen open bugs against the old package, yet only
   a single one gets closed. Did you review the outstanding bugs and
   check if any of them are fixed by the new upstream release and/or
   the revamped packaging?
 + it's team policy to keep the target distribution at UNRELEASED
   while a package is under review.
 + ITP bug not closed upon package reintroduction?
* clean: consider converting the entries for deleting pycache stuff
  at depths other than 3 to also use globbing.                  
* control:
 + long description should be extended. Assume the reader knows
   little or nothing about the application at all; what can it do,
   what makes it special, what services does it integrate with, and
   so on. Take a look at the upstream homepage if you need
   inspiration.
 + why list the old maintainer as uploader?
 + multiple missing dependencies for utilities called by
   script_pocketmod.
 + missing dep gir1.2-secret-1 (for the optional import of
   gi.repository.Secret in GTG/core/keyring.py)
 + missing dep for optional import of gi.repository.GnomeKeyring in
   GTG/core/keyring.py (though it seems that's not yet packaged in
   Debian so we might have to forego it for now).
 + missing dep gir1.2-pango-1.0 (for the unconditional import of
   Pango in GTG/gtk/browser/treeview_factory.py and other files; as
   well as PangoCairo in GTG/gtk/browser/cell_renderer_tags.py)
 + unused build-dep on itstool?
 + lots of build-deps only appear useful for testing; please mark
   those <!nocheck>.
* copyright:
 + public domain without explanation detailing exactly what exemption
   the files in question have from default copyright restrictions.
 + GTG/plugins/dev_console/* headers say LGPL, not GPL.
 + one Jean-François Fortin Tam is listed in the 'Files: *'
   paragraph, but only appears as a copyright holder in two files
   (GTG/core/info.py.in and a single translation).
* docs: what purpose does a list of upstream authors serve as end
  user documentation?
* patches: two out of three patches at first glance appear useful for
  inclusion upstream, yet all are marked 'Forwarded: not-needed'?
* rules:
 + override_dh_auto_install starts by calling dh_auto_install;
   consider using execute_after_ instead of an override in such cases.
 + upstream testsuite (based on pytest) not run on build, why?
* lintian:
 + X: gtg: executable-in-usr-lib
   usr/lib/python3/dist-packages/GTG/plugins/export/export_templates/script_pocketmod
   (wrong install location per FHS?)
 + X: gtg: executable-in-usr-lib
   usr/lib/python3/dist-packages/GTG/core/networkmanager.py (imported
   as a python module, file probably shouldn't be executable at all?)
* autopkgtests:
 + please change directory to $AUTOPKGTEST_TMP before running test
   commands to ensure the test doesn't depend on anything from the
   extracted source pkg, see best practices at [1].
 + consider adding an autopkgtest based on the upstream testsuite.
* source: variables not properly quoted in 'script_pocketmod', cannot
  handle spaces (etc.) in the path of the source file; please patch.


Once the above comments have been addressed, simply re-add the
package to the IRC channel topic.

Note: I didn't do any functional testing yet, in light of the need
for significant changes to the current packaging.


[1]https://wiki.debian.org/ContinuousIntegration/AutopkgtestBestPractices

[toc] | [next] | [standalone]

#14603

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

Hello Jeroen,

thanks for this important package review!

I've fixed most of your remarks. Here are the ones that I don't know
what to do:

> * copyright:
>  + public domain without explanation detailing exactly what exemption
>    the files in question have from default copyright restrictions.

I don't know what do you expect here. The current description contains
"No copyright is claimed". What should I add? 

> * autopkgtests:
>  + consider adding an autopkgtest based on the upstream testsuite.

The upstream provides two test datasets but no associated tests for the
installed application. Should I write them myself? (would be quite
complex as it's a graphical interface)


This was intense modifications of the package, but worth it! Thanks for
your time. The package is ready for a new round of review.

Best Regards,
François

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

#14608

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

On Fri, 4 Nov 2022 16:07:56 +0100
François Mazen <francois@mzf.fr> wrote:

Thank you for making all those improvements.

> > * copyright:
> >  + public domain without explanation detailing exactly what
> > exemption the files in question have from default copyright
> > restrictions.  
> 
> I don't know what do you expect here. The current description
> contains "No copyright is claimed". What should I add? 

Copyright restrictions apply by default, so for something to be in
the public domain requires some explicit statement to that effect by
the copyright holder. While d/copyright says "No copyright is
claimed" (which may or may not serve that purpose, depending on
context) I cannot find that particular statement anywhere in the
source. Where does it come from, who wrote it? (Or am I just
overlooking something obvious?)

> > * autopkgtests:
> >  + consider adding an autopkgtest based on the upstream
> > testsuite.  
> 
> The upstream provides two test datasets but no associated tests for
> the installed application. Should I write them myself? (would be
> quite complex as it's a graphical interface)

Tests don't necessarily have to be for the application as a whole,
not to mention the existing 'smoke' and 'check-graphical-app' cover
that part already. With the Debian CI triggering an autopkgtest run
for gtg every time there's a change in a dependency, running an
upstream testsuite that tests bits and pieces of code can still be
useful for finding incompatibilities such changes may introduce. Also,
testsuites based on pytest tend to be straightforward to turn into an
autopkgtest, see for example [1] and [2].


I noticed on salsa that the updated d/rules tries to run the upstream
testsuite during build but it errors out during test collection (and
that error in turn doesn't trigger build failure).


[1]https://salsa.debian.org/python-team/packages/puremagic/-/tree/master/debian/tests
[2]https://salsa.debian.org/python-team/packages/nfoview/-/blob/debian/master/debian/tests/upstream-pytest

[toc] | [prev] | [standalone]

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

csiph-web