[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#763540: Review of psocksxx/0.0.5-1



Hello Paul,
Hello Harlan,

thanks for the review.

Am Mittwoch, den 01.10.2014, 12:50 +0800 schrieb Paul Wise:
> On Wed, Oct 1, 2014 at 10:18 AM, Harlan Lieberman-Berg wrote:
> 
[...]
> 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.
changed in d/rules.

> 
> libpsocksxx0 is missing a Pre-Depends on multiarch-support as it
> doesn't use ${misc:Pre-Depends}.
added

> You might want to build-dep on libcppunit-dev so that the build-time
> test suite is enabled.
added
>
> The git repository mentioned in Vcs-* does not exist.
My opinion was to prevent a repository with a not publish packet and to
save space. So i would create the repository shortly before upload into
the new queue. But I have done it now. 
> 
> 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.
The reason is that I want use GPL-3+ for my work. But for your intention
I set the patch to the license of the source file(s). I thinks that's a
good way, especially by sources with more then one licenses.

> 
> 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.
Removed
> 
> override_dh_installdocs should be removed, the make part should be in
> override_dh_auto_build and the cp part in libpsocksxx-doc.docs.
I have move the build part to override_dh_auto_build.
I want to copy the doxygen generated docu in a subdir (docs). With
d/*.docs are all files in doc directory. A second parameter link in
d/*.install are not supported.

> 
> You can pass arguments to dpkg-gensymbols like this: dh_makeshlibs --
> -plibpsocksxx0
I have removed the complete override

> 
> 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.
ok. Done

> 
> I would suggest not installing the static library (remove *.a from
> debian/*.install) unless someone files a bug asking for it.
Removed

>
> README.md is not needed in the libpsocksxx0 library package since that
> will only be installed as a dep of other things.
Removed

> 
> 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.
Ok. Done
> 
> The upstream test suite is hardcoding paths to test sockets in to
> /tmp, it should create those sockets in the test/build directory
> instead
Under work
> .
> 
> 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
This duplicate files comes from doxygen. Is a override necessary?
Until now I only know the only errors and warnings are to be
overwritten.

> 
> $ 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;
I have removed the (>= 1.12.1)

> Warning in 'control binary:"libpsocksxx-dev" Synopsis' value 'psocksxx
> is a C++ wrapper for POSIX sockets (development files)': Synopsis is
> too long.
A little reduced

> 
> $ 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
see above

> 
> $ 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"
see above

> 
> $ grep -i warn ../*.build
> configure: WARNING: Cppunit not found - continuing without unit test support
see above

> 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.
I have written a a patch to comment out this vars

> dpkg-gencontrol: warning: package libpsocksxx0: unused substitution
> variable ${misc:Pre-Depends}
see above

> 
> $ 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.
No changes

> 
> $ 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>
> 

New package is uploaded to mentors[1].


> -- 
> bye,
> pabs
> 
> https://wiki.debian.org/PaulWise
> 
> 

CU
Jörg

[1] http://mentors.debian.net/debian/pool/main/p/psocksxx/psocksxx_0.0.5-1.dsc


-- 
pgp Fingerprint: 7D13 3C60 0A10 DBE1 51F8  EBCB 422B 44B0 BE58 1B6E
pgp Key: BE581B6E
CAcert Key S/N: 0E:D4:56

Jörg Frings-Fürst
D-54526 Niederkail

Threema: SYR8SJXB

IRC: j_f-f@freenode.net
     j_f-f@oftc.net





Attachment: signature.asc
Description: This is a digitally signed message part


Reply to: