hello pabs, hello *, On 05/27/2016 06:29 AM, Paul Wise wrote: > Here is another review, I don't intend to sponsor this though. thanks for the review! again, i've learned a lot ;) i'v now pushed everythin also to https://gitlab.com/muri/usbguard-debian/, but not yet updated the package on mentors, because the build dependencies are now unresolvable > 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. oke, i've fixed that to my best knowlegde. i've created a list with files that lack any license and will file a bug upstream about that. > debian/changelog should have unstable instead of UNRELEASED if you are > requesting an upload to unstable. fixed > 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). oke, so: i've filed #825620 (ITP: quex), #825623 (ITP: json), #825625 (ITP: usbmon) i've tried to exclude the folders using Files-Exclude, but that didn't seem to work. i'm now deleting them in override_dh_auto_configure and override_dh_auto_clean. as soon as all the dependencies are packaged, i'll file a bug report about removing the bundled code. > The parser/lexer are not build from source. The documentation is > actually missing the source code (Markdown). i don't get the meaning of that last sentence, could you elaborate? > 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. oke > These things would be nice to fix: > > The debian/changlog excerpts in debian/patches/* aren't needed. fixed > I like to have this in my ~/.quiltrc to keep quilt-generated patches clean: > > QUILT_REFRESH_ARGS="-pab --no-timestamps --no-index" thanks for the tip! > Please add some DEP-3 headers to the patches, especially Forwarded: done > The watch file doesn't work (see the uscan output below), I think you > need to check the releases page instead. fixed > Could you ask upstream to sign their commits, tags and release files? putting it on my TODO list > You can glob the manual page paths in usbguard.install: fixed > Personally I dislike the "documentation and commented-out settings in > /etc/..." pattern, how systemd does it is nicer. what do you mean by 'how systemd does it'? > For the symbols file you might want to use the c++ pattern type. See > the dpkg-gensymbols manual page for more details. oke > 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 thanks for the tip, done! > Please add some upstream metadata: https://wiki.debian.org/UpstreamMetadata done > debhelper already passes --disable-silent-rules to ./configure so you > don't need to. removed. > 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. adding to my TODO list > 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: adding to my TODO list > Will you also package the usbguard-applet-qt thing mentioned in README.md? yes, as far as i can see, the qt-applet will also be part of the source package in the next release thanks a lot for all the tips! -- muri
Attachment:
signature.asc
Description: OpenPGP digital signature