Bug#763540: Review of psocksxx/0.0.5-1
On Wed, Oct 1, 2014 at 10:18 AM, Harlan Lieberman-Berg wrote:
> I took a look at psocksxx, and it looks pretty good!
Agreed, good review Harlan!
> A couple minor things that need to be changed:
Agreed with both of these.
Some more minor issues that you might want to fix at some point:
The build process doesn't include -Wall in the build flags for gcc.
libpsocksxx0 is missing a Pre-Depends on multiarch-support as it
doesn't use ${misc:Pre-Depends}.
You might want to build-dep on libcppunit-dev so that the build-time
test suite is enabled.
The git repository mentioned in Vcs-* does not exist.
Is there any particular reason you chose GPL-3+ for the Debian
packaging instead of using the same as the upstream code (LGPL-3+)? If
you ever develop patches that would mean the resulting binary packages
are GPL-3+ not LGPL-3+ as upstream had intended and there is a very
very slight chance this could cause license incompatibility issues as
a result.
compression = xz is the default for format 3.0 source packages, no
need for it in debian/source/options. compression-level = 9 has no
advantage (exact same size for the debian.tar.xz) over the default
compression level for this tiny package so I wouldn't bother with it,
removing this file might even reduce the size of the debian.tar.xz.
override_dh_installdocs should be removed, the make part should be in
override_dh_auto_build and the cp part in libpsocksxx-doc.docs.
You can pass arguments to dpkg-gensymbols like this: dh_makeshlibs --
-plibpsocksxx0
You should move the contents of README.Debian into a comment in
libpsocksxx-doc.lintian-overrides since it isn't useful to users of
the binary package looking for documentation.
I would suggest not installing the static library (remove *.a from
debian/*.install) unless someone files a bug asking for it.
README.md is not needed in the libpsocksxx0 library package since that
will only be installed as a dep of other things.
The suggests from libpsocksxx0 to libpsocksxx-doc is not needed since
that will only be installed as a dep of other things, please move that
to libpsocksxx-dev since developers will install the -dev package and
might also want the -doc package.
The upstream test suite is hardcoding paths to test sockets in to
/tmp, it should create those sockets in the test/build directory
instead.
Automated checks:
https://wiki.debian.org/HowToPackageForDebian#Check_points_for_any_package
https://anonscm.debian.org/cgit/collab-maint/check-all-the-things.git
$ lintian
P: psocksxx source: debian-watch-may-check-gpg-signature
X: libpsocksxx-doc: duplicate-files
usr/share/doc/libpsocksxx-doc/docs/search/all_f.js
usr/share/doc/libpsocksxx-doc/docs/search/functions_d.js
X: libpsocksxx-doc: duplicate-files
usr/share/doc/libpsocksxx-doc/docs/search/all_5.js
usr/share/doc/libpsocksxx-doc/docs/search/functions_3.js
X: libpsocksxx-doc: duplicate-files
usr/share/doc/libpsocksxx-doc/docs/search/all_2.js
usr/share/doc/libpsocksxx-doc/docs/search/functions_1.js
X: libpsocksxx-doc: duplicate-files
usr/share/doc/libpsocksxx-doc/docs/search/all_3.js
usr/share/doc/libpsocksxx-doc/docs/search/functions_2.js
X: libpsocksxx-doc: duplicate-files
usr/share/doc/libpsocksxx-doc/docs/ftv2doc.png
usr/share/doc/libpsocksxx-doc/docs/ftv2link.png
X: libpsocksxx-doc: duplicate-files
usr/share/doc/libpsocksxx-doc/docs/search/all_b.js
usr/share/doc/libpsocksxx-doc/docs/search/functions_9.js
X: libpsocksxx-doc: duplicate-files
usr/share/doc/libpsocksxx-doc/docs/ftv2blank.png
usr/share/doc/libpsocksxx-doc/docs/ftv2lastnode.png
usr/share/doc/libpsocksxx-doc/docs/ftv2node.png
usr/share/doc/libpsocksxx-doc/docs/ftv2vertline.png
X: libpsocksxx-doc: duplicate-files
usr/share/doc/libpsocksxx-doc/docs/ftv2plastnode.png
usr/share/doc/libpsocksxx-doc/docs/ftv2pnode.png
X: libpsocksxx-doc: duplicate-files
usr/share/doc/libpsocksxx-doc/docs/ftv2mlastnode.png
usr/share/doc/libpsocksxx-doc/docs/ftv2mnode.png
X: libpsocksxx-doc: duplicate-files
usr/share/doc/libpsocksxx-doc/docs/search/all_0.js
usr/share/doc/libpsocksxx-doc/docs/search/variables_0.js
X: libpsocksxx-doc: duplicate-files
usr/share/doc/libpsocksxx-doc/docs/search/all_10.js
usr/share/doc/libpsocksxx-doc/docs/search/functions_e.js
$ cme check dpkg
Warning in 'control source Build-Depends:3' value 'libcunit1-dev (>=
1.12.1)': unnecessary versioned dependency: libcunit1-dev >= 1.12.1.
Debian has squeeze -> 2.1-0.dfsg-9; wheezy -> 2.1-0.dfsg-10; jessie ->
2.1-2.dfsg-1; sid -> 2.1-2.dfsg-2;
Warning in 'control binary:"libpsocksxx-dev" Synopsis' value 'psocksxx
is a C++ wrapper for POSIX sockets (development files)': Synopsis is
too long.
$ duck
keys on reference is experimental at /usr/bin/duck line 183.
debian/control: Vcs-Git:
git://anonscm.debian.org/collab-maint/psocksxx.git: ERROR
fatal: '/git/collab-maint/psocksxx.git' does not appear to be a git repository
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
debian/control: Vcs-Browser:
http://anonscm.debian.org/cgit/collab-maint/psocksxx.git: ERROR
Curl:0 HTTP:404 No error
$ grep -r '/tmp/' .
./test/sockstreambuf_test.h:#define LOCAL_LISTENER_SOCK_PATH
"/tmp/psocksxx.listener.sock"
./test/sockstreambuf_test.h:#define LOCAL_SOCK_PATH
"/tmp/psocksxx.sock"
./test/lsockstream_test.h:#define LOCAL_LISTENER_SOCK_PATH
"/tmp/psocksxx.listener.sock"
./test/lsockstream_test.h:#define LOCAL_SOCK_PATH
"/tmp/psocksxx.sock"
$ grep -i warn ../*.build
configure: WARNING: Cppunit not found - continuing without unit test support
Warning: Tag `SYMBOL_CACHE_SIZE' at line 47 of file `doxygen.cfg' has
become obsolete.
To avoid this warning please remove this line from your
configuration file or upgrade it using "doxygen -u"
Warning: Tag `XML_SCHEMA' at line 249 of file `doxygen.cfg' has become obsolete.
To avoid this warning please remove this line from your
configuration file or upgrade it using "doxygen -u"
Warning: Tag `XML_DTD' at line 250 of file `doxygen.cfg' has become obsolete.
dpkg-gencontrol: warning: package libpsocksxx0: unused substitution
variable ${misc:Pre-Depends}
$ cppcheck --enable=all -j8 --quiet -f .
cppcheck: unusedFunction check can't be used with '-j' option.
Disabling unusedFunction check.
[test/lecho.cpp:100]: (style) The scope of the variable 'peer_sockfd'
can be reduced.
[test/nsockstream_test.cpp:99]: (style) Variable 't' is assigned a
value that is never used.
[test/lsockstream_test.cpp:120]: (style) Variable 'c' is assigned a
value that is never used.
[test/necho.cpp:152]: (style) The scope of the variable 'peer_sockfd'
can be reduced.
[lib/psocksxx/sockstreambuf.cpp:318]: (style) The scope of the
variable 'b_ready' can be reduced.
[test/tap/tap_listener.cpp:38]: (style) The scope of the variable 'c'
can be reduced.
[test/sockstreambuf_test.cpp:587]: (style) Variable 't' is assigned a
value that is never used.
[lib/psocksxx/sockstreambuf.cpp:1]: (information) Skipping
configuration 'SO_NOSIGPIPE' since the value of 'SO_NOSIGPIPE' is
unknown. Use -D if you want to check it. You can use -U to skip it
explicitly.
$ find \( -iname \*.c -o -iname \*.cxx -o -iname \*.cpp -o -iname
\*.cc -o -iname \*.h -o -iname \*.hpp -o -iname \*.hh -o -iname \*.hxx
\) -print0 | xargs --no-run-if-empty --null -n1 include-what-you-use
<lots of warnings>
--
bye,
pabs
https://wiki.debian.org/PaulWise
Reply to: