Bug#538067: status of the opencpn PPA for inclusion in Debian
Paul…
Many thanks for the review.
> On Apr 23, 2016, at 00:34, Paul Wise <pabs@debian.org> wrote:
>
> 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
The linked document says “Debian packages *should* not use convenience copies”, is it now changed to a hard requirement? It would IMO be unfortunate for the upstream to have to make the build from Git more complicated and per se broken for 99% of the people because of the packaging requirements of one single flavor of one single platform, especially as the files possibly offending Debian can be removed during the creation of the tarball. The embedded code also receives fixes for problems that affect us, which is just natural to handle in a VCS, not by making people watch for updates to some tarball (Which they fail to do as we know by experience). We do not include copies of libraries that do not need patching or where upstream exists and accepts patches.
Also, I wonder what sense does it make to create packages nobody else uses? (https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=813151)
>
> I encourage de-forking the modified embedded code copies (zygrib etc).
While OpenCPN’s grib implementation originally came from zygrib many years ago, they are completely different now, how do you imagine the de-forking to be done? Same with GDAL etc.
Is this a hard requirement? If so, I’m afraid the Debian inclusion is out of question as zygrib internally is a Qt application, not a library, while OpenCPN is a wxWidgets application.
>
> I encourage de-duplicating the duplicated embedded code copies (wxJSON etc).
It is upstream policy that the plugins have to be buildable completely independently of the core source tree.
>
> I encourage removing the non-free embedded code copies (unrar) and
> replacing them with dependencies on free equivalents (unar).
No problem, done and ready to be pushed upstream.
>
> 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.
There is no problem as far as I can see, the SVGs being “out of sync” are not meant to be “in sync” but are “original artwork templates".
>
> Some of the generated PNG images (pkg_background.jpg for eg) look like
> they were generated from proprietary maps.
Do you say that taking a screenshot of a public domain chart (NOAA copyright states “No copyright is claimed by the United States Government under Title 17 U.S.C.") produces an image incompatible with GPL? They of course can be removed, I’m just wondering if this situation is really considered problematic.
>
> 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.
Resolved, ready to be ushed upstream.
>
> Helvetica.txf seems to have been generated from a proprietary font.
I’m confused, this file is already distributed by Debian in several packages. Is it now a hard requirement to replace it with another font in new packages?
>
> There are some Depends/Recommends that need to be packaged and uploaded
> before opencpn can be uploaded.
You mean the opencpn-* data packages? Yes, they will be submitted once it will look like there is a chance the packaging effort is going to be succesful. Should be much easier than the code.
>
> 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.
Does this have something to do with Debain packaging? Is it a hard requirement? Not saying upstream would not like to do the same, just curious.
>
> 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
>
>
Reply to: