On Sat, Apr 23, 2016 at 12:44 AM, HAYASHI Kentaro wrote: > 0.8.2 has been released, so I've updated to it. > > > http://mentors.debian.net/debian/pool/main/g/growl-for-linux/growl-for-linux_0.8.2-1.dsc
I don't intend to sponsor this, but here is a review. Some things that I think block the upload of this package: Some of the XPM/PNG images look like they were derived from proprietary icons and interfaces on Windows/etc. I don't know what they are used for (they look like screenshots) but I would encourage you to replace them. The Tux image is not under a fully DFSG-free license AFAICT and you haven't complied with the license for it either. Please remove your package from mentors.d.n until that is fixed. In addition the differing license/copyright should be documented in debian/copyright. https://en.wikipedia.org/wiki/Tux Some other files have copyright/license info that is not mentioned in debian/copyright. Some things you might want to fix at some point: Use DEP-3 headers for the patches: http://dep.debian.net/deps/dep3/ Why do you enable all hardening, disable pie in debian/rules and then enable pie in the upstream Makefile.am? I would suggest upgrading to debhelper compat 10, which automatically runs autoreconf. The override_dh_install looks like it is done in the wrong order, shouldn't the find commands come after the dh_install command? override_dh_makeshlibs looks like something that should be handled by debhelper, have you filed a bug? The upstream NEWS file is empty, don't install it in the binary package. The upstream README/README.mkd files are basically duplicates. Upstream should remove one of them but definitely don't install both into the binary package. The Standards-Version is out of date: http://www.debian.org/doc/debian-policy/upgrading-checklist The Homepage field redirects to here: https://mattn.github.io/growl-for-linux/ Please remove all the libraries hardcoded into the Depends, ${shlibs:Depends} will automatically calculate those for you. I would encourage you to get the package description reviewed by debian-l10n-english. https://wiki.debian.org/I18n/SmithReviewProject What are these files about? DO_NOT_USE_THIS_MODULE I would encourage you to include copyright/license info in all files: http://lu.is/blog/2012/03/17/on-the-importance-of-per-file-license-information/ Some of the XPM/PNG images are duplicates of each other. You should choose which is the source image and generate the other one at build time using something like convert from imagemagick. I would also generate icon_dnd.png, gol.ico and growl4linux.jpg from icon.png and a Linux mascot image. I would encourage you to use xz instead of bzip2 in the upstream makedist.sh, it is superior in every way. Please add some upstream metadata: https://wiki.debian.org/UpstreamMetadata Please add cryptographic verification of the upstream tarball: https://wiki.debian.org/debian/watch#Cryptographic_signature_verification You might want to fuzz-test the code using zzuf or afl. I wonder if the upstream code needs copyright years bumped, you have clearly worked on it since 2011: https://github.com/mattn/growl-for-linux/commits/master Automatic checks: build aclocal: warning: couldn't open directory 'm4': No such file or directory ar: `u' modifier ignored since `D' is the default (see `U') dpkg-shlibdeps: warning: debian/growl-for-linux/usr/lib/x86_64-linux-gnu/growl-for-linux/display/libnotify_gol.so.0.0.0 contains an unresolvable reference to symbol curl_easy_cleanup: it's probably a plugin dpkg-shlibdeps: warning: 5 other similar warnings have been skipped (use -v to see them all) <more> lintian P: growl-for-linux source: debian-watch-may-check-gpg-signature W: growl-for-linux: binary-without-manpage usr/bin/gol check-all-the-things # Just style checks, feel free to ignore $ find -type f \( -iname '*.sh' -o -iname '*.bash' \) -exec bashate --ignore E002,E003 {} + E010: Do not on same line as for: 'for distro in "${Ubuntus[@]}"' - ./release.sh : L16 1 bashate error(s) found $ find -type f -iname '*.sh' -exec checkbashisms {} + possible bashism in ./makedist.sh line 9 (brace expansion): rm -rf INSTALL aclocal.m4 autom4te.cache/ compile config.{sub,guess} configure depcomp install-sh ltmain.sh m4/ missing $ cme check dpkg ... Warning in 'control source Standards-Version' value '3.9.7': Current standards version is 3.9.8 Warning in 'patches:"disable-display-execstack.patch" Synopsis' value <undef>: Empty synopsis (this can be fixed with 'cme fix' command) Warning in 'patches:"add-pie-flags-for-gol.patch" Synopsis' value <undef>: Empty synopsis (this can be fixed with 'cme fix' command) $ codespell --quiet-level=3 ./gol.c:1244: adn ==> and ./gol.c:1247: adn ==> and $ cppcheck -j1 --quiet -f . [plugins/memfile.c:16]: (error) Memory pointed to by 'mf' is freed twice. $ 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 ./NEWS ./subscribe/tweets/DO_NOT_USE_THIS_MODULE $ flawfinder -Q -c . <lots> # check if these can be switched to https:// $ grep -rF http: . <several> $ 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> # Users of binary packages do not need install instructions. $ find -type f -iname '*README*' -a ! \( -iname README.md -o -iname README.rst -o -iname README.install \) -exec grep --ignore-case --fixed-strings --with-filename install {} + ./README:Installation: ./README: # make && make install ./README: # sudo apt-get install growl-for-linux ./README.mkd:Installation: ./README.mkd: # make && make install ./README.mkd: # sudo apt-get install growl-for-linux $ rpmlint . gol.spec:2: W: unversioned-explicit-provides gol gol.spec: W: no-cleaning-of-buildroot %install gol.spec: W: no-cleaning-of-buildroot %clean gol.spec: W: no-buildroot-tag gol.spec: W: no-%clean-section 0 packages and 1 specfiles checked; 0 errors, 5 warnings. $ find -type f -iname '*.sh' -exec sh -n {} \; ./release.sh: 6: ./release.sh: Syntax error: "(" unexpected $ find -type f \( -iname '*.sh' -o -iname '*.bash' -o -iname '*.zsh' \) -exec shellcheck {} + <lots> $ 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 {} + ./README: linux -> Linux ./compatibility.h: GTK -> GTK+ ./README.mkd: linux -> Linux # Potentially vulnerable to tmpfile symlink attacks $ grep -r '/tmp/' . ./release.sh:WORKDIR=/tmp/launchpad $ grep -riE 'fixme|todo|hack|xxx|broken' . ./plugins/from_url.c:// FIXME: More refactor this function. ./display/fog/fog.c: // FIXME: g_list_free_full will fail symbol lookup. ./gol.c: // TODO: absolute path ./display/balloon/balloon.c: // FIXME: g_list_free_full will fail symbol lookup. ./subscribe/rhythmbox/rhythmbox.c: // XXX: too deep!!!!!!!!!!!!!!!!! ./subscribe/rhythmbox/rhythmbox.c:// FIXME: too long!!!!!!!!!!!!!!!!!!!!! -- bye, pabs https://wiki.debian.org/PaulWise
