Bug#825302: RFS: usbguard/0.4-2 [ITP]
On Fri, May 27, 2016 at 4:19 AM, Muri Nicanor wrote:
> thanks again, such thorough reviews really help to get an understanding
> for the packaging process!
Here is another review, I don't intend to sponsor this though.
These things block the upload of this package to Debian:
debian/copyright isn't complete, some files have different
licenses/copyright holders to what is documented. I would recommend
looking at the header of each file and ensuring that is documented.
debian/changelog should have unstable instead of UNRELEASED if you are
requesting an upload to unstable.
These things block the upload of this package to Debian in my opinion
but maybe not for others:
src/ThirdParty and src/Library/RuleParser/quex contain embedded code
copies. Please ask upstream to remove them from their VCS and tarballs
and depend on them instead. You can then package them separately.
Alternatively, package them separately and remove all of them at
`debian/rules build` time before dh_auto_configure and at
`debian/rules clean` time (or just have uscan auto-repack the upstream
tarball using Files-Excluded).
https://wiki.debian.org/EmbeddedCodeCopies
The parser/lexer are not build from source. The documentation is
actually missing the source code (Markdown). Please ask upstream to
remove all the generated files from their VCS and tarballs and create
them at build time. Obviously some autotools things like ./configure
need to be in the tarball though. As long as they are regenerated at
build time using autoreconf that is fine though.
These things would be nice to fix:
The debian/changlog excerpts in debian/patches/* aren't needed.
I like to have this in my ~/.quiltrc to keep quilt-generated patches clean:
QUILT_REFRESH_ARGS="-pab --no-timestamps --no-index"
Please add some DEP-3 headers to the patches, especially Forwarded:
http://dep.debian.net/deps/dep3/
The watch file doesn't work (see the uscan output below), I think you
need to check the releases page instead.
Could you ask upstream to sign their commits, tags and release files?
And then add signature verification to debian/watch.
https://mikegerwitz.com/papers/git-horror-story
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
You can glob the manual page paths in usbguard.install:
usr/share/man/man*
Personally I dislike the "documentation and commented-out settings in
/etc/..." pattern, how systemd does it is nicer.
For the symbols file you might want to use the c++ pattern type. See
the dpkg-gensymbols manual page for more details.
I like to run this command to wrap-and-sort the debian/ directory to
make diffs easier to read:
wrap-and-sort --short-indent --wrap-always --sort-binary-packages
--trailing-comma --verbose
Please add some upstream metadata: https://wiki.debian.org/UpstreamMetadata
debhelper already passes --disable-silent-rules to ./configure so you
don't need to.
dist/usbguard.service doesn't pay attention to the --prefix, --bindir,
--etcdir, etc options passed to configure, I would suggest getting
./configure to generate it from a .in file.
The upstream README.md would be useful to install in the binary
package if it didn't contain build/install instructions. I would
suggest using sed or similar to automatically strip out those parts
and copy the result to a README file, which could then be installed.
Everything between these two headings should be stripped:
## Supported Operating Systems
## Rules
Typo in the upstream README.md: s/distributes using/distributed in/
Will you also package the usbguard-applet-qt thing mentioned in README.md?
This line in the README.md indicates a possible flaw in usbguard; can
it block new USB keyboards if there is a built-in keyboard or one
keyboard plugged in via a non-USB method such as PS/2?
### Allow a keyboard-only USB device only if there isn't already a USB
device with a keyboard interface allowed
It would be nice to have doctoc in Debian so people modifying the
README.md can auto-update the ToC.
You might want to ask upstream to add copyright/license headers on the
files that are missing those:
http://lu.is/blog/2012/03/17/on-the-importance-of-per-file-license-information/
Automated checks:
build
src/Library/LinuxDeviceManager.cpp: In member function 'virtual void
usbguard::LinuxDeviceManager::stop()':
src/Library/LinuxDeviceManager.cpp:206:41: warning: ignoring return
value of 'ssize_t write(int, const void*, size_t)', declared with
attribute warn_unused_result [-Wunused-result]
write(_event_fd, &one, sizeof one);
^
src/Library/IPCClientPrivate.cpp: In member function 'void
usbguard::IPCClientPrivate::stop()':
src/Library/IPCClientPrivate.cpp:479:38: warning: ignoring return
value of 'ssize_t write(int, const void*, size_t)', declared with
attribute warn_unused_result [-Wunused-result]
write(_eventfd, &one, sizeof one);
^
In file included from
./src/Library/RuleParser/quex/code_base/buffer/filler/BufferFiller.i:238:0,
from
./src/Library/RuleParser/quex/code_base/buffer/Buffer.i:879,
from
./src/Library/RuleParser/quex/code_base/analyzer/struct/constructor.i:7,
from
./src/Library/RuleParser/quex/code_base/analyzer/headers.i:20,
from Lexer.hpp:573,
from src/Library/RuleParser.cpp:21:
./src/Library/RuleParser/quex/code_base/buffer/filler/BufferFiller_navigation.i:
In function 'bool
quex::Lexer_BufferFiller_character_index_step_to(quex::Lexer_BufferFiller*,
intmax_t)':
./src/Library/RuleParser/quex/code_base/buffer/filler/BufferFiller_navigation.i:180:25:
warning: comparison between signed and unsigned integer expressions
[-Wsign-compare]
if( remaining_n > loaded_n ) {
^
In file included from
./src/Library/RuleParser/quex/code_base/buffer/filler/BufferFiller.i:238:0,
from
./src/Library/RuleParser/quex/code_base/buffer/Buffer.i:879,
from
./src/Library/RuleParser/quex/code_base/analyzer/struct/constructor.i:7,
from
./src/Library/RuleParser/quex/code_base/analyzer/headers.i:20,
from Lexer.hpp:573,
from src/Library/RuleParser/Lexer.cpp:2:
./src/Library/RuleParser/quex/code_base/buffer/filler/BufferFiller_navigation.i:
In function 'bool
quex::Lexer_BufferFiller_character_index_step_to(quex::Lexer_BufferFiller*,
intmax_t)':
./src/Library/RuleParser/quex/code_base/buffer/filler/BufferFiller_navigation.i:180:25:
warning: comparison between signed and unsigned integer expressions
[-Wsign-compare]
if( remaining_n > loaded_n ) {
^
dh_install --list-missing
dh_install: usr/bin/usbguard-rule-parser exists in debian/tmp but is
not installed to anywhere
dpkg-shlibdeps: warning: package could avoid a useless dependency if
debian/usbguard/usr/sbin/usbguard-daemon was not linked against
librt.so.1 (it uses none of the library's symbols)
dpkg-shlibdeps: warning: package could avoid a useless dependency if
debian/usbguard/usr/sbin/usbguard-daemon was not linked against
libdl.so.2 (it uses none of the library's symbols)
dpkg-shlibdeps: warning: package could avoid a useless dependency if
debian/libusbguard0/usr/lib/x86_64-linux-gnu/libusbguard.so.0.0.0 was
not linked against librt.so.1 (it uses none of the library's symbols)
dpkg-shlibdeps: warning: package could avoid a useless dependency if
debian/libusbguard0/usr/lib/x86_64-linux-gnu/libusbguard.so.0.0.0 was
not linked against libdl.so.2 (it uses none of the library's symbols)
lintian:
P: usbguard source: debian-watch-may-check-gpg-signature
P: usbguard: no-upstream-changelog
P: libusbguard-dev: no-upstream-changelog
P: libusbguard0: no-upstream-changelog
check-all-the-things:
$ codespell --quiet-level=3
<lots>
$ cppcheck -j1 --quiet -f .
[Parser.y:46]: (error) Uninitialized variable: possible_tokens
<possibly more, overheated my laptop>
$ debmake -k
<several differences>
$ duck
I: debian/copyright:1: URL:
http://www.debian.org/doc/packaging-manuals/copyright-format/1.0/:
INFORMATION (Certainty:possible)
The web page at
http://www.debian.org/doc/packaging-manuals/copyright-format/1.0/
works, but is also available via
https://www.debian.org/doc/packaging-manuals/copyright-format/1.0/,
please consider switching to HTTPS urls.
# Please check if these directories contain embedded code/data copies.
# Please remove any embedded copies from the upstream VCS and tarballs.
# https://wiki.debian.org/EmbeddedCodeCopies
$ find -type d -name 'vendor*' -o -iname '*rd*party' -o -name contrib
-o -name imports
./src/ThirdParty
# Please check if these README files belong to embedded code/data copies.
# Please remove any embedded copies from the upstream VCS and tarballs.
# https://wiki.debian.org/EmbeddedCodeCopies
$ find -mindepth 2 -iname '*README*'
./src/Library/RuleParser/README.md
$ flawfinder -Q -c .
<lots>
# check if these can be switched to https://
$ grep -rF http: .
<lots>
$ find -type f \( -iname '*.c' -o -iname '*.cc' -o -iname '*.cxx' -o
-iname '*.cpp' -o -iname '*.h' -o -iname '*.hh' -o -iname '*.hxx' -o
-iname '*.hpp' \) -exec include-what-you-use {} \;
<lots>
$ license-reconcile
CopyrightParsing: Invalid field given (Upstream_Contact) at
/usr/share/perl5/Debian/Copyright.pm line 131.
$ licensecheck --check=. --recursive --copyright . | grep --text -F
'GENERATED FILE'
<lots>
$ licensecheck --check=. --recursive --copyright . | grep --text -F
'with incorrect FSF address'
./aclocal.m4: GPL (v2 or later) (with incorrect FSF address) GENERATED FILE
$ 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 {} +
<lots>
$ grep -riE 'fixme|todo|hack|xxx+|broken|tba' .
<lots>
$ uscan --report-status --no-verbose
uscan warn: In watchfile debian/watch, reading webpage
https://dkopecek.github.io/usbguard/dist/ failed: 404 Not Found
--
bye,
pabs
https://wiki.debian.org/PaulWise
Reply to: