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
