On Mon, Dec 7, 2015 at 4:49 PM, Lennart Weller wrote: > I am looking for a sponsor for my package "network-manager-ssh"
I don't intend to sponsor this but here is a review: This might be a blocker, depends on if ftpmasters notice: debian/copyright doesn't acknowledge Dan Williams although some source files do. Here are some issues that would be nice to fix: Why do the last two overrides in debian/rules have two tab characters instead of one? The upstream README file is not useful, please install README.md instead. However README.md includes install instructions and usage instructions for people on non-Debian distros, which aren't useful to users of Debian binary packages so I would sed those parts out during `debian/rules build`. The upstream NEWS file is not useful, please don't install it. I don't think the package descriptions should describe NetworkManager, that is a job for the network-manager package description. Instead, they should describe the network-manager-ssh packages. It would be great if there were a DEP-8 test: http://dep.debian.net/deps/dep8/ http://ci.debian.net/ I always wonder if screenshots like images/*.png should be generated at build time so they never get out of date. I wonder what "PCF" in the icon means, probably it would be better to have "SSH" in there? Upstream's po files look like they have poorly maintained or unmaintained copyright headers. Upstream's test suite is using an IP address belonging to the USA Department of Defence. I would strongly suggest using a proper private IP address according to RFC 6761. http://tools.ietf.org/html/rfc6761 Please add some upstream metadata: https://wiki.debian.org/UpstreamMetadata If you contact upstream, please mention the Debian upstream guide: https://wiki.debian.org/UpstreamGuide Automated checks: check-all-the-things: $ find -type f -iname '*.sh' -exec checkbashisms {} + could not find any possible bashisms in bash script ./test/nm-ssh-server.sh $ codespell --quiet-level=3 ./src/nm-ssh-service.c:907: arguements ==> arguments $ find -type f \( -iname '*.sh' -o -iname '*.bash' -o -iname '*.zsh' \) -exec shellcheck {} + In ./test/nm-ssh-server.sh line 27: local external_interface=`_get_external_interface` ^-- SC2155: Declare and assign separately to avoid masking return values. ^-- SC2006: Use $(..) instead of legacy `..`. In ./test/nm-ssh-server.sh line 28: iptables -t nat -I POSTROUTING -o $external_interface -j MASQUERADE ^-- SC2086: Double quote to prevent globbing and word splitting. $ cppcheck -j1 --quiet -f . | grep -vF 'cppcheck: error: could not find or open any of the paths given.' [properties/nm-ssh.c:1287]: (error) Possible null pointer dereference: gateway [properties/nm-ssh.c:1289]: (error) Possible null pointer dereference: remote_ip [properties/nm-ssh.c:1290]: (error) Possible null pointer dereference: local_ip [properties/nm-ssh.c:1291]: (error) Possible null pointer dereference: netmask [properties/nm-ssh.c:1316]: (error) Possible null pointer dereference: preferred_authentication $ find \( -name .git -o -name .svn -o -name .bzr -o -name CVS -o -name .hg -o -name _darcs -o -name _FOSSIL_ -o -name .sgdrawer \) -prune -o -empty -print ./ChangeLog ./NEWS ./intltool-extract.in ./intltool-update.in ./intltool-merge.in $ flawfinder -Q -c . <lots> $ find -type f \( -iname '*.po' -o -iname '*.pot' \) -exec POFileChecker {} + <lots> $ find -type f \( -iname '*.po' -o -iname '*.pot' \) -exec POFileSpell {} + <lots> $ find -type f \( -iname '*.po' -o -iname '*.pot' -o -iname '*.mo' -o -iname '*.gmo' \) -exec i18nspector {} + <lots> $ find -type f \( -iname '*.po' -o -iname '*.pot' \) -exec msgfmt --check --check-compatibility --check-accelerators --output-file=/dev/null {} \; <lots> $ grep -riE 'fixme|todo|hack|xxx' . <lots> -- bye, pabs https://wiki.debian.org/PaulWise
