Hi pabs,
Thanks a lot for your review, and sorry for not responding earlier, too
much stuff on my table:
On 18-03-18 11:33:59, Paul Wise wrote:
> The copyright years are missing 2012, I'm not sure if the ftp-masters
> would reject the package on that basis.
I've referred to the upstream git history, instead of the licence file.
Fixed.
> I require these things to be fixed before I would sponsor the package:
>
> doc/mwic.1 and tests/coverage are generated files, they should be
> removed from the upstream VCS and tarballs and be always built from
> source. If upstream refuses, you should remove them in `debian/rules
> clean` and very early in `debian/rules build` so that the package will
> never depend on the existing upstream files.
I've searched quite a bit on the Internets how to do this, and leverage,
for now, dh_clean. I hope that this is an acceptable method for this, I
didn't found any "official" source. I'll approach upstream regarding
these files.
Also, the man page is now generated during dh_auto_install. I've added a
build dependency on python3-docutils for this.
> Some things that you may want to improve at some point:
>
> Please include the upstream signature in the source package. uscan
> does the right thing these days, or you can rename it to
> mwic_0.7.4.orig.tar.gz.asc and place it alongside the orig.tar.gz file
> and rebuild the source package.
Thanks for this pointer -- highly appreciated!
Actually, I've debugged this for quite some time during the last year,
and exchanged mails regarding this with Holger Levsen and Guido Günther
seeking a way to include the upstream signature. Back then, I've read
[1], and the man pages as well, but still couldn't figure it out -
dropping the .asc into the top level directory didn't made a difference.
After quite some time, I finally gave up. Up until yesterday I thought
the tooling is currently limited.
After reading your mail I debugged this again: It's not about the top
level directory, at least if using gbp buildpackage, but about
'export-dir'. Placing a symlink inside there which points to the actual
signature makes this work! :)
The new upload should look better now in this regard.
For now, I've wrote a wrapper script just for personal use, but I'll
work on [2] and [3] to get this implemented in a proper way.
> I like to wrap each of the fields in debian/watch on new lines.
Although I think this is a matter of personal taste: fixed.
> I like to use these wrap-and-sort parameters:
>
> wrap-and-sort --short-indent --wrap-always --sort-binary-packages
> --trailing-comma
See above. (Personally, I really dislike the trailing comma.)
> misc/mwic4po seems like it would be worth installing in the package?
There is no man page currently, and also, more importantly, out of the
box this doesn't run in a non-interactive way. I guess, at least some
people, might run mwic via a CI pipeline, that's why I don't ship this.
I'll approach upstream regarding this.
> Please add some upstream metadata:
>
> https://wiki.debian.org/UpstreamMetadata
I've did this as well, although I'm unsure why this isn't defined in the
policy. I'm aware that this was recently added to lintian, and reading
the bug, again, makes me wonder what's the process of getting a new
check into lintian. As it stands currently, some people take care of
this, and some don't. This is of course not set in stone, but...well,
not sure.
> Output from automated checks is available at the very end of this
> email.
Thanks!
> > Could you move the repo as well, or create a new one within the
> > Python team?
>
> I'll leave that to someone who knows how to do that, I haven't dealt
> with salsa yet.
Alright!
> > Yeah, I've used this as well, but in this case it's not that helpful
> > as it finds the code defining the error [2].
>
> I'd guess this is the source of the line you quoted:
>
> https://sources.debian.org/src/autopkgtest/5.1/runner/autopkgtest/?hl=144#L138
Correct. Still, my question stands: How do I enable autopkgtests for
this package?
> X: mwic source: upstream-metadata-file-is-missing
Fixed.
> check-all-the-things
>
> $ env PERL5OPT=-m-lib=. cme check dpkg
> ...
> Warning in 'control source Build-Depends:0' value 'debhelper (>=
> 11~)': should be (>= 11) not (>= 11~) because compat is 11
> Warning in 'control source Build-Depends:1' value 'python3 (>= 3.2)':
> unnecessary greater-than versioned dependency: python3 (>= 3.2).
> Debian has oldoldstable -> 3.2.3-6; oldstable -> 3.4.2-2;
> oldstable-kfreebsd -> 3.4.2-2; stable -> 3.5.3-1; unstable -> 3.6.3-2;
> testing -> 3.6.4-1; unstable -> 3.6.4-1;
> Warning in 'control binary:mwic Depends:0' value 'python3 (>= 3.2)':
> unnecessary greater-than versioned dependency: python3 (>= 3.2).
> Debian has oldoldstable -> 3.2.3-6; oldstable -> 3.4.2-2;
> oldstable-kfreebsd -> 3.4.2-2; stable -> 3.5.3-1; unstable -> 3.6.3-2;
> testing -> 3.6.4-1; unstable -> 3.6.4-1;
Fixed.
> $ doc8
> Scanning...
> Validating...
> .../mwic-0.7.4/doc/manpage.rst:19: D001 Line too long
> .../mwic-0.7.4/doc/manpage.rst:20: D001 Line too long
> .../mwic-0.7.4/doc/manpage.rst:53: D001 Line too long
I'll take these upstream.
> # You may want to file a bug on license-reconcile
> # about these false positives re Expat vs MIT/X11
> $ env PERL5OPT=-m-lib=. license-reconcile
> ...
> License mismatch: File private/check-rst has license MIT/X11 (BSD
> like) which does not match Expat. at
> /usr/share/perl5/Debian/LicenseReconcile/App.pm line 222, <GEN0> line
> 3.
> <lots more>
The texts are the same, there are only differences in special chars like
", so I guess this is a false positive.
> # This command checks style. While a consistent style
> # is a good idea, people who have different style
> # preferences will want to ignore some of the output.
> # Do not bother adding non-upstreamable patches for this.
> $ find . -type f -iname '*.py' -exec pycodestyle --ignore W191 {} +
> <lots>
>
> # This command checks style. While a consistent style
> # is a good idea, people who have different style
> # preferences will want to ignore some of the output.
> # Do not bother adding non-upstreamable patches for this.
> $ pydocstyle .
> <lots>
>
> $ find . -type f -iname '*.py' -exec pylint3 --rcfile=/dev/null
> --msg-template='{path}:{line}:{column}: [{category}:{symbol}] {obj}:
> {msg}' --reports=n {} +
> <lots>
>
> $ python3-bandit -r .
> <lots>
>
> $ grep -nHriE 'fixme|todo|hack|xxx+|broken' .
> ./tests/multiword-t-he.txt:6:# FIXME:
> ./tests/test_extdict.py:121: t({'aasumes'}, {'assumes'}) # FIXME? 'assumes'
> ./tests/test_extdict.py:122: t({'Addtional'}, {'Additional'}) #
> FIXME? 'addtional'
> ./tests/multiword-t-he.exp:17:FIXME:
> ./tests/multiword-t-he.exp:18:| # FIXME:
> ./private/run-pylint:32:log=$(mktemp -t pylint.XXXXXX)
> ./.pylintrc:5: fixme,
>
> $ vulture .
> lib/cli.py:54: Unused variable 'namespace'
> lib/cli.py:54: Unused variable 'option_string'
> lib/cli.py:187: Unused variable 'ch'
> lib/cli.py:192: Unused variable 'ch'
> lib/cli.py:310: Unused variable 'args'
> lib/colors.py:32: Unused variable 'warn'
> lib/colors.py:35: Unused variable 'unreverse'
> tests/test_blackbox.py:35: Unused attribute 'maxDiff'
> tests/test_blackbox.py:85: Unused attribute 'redundant'
> tests/test_blackbox.py:90: Unused variable 'enabled'
> tests/test_blackbox.py:101: Unused function 'loadTestsFromFile'
> tests/test_blackbox.py:105: Unused function 'wantFunction'
> tests/test_blackbox.py:115: Unused function '_test'
I'll take these to upstream, too.
Changes pushed to git and uploaded to m.d.n [4].
Thanks again,
cheers,
Georg
[1] https://lists.debian.org/debian-devel/2017/08/msg00072.html
[2] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=874550
[3] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=872864
[4] https://mentors.debian.net/debian/pool/main/m/mwic/mwic_0.7.4-1.dsc
Attachment:
signature.asc
Description: Digital signature