Bug#823187: RFS: icdiff/1.7.3-1
Control: tags -1 + moreinfo
On Mon, May 2, 2016 at 6:14 AM, Sascha Steinbiss wrote:
> Alternatively, one can download the package with dget using this command:
>
> dget -x https://mentors.debian.net/debian/pool/main/i/icdiff/icdiff_1.7.3-1.dsc
Here is a review:
These need to be fixed before I upload this:
> This NEW upload closes RFP #818245.
Any particular reason you haven't retitled it to ITP and set yourself
as the owner?
tests/b/1 and tests/input-3.txt looks like duplicate of a file copied
from another project. At minimum the source location, copyright and
license info for it looks like it needs to be clarified.
Since this is a fork of part of the Python stdlib, I would expect
there would be some more copyright holders listed in the script itself
and in debian/copyright. This needs to be clarified upstream. Please
also let the security team know that it is a fork, they track forks
through the embedded code copies stuff.
https://wiki.debian.org/EmbeddedCodeCopies
These would be nice to fix:
There isn't any point in adding DEP-8 tests because test.sh only runs
icdiff from the current directory but DEP-8 is about testing the
installed package. You might be able to fix this by sending upstream a
patch for the script though.
Please add more DEP-3 headers to the patch, including at minimum the
Forwarded header.
http://dep.debian.net/deps/dep3/#Forwarded
Could you ask upstream to sign their commits, tags and release files?
And then add signature verification to debian/watch.
https://wiki.debian.org/Creating%20signed%20GitHub%20releases
https://wiki.debian.org/debian/watch#Cryptographic_signature_verification
https://help.riseup.net/en/security/message-security/openpgp/best-practices
I can't find the syntax right now, but there is a way for manual pages
to redirect from one to the other instead of duplicating them.
Any particular reason you used a different license to upstream for the
Debian directory? Generally it is advisable to use the same license so
everything is compatible license wise.
Upstream might want to work on merging their changes back into Python stdlib.
There is a potential tmpfile vulnerability in the test suite.
If upstream switched to argparser they could easily add bash
completion for the icdiff options using argcomplete. That can be
easily made optional too, see the code for check-all-the-things.
https://anonscm.debian.org/cgit/collab-maint/check-all-the-things.git/tree/check-all-the-things
Upstream might want to port this to Python 3 since version 2 will go
away eventually.
The upstream ChangeLog file looks more like a NEWS file to me,
ChangeLog is usually more like a git commit log generated with git2cl.
Please add some upstream metadata: https://wiki.debian.org/UpstreamMetadata
Upstream might want to look at our upstream guide:
https://wiki.debian.org/UpstreamGuide
Automatic checks:
lintian
P: icdiff source: debian-watch-may-check-gpg-signature
check-all-the-things
$ find -type f \( -iname '*.sh' -o -iname '*.bash' \) -exec bashate
--ignore E002,E003 {} +
E020: Function declaration not in format "^function name {$":
'function fail() {'
- ./test.sh : L35
E020: Function declaration not in format "^function name {$":
'function check_gold() {'
- ./test.sh : L40
2 bashate error(s) found
$ find \( -name .git -o -name .svn -o -name .bzr -o -name CVS -o -name
.hg -o -name _darcs -o -name _FOSSIL_ -o -name .sgdrawer \) -prune -o
-empty -print
./tests/a/1
./debian/man
$ fdupes -q -r . | grep -vE
'/(\.(git|svn|bzr|hg|sgdrawer)|_(darcs|FOSSIL_)|CVS)(/|$)' | cat -s
...
./tests/b/d/q
./debian/compat
./tests/b/c/e
./tests/a/c/e
./tests/gold-45-h.txt
./tests/gold-45-sas-h.txt
./tests/gold-45-sas-h-nb.txt
./tests/gold-45-h-nb.txt
./tests/input-3.txt
./tests/b/1
# check if these can be switched to https://
$ grep -rF http: .
./README.md:This file is derived from `difflib.HtmlDiff` which is
under the [license](http://www.python.org/download/releases/2.6.2/license/).
Binary file ./tests/input-3.txt matches
Binary file ./tests/b/1 matches
./setup.py: url="http://www.jefftk.com/icdiff";,
./icdiff: python. See: http://www.python.org/psf/license/
./debian/copyright:Format:
http://www.debian.org/doc/packaging-manuals/copyright-format/1.0/
./debian/copyright: along with this program. If not, see
<http://www.gnu.org/licenses/>
./debian/control:Homepage: http://www.jefftk.com/icdiff
$ find -type f -iname '*.py' -exec pyflakes {} +
./setup.py:1: 'convert_path' imported but unused
$ find -type f -iname '*.py' -exec pyflakes3 {} +
./setup.py:1: 'convert_path' imported but unused
$ find -type f -iname '*.py' -exec pylint
--msg-template='{path}:{line}:{column}: [{category}:{symbol}] {obj}:
{msg}' --reports=n {} +
No config file found, using default configuration
************* Module setup
setup.py:1:0: [convention:missing-docstring] : Missing module docstring
setup.py:4:0: [convention:invalid-name] : Invalid constant name "package_data"
setup.py:5:0: [warning:exec-used] : Use of exec
setup.py:1:0: [warning:unused-import] : Unused convert_path imported
from distutils.util
$ find -type f -iname '*.py' -exec pylint3
--msg-template='{path}:{line}:{column}: [{category}:{symbol}] {obj}:
{msg}' --reports=n {} +
No config file found, using default configuration
************* Module setup
setup.py:1:0: [convention:missing-docstring] : Missing module docstring
setup.py:4:0: [convention:invalid-name] : Invalid constant name "package_data"
setup.py:5:0: [warning:exec-used] : Use of exec
setup.py:1:0: [warning:unused-import] : Unused convert_path imported
from distutils.util
$ find -type f -iname '*.sh' -exec sh -n {} \;
./test.sh: 35: ./test.sh: Syntax error: "(" unexpected
$ find -type f \( -iname '*.sh' -o -iname '*.bash' -o -iname '*.zsh'
\) -exec shellcheck {} +
<lots>
$ find -type d \( -iname .bzr -o -iname .git -o -iname .hg -o -iname
.svn -o -iname CVS -o -iname RCS -o -iname SCCS -o -iname _MTN -o
-iname _darcs -o -iname .pc -o -iname .cabal-sandbox -o -iname .cdv -o
-iname .metadata -o -iname CMakeFiles -o -iname _build -o -iname
_sgbak -o -iname autom4te.cache -o -iname blib -o -iname cover_db -o
-iname node_modules -o -iname '~.dep' -o -iname '~.dot' -o -iname
'~.nib' -o -iname '~.plst' \) -prune -o -type f ! \( -iname '*.bak' -o
-iname '*.swp' -o -iname '#.*' -o -iname '#*#' -o -iname 'core.*' -o
-iname '*~' -o -iname '*.gif' -o -iname '*.jpg' -o -iname '*.jpeg' -o
-iname '*.png' -o -iname '*.min.js' -o -iname '*.js.map' -o -iname
'*.js.min' -o -iname '*.min.css' -o -iname '*.css.map' -o -iname
'*.css.min' \) -exec spellintian --picky {} +
./README.md: subversion -> Subversion
./ChangeLog: python -> Python
./icdiff: python -> Python
./icdiff: chinese -> Chinese
$ grep -r '/tmp/' .
./test.sh: local tmp=/tmp/icdiff.output
--
bye,
pabs
https://wiki.debian.org/PaulWise
Reply to: