Bug#821171: RFS: growl-for-linux/0.8.1-2 [ITP]
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
Reply to: