On Fri, 2016-04-22 at 16:20 -0300, Pavel Kalian wrote:
> Unfortunately I still did not receive any useful feedback from
> anybody, neither on Debian GIS mailing list, or any information on
> where I should submit something for review.
Generally it works like this:
https://mentors.debian.net/intro-maintainers
Here is a review of this package:
https://launchpad.net/~nohal/+archive/ubuntu/opencpn/+files/opencpn_4.2.0-0~xenial1.dsc
Based on my review, not much has been fixed since I last reviewed it.
In my opinion these issues block the upload of this package:
Others may accept the package despite these issues but I wouldn't.
All the embedded code copies (wxSVG etc) need to be removed from the
upstream VCS repo and tarballs and packaged separately (some are now).
For folks on other platforms they could provide a dependencies tarball.
https://wiki.debian.org/EmbeddedCodeCopies
I encourage de-forking the modified embedded code copies (zygrib etc).
I encourage de-duplicating the duplicated embedded code copies (wxJSON etc).
I encourage removing the non-free embedded code copies (unrar) and
replacing them with dependencies on free equivalents (unar).
There are some generated files in the source package (see the
licensecheck output below), these should be removed from the upstream
VCS repo and tarballs and created at build time by the upstream build
system instead.
Some of the generated PNG images have got out of sync with their SVG
source, upstream needs to investigate this.
Some of the generated PNG images (pkg_background.jpg for eg) look like
they were generated from proprietary maps.
Some of the generated files (MathJax.js and *.min.*) do not have their
corresponding source available in the package. This may be either a GPL
violation or a DFSG violation depending on the license of those files.
Helvetica.txf seems to have been generated from a proprietary font.
There are some Depends/Recommends that need to be packaged and uploaded
before opencpn can be uploaded.
The debian/copyright file is not accurate, it needs to document the
copyright and licenses of all the files in the source package.
https://www.debian.org/doc/packaging-manuals/copyright-format/1.0/
http://people.skolelinux.org/pere/blog/Creating__updating_and_checking_debian_copyright_semi_automatically.html
In my opinion these issues would be nice to fix:
Please pass Debian's guide for upstreams along to the OpenCPN devs:
https://wiki.debian.org/UpstreamGuide
data/license.txt is a copy of the GPL, there is no need to install that
in the package via debian/docs since it is in the common licenses dir.
/usr/share/common-licenses/
The new location for DEP-5 is on the Debian website:
https://www.debian.org/doc/packaging-manuals/copyright-format/1.0/
I suggest upgrading to debhelper compat 10 and using dh instead of
CDBS, but those are my personal preferences.
I suggest wrapping and sorting the debian/ dir using this command:
wrap-and-sort --short-indent --wrap-always --sort-binary-packages --trailing-comma --verbose
Is there any reason not to uncomment the OpenGL ES dependencies?
#For the ARM build, perhaps add: , libdri2-dev [armhf],libgles1-mesa-dev [armhf]
OpenGL ES is available on all Debian packages of Mesa so you don't need
to restrict it to armhf though.
The commented out Vcs-* URLs don't exist, I would remove them.
The URL in the Homepage field redirects to here:
http://opencpn.org/ocpn/
The package description doesn't need to mention the license because
that is in debian/copyright. It doesn't need to mention that opencpn is
free software because it would be in Debian main if accepted.
You might want to update the package to the latest Standards-Version:
http://www.debian.org/doc/debian-policy/upgrading-checklist
How are you creating/editing debian/changelog? The normal method (dch)
inserts a blank line between the entries.
Some of the SVG files in the plugin dirs seem to be duplicated in the
src/ and data/ subdirs.
If possible I would recommend the screenshots be generated at build
time so that they represent the current platform being built for and
also so they don't get outdated as UI elements change.
data/copyright and data/changelog.Debian.gz look like an aborted
attempt at Debian packaging and should be removed.
Please add a debian/watch file:
https://wiki.debian.org/debian/watch
Please add some upstream metadata:
https://wiki.debian.org/UpstreamMetadata
Please consider fuzz testing any installed programs that could parse
untrusted input using zzuf and afl.
I would suggest upstream remove uncontrolled advertising from their
download page, especially if it is hijacking downloads.
Automated checks:
build
lots of compiler warnings
lots of dpkg-shlibdeps warnings
lintian
P: opencpn source: duplicate-in-relation-field in source build-depends: libgtk2.0-dev, libgtk2.0-dev
P: opencpn source: insane-line-length-in-source-file plugins/chartdldr_pi/data/doc/MathJax.js line length is 32123 characters (>512)
P: opencpn source: source-contains-prebuilt-javascript-object plugins/chartdldr_pi/data/doc/MathJax.js line length is 32123 characters (>512)
E: opencpn source: source-is-missing plugins/chartdldr_pi/data/doc/MathJax.js line length is 32123 characters (>512)
P: opencpn source: source-contains-prebuilt-javascript-object plugins/chartdldr_pi/data/doc/highlight.min.js
E: opencpn source: source-is-missing plugins/chartdldr_pi/data/doc/highlight.min.js
P: opencpn source: package-uses-old-debhelper-compat-version 8
P: opencpn source: unversioned-copyright-format-uri http://dep.debian.net/deps/dep5
W: opencpn source: dep5-copyright-license-name-not-unique (paragraph at line 24)
W: opencpn source: ancient-standards-version 3.9.3 (current is 3.9.7)
I: opencpn source: debian-watch-file-is-missing
<more>
check-all-the-things
# Checks style, feel free to ignore
$ find -type f \( -iname '*.sh' -o -iname '*.bash' \) -exec bashate --ignore E002,E003 {} +
<lots>
# Check with upstream where the Inkscape SVG source files are.
$ find -type f \( -iname '*.png' -o -iname '*.gif' -o -iname '*.jpg' -o -iname '*.jpeg' \) -exec grep -iF inkscape {} +
Binary file ./plugins/wmm_pi/src/wmm_pi.png matches
Binary file ./plugins/wmm_pi/src/wmm.png matches
Binary file ./plugins/chartdldr_pi/data/open182.png matches
Binary file ./plugins/chartdldr_pi/data/folder215.png matches
Binary file ./plugins/chartdldr_pi/src/chartdldr.png matches
Binary file ./plugins/chartdldr_pi/src/chartdldr_pi.png matches
Binary file ./data/opencpn.png matches
Binary file ./src/bitmaps/opencpn.png matches
Binary file ./src/bitmaps/other_svg_src/opencpn_mobile0.png matches
$ find -type f -iname '*.sh' -exec checkbashisms {} +
could not find any possible bashisms in bash script ./plugins/grib_pi/src/icons.sh
could not find any possible bashisms in bash script ./plugins/wmm_pi/src/icons.sh
could not find any possible bashisms in bash script ./plugins/dashboard_pi/src/icons.sh
could not find any possible bashisms in bash script ./plugins/chartdldr_pi/src/icons.sh
could not find any possible bashisms in bash script ./src/bitmaps/28x28_svg_src/create_all_28x28.sh
could not find any possible bashisms in bash script ./src/bitmaps/32x32_svg_src/ribbon/create_all_32x32.sh
could not find any possible bashisms in bash script ./src/bitmaps/32x32_svg_src/cursor/create_all_32x32.sh
could not find any possible bashisms in bash script ./src/bitmaps/16x16_svg_src/create_all_16x16.sh
could not find any possible bashisms in bash script ./src/bitmaps/13xX_svg_src/create_all_13xX.sh
could not find any possible bashisms in bash script ./src/bitmaps/other_svg_src/create_ship.sh
could not find any possible bashisms in bash script ./src/bitmaps/other_svg_src/create_opencpn_main_icon.sh
$ cme check dpkg
...
Warning in 'control source Standards-Version' value '3.9.3': Current standards version is 3.9.8
Warning in 'control binary:opencpn Depends:3' value 'opencpn-tcdata': package opencpn-tcdata is unknown. Check for typos if not a virtual package.
Warning in 'control binary:opencpn Depends:4' value 'opencpn-gshhs-low': package opencpn-gshhs-low is unknown. Check for typos if not a virtual package.
Warning in 'control binary:opencpn Recommends:2' value 'opencpn-doc': package opencpn-doc is unknown. Check for typos if not a virtual package.
Warning in 'control binary:opencpn Recommends:3' value 'opencpn-gshhs-crude': package opencpn-gshhs-crude is unknown. Check for typos if not a virtual package.
Warning in 'control binary:opencpn Recommends:4' value 'opencpn-gshhs-intermediate': package opencpn-gshhs-intermediate is unknown. Check for typos if not a virtual package.
Warning in 'control binary:opencpn Recommends:5' value 'opencpn-gshhs-high': package opencpn-gshhs-high is unknown. Check for typos if not a virtual package.
Warning in 'control binary:opencpn Recommends:6' value 'opencpn-gshhs-full': package opencpn-gshhs-full is unknown. Check for typos if not a virtual package.
Warning in 'control binary:opencpn Conflicts:0' value 'opencpn-plugin-wmm': package opencpn-plugin-wmm is unknown. Check for typos if not a virtual package.
Warning in 'control binary:opencpn Conflicts:1' value 'opencpn-plugin-chartdldr': package opencpn-plugin-chartdldr is unknown. Check for typos if not a virtual package.
Warning in 'control binary:opencpn Replaces:0' value 'opencpn-plugin-wmm': package opencpn-plugin-wmm is unknown. Check for typos if not a virtual package.
Warning in 'control binary:opencpn Replaces:1' value 'opencpn-plugin-chartdldr': package opencpn-plugin-chartdldr is unknown. Check for typos if not a virtual package.
Warning in 'control binary:opencpn Description' value 'OpenCPN is a free software (GPLv2) project to create a concise chartplotter and navigation software for use as an underway or planning tool. OpenCPN is developed by a team of active sailors using real world conditions for program testing and refinement.': Line too long in description
Warning in 'copyright Format' value 'http://dep.debian.net/deps/dep5': Format does not match the recommended URL for DEP-5
Warning in 'control source Build-Depends': Duplicated value: 11:"libgtk2.0-dev"
$ codespell --quiet-level=3
<lots>
$ cppcheck -j1 --quiet -f .
[plugins/chartdldr_pi/src/unrar/extract.cpp:36]: (error) Uninitialized struct member: FD.Size
[plugins/chartdldr_pi/src/unrar/recvol5.cpp:342]: (error) Memory leak: Data
<more>
$ debmake -k
<lots>
$ find -type f -iname '*.desktop' -exec desktop-file-validate {} \;
./data/opencpn.desktop: hint: value "Education;Science;Geography;" for key "Categories" in group "Desktop Entry" contains more than one main category; application might appear more than once in the application menu
$ 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
./src/glu/libutil/.dirstamp
$ fdupes -q -r . | grep -vE '/(\.(git|svn|bzr|hg|sgdrawer)|_(darcs|FOSSIL_)|CVS)(/|$)' | cat -s
<lots>
$ grep -Er '/(home|srv|opt)(\W|$)' .
<lots>
$ 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>
# check if these can be switched to https://
$ grep -rF http: .
<lots>
$ find -type f \( -iname '*.po' -o -iname '*.pot' -o -iname '*.mo' -o -iname '*.gmo' \) -exec i18nspector {} +
<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>
$ find -type d \( -iname .git -o -iname .svn -o -iname .bzr -o -iname CVS -o -iname .hg -o -iname _darcs -o -iname _FOSSIL_ -o -iname .sgdrawer \) -prune -o -type f ! \( -iname '*.blend' -o -iname '*.icns' -o -iname '*.bmp' -o -iname '*.ico' -o -iname '*.png' -o -iname '*.gif' -o -iname '*.jpg' -o -iname '*.jpeg' -o -iname '*.tga' -o -iname '*.xcf' -o -iname '*.tif' -o -iname '*.tiff' -o -iname '*.mo' -o -iname '*.gmo' -o -iname '*.gz' -o -iname '*.bz2' -o -iname '*.xz' -o -iname '*.lz' -o -iname '*.zip' -o -iname '*.tar' -o -iname '*.deb' -o -iname '*.pdf' -o -iname '*.odt' -o -iname '*.docx' -o -iname '*.doc' -o -iname '*.chm' -o -iname '*.torrent' -o -iname '*.pyc' -o -iname '*.pyo' -o -iname '*.o' -o -iname '*.so' -o -iname '*.so.*' -o -iname '*.debug' -o -iname '*.wav' -o -iname '*.ogg' -o -iname '*.oga' -o -iname '*.ogv' -o -iname '*.mid' -o -iname '*.mp3' -o -iname '*.flac' -o -iname '*.ttf' -o -iname '*.otf' -o -iname '*.fon' -o -iname '*.pgp' -o -iname '*.gpg' -o -iname '*.dat' \) -exec isutf8 {} +
./plugins/dashboard_pi/src/wind.cpp: line 190, char 1, byte offset 29: invalid UTF-8 code
./plugins/chartdldr_pi/src/unrar/rs16.cpp: line 147, char 1, byte offset 15: invalid UTF-8 code
$ license-reconcile
<lots>
$ grep -rE -- '-m64|-m32' .
./plugins/chartdldr_pi/src/unrar/makefile:#CXXFLAGS=-O3 -mcpu=v9 -mtune=ultrasparc -m32
$ find -type f \( -iname '*.po' -o -iname '*.pot' \) -exec msgfmt --check --check-compatibility --check-accelerators --output-file=/dev/null {} \;
<lots>
$ find -type f -iname '*.py' -exec pep8 --ignore W191 {} +
./src/glshim/spec/gen.py:15:1: E302 expected 2 blank lines, found 1
./src/glshim/spec/gen.py:43:1: E302 expected 2 blank lines, found 1
./src/glshim/spec/gen.py:56:1: E302 expected 2 blank lines, found 1
./src/glshim/spec/gen.py:66:1: E302 expected 2 blank lines, found 1
./src/glshim/spec/gen.py:73:1: E302 expected 2 blank lines, found 1
./src/glshim/spec/gen.py:116:16: E713 test for membership should be 'not in'
./src/glshim/spec/gen.py:135:80: E501 line too long (81 > 79 characters)
./src/glshim/spec/gen.py:140:80: E501 line too long (81 > 79 characters)
./src/glshim/spec/xml/toyml.py:53:20: E713 test for membership should be 'not in'
./src/glshim/spec/xml/toyml.py:53:80: E501 line too long (80 > 79 characters)
./src/glshim/spec/xml/toyml.py:110:1: W293 blank line contains whitespace
$ perlcritic -1 . 2>&1 | grep -vF 'No perl files were found.'
<lots>
$ find -type f -iname '*.py' -exec pyflakes3 {} +
./src/glshim/spec/gen.py:158:13: invalid syntax
print gen(files, args.template, args.name,
^
./src/glshim/spec/xml/toyml.py:99:38: invalid syntax
print 'unrecognized root tag:', xml.tag
$ find -type f -iname '*.py' -exec pylint --msg-template='{path}:{line}:{column}: [{category}:{symbol}] {obj}: {msg}' --reports=n {} +
<lots>
$ find -type f -iname '*.py' -exec pylint3 --msg-template='{path}:{line}:{column}: [{category}:{symbol}] {obj}: {msg}' --reports=n {} +
No config file found, using default configuration
************* Module gen
src/glshim/spec/gen.py:158:0: [error:syntax-error] : invalid syntax
************* Module toyml
src/glshim/spec/xml/toyml.py:99:0: [error:syntax-error] : Missing parentheses in call to 'print'
# 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 {} +
./data/sounds/README.bells:Installation
./data/sounds/README.bells:The files 1bells, 2bells..., 8bells should be placed in the "sounds" directory under the main OpenCPN installation directory. The sounds are enabled by selecting the appropriate option in the Toolbox->Etc dialog.
./data/doc/readme:To link to "Installing Charts", the link that users see and click on _must_ be "Installing Charts", not even including a final "." !!
$ 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 {} +
<lots>
$ suspicious-source
./plugins/wmm_pi/unused-data/EGM9615.BIN
./data/changelog.Debian.gz
./data/s57data/Helvetica.txf
./data/s57data/S52RAZDS.RLE
# Prevents reproducible builds: http://reproducible-builds.org/
$ grep -rE ' __DATE__|__TIME__' .
./src/glshim/src/glx/glx.c: printf("libGL: built on %s %s\n", __DATE__, __TIME__);
# Potential tmpfile vulnerabilities
$ grep -r '/tmp/' .
./plugins/grib_pi/src/email.cpp: filename.Printf(wxT("/tmp/msg-%ld-%ld-%ld.txt"), (long) getpid(), wxGetLocalTime(),
./src/chart1.cpp: f = fopen("/var/tmp/serial", "r");
./src/chart1.cpp: f = fopen("/var/tmp/usbserial", "r");
./src/mygdal/cpl_path.cpp: * CPLProjectRelativeFilename("abc/def","tmp/abc.gif") == "abc/def/tmp/abc.gif"
./src/mygdal/cpl_path.cpp: * CPLProjectRelativeFilename("abc/def","/tmp/abc.gif") == "/tmp/abc.gif"
./src/ocpnhelper.c: if((ouF = open("/var/tmp/usbserial", O_WRONLY | O_CREAT | O_TRUNC, 0777)) != -1)
./src/ocpnhelper.c: if((ouF = open("/var/tmp/serial", O_WRONLY | O_CREAT | O_TRUNC, 0777)) != -1)
./src/ocpnhelper.c: unlink("/var/tmp/usbserial");
./src/ocpnhelper.c: unlink("/var/tmp/serial");
./src/glshim/src/gl/pixel.c: snprintf(filename, 64, "/tmp/tex.%d.ppm", name);
$ grep -riE 'fixme|todo|hack|xxx|broken' .
<lots>
$ find .. -maxdepth 1 -type f -iwholename '../*.build' -exec blhc --all {} +
CXXFLAGS missing (-fPIE): <lots>
LDFLAGS missing (-Wl,-z,relro -Wl,-z,now): ...
$ grep -H -i warn ../*.build
<lots>
--
bye,
pabs
https://wiki.debian.org/PaulWise
Attachment:
signature.asc
Description: This is a digitally signed message part