Re: RFS: opencpn
Hi,
this is an update on this from a year ago:
http://lists.debian.org/debian-mentors/2010/02/msg00115.html
after a while and a fair bit of work upstream* I think we're ready for
another shot at review. *[mainly hard work by Anton and Dave]
In the last year the program has enjoyed phenomenal growth in popularity
for a niche product, now ~400 downloads per day from sourceforge:
http://sourceforge.net/project/stats/detail.php?group_id=180842&ugn=opencpn&type=prdownload&mode=alltime&file_id=0
That success is well deserved! It's a fine program.
packaging efforts are being coordinated here:
http://www.cruisersforum.com/forums/f134/opencpn-build-on-debian-35977-8.html#post667237
Highlights since last time:
- copyright review conducted upstream
- upstream now uses Git, source tree cleaned up
- arch-independent -data and -doc split off
- depends on xtide's coastline package instead of shipping a dupe
- a lot of patches based on the last review now applied upstream
debian/ dir can be viewed here:
http://svn.debian.org/viewsvn/pkg-grass/packages/opencpn/trunk/debian/
to play along at home:
git clone git://opencpn.git.sourceforge.net/gitroot/opencpn/opencpn
svn co svn://svn.debian.org/pkg-grass/packages/opencpn/trunk/debian opencpn/debian
cd opencpn/
#review+install any build depends ...
grep -A1 'Build-Depends:' debian/control
# ...
debuild -i -uc -us -b
I've done my package building on Squeeze. Maybe package deps have
changed in sid, maybe not? I haven't set up a sid chroot or VM on
my new machine yet. I mention this as some gtk issues have been
reported on Ubu 11.04 in the last days and are currently being
diagnosed.
To address Paul's earlier comments:
--
> On Mon, Feb 8, 2010 at 4:06 PM, Paul Wise <pabs@debian.org> wrote:
>
> > I'll take a closer look at the package when I'm on a Debian system.
>
> I took a much closer look at the package and in summary; I'd really
> like to sponsor this package but it needs a lot of work upstream,
> especially in the copyright/license department. Please do contact this
> list again when you have worked with upstream to resolve the issues
> listed below. Should you have further questions, please don't hesitate
> to contact this list.
... and so here we are
> I think upstream is installing the icons in the wrong directory. IIRC
> it should be be installed as
> /usr/share/icons/hicolor/48x48/apps/opencpn.png for bitmaps of size 48
> x 48. XPM is not a good format for modern desktops since it has only
> 1-bit transparency and therefore no possibility of anti-aliased
> transparency. Unfortunately the Debian menu system requires it. I'd
> suggest that the .xpm file be installed in /usr/share/pixmaps, IIRC
> that has a lower priority in most desktops than /usr/share/icons/.
> Also, one of the opencpn.svg files could be shipped as
> /usr/share/icons/hicolor/scalable/apps/opencpn.svg
done upstream.
> The file 136ReleaseNotes might be relevant to install, in
> /usr/share/doc/opencpn perhaps. Hmm, half of it seems to be exactly
> the same as the changelog so maybe not. I suggest contacting upstream
> about it. Looks like upstream is using the ChangeLog as a NEWS file
> but have abandoned the NEWS file after the 1.2.6 release and now store
> NEWS entries in the ChangeLog instead. Whee! The NEWS file should
> summarise user-visible changes between releases and the ChangeLog file
> should be similar to VCS logs. The GNU Coding Standards document has
> more detail here:
>
> http://www.gnu.org/prep/standards/standards.html#NEWS-File
> http://www.gnu.org/prep/standards/standards.html#Change-Logs
cleaned up/out upstream.
> I'm confused, is upstream using both autotools and CMake?
now it's cmake.
> src/grib/bzip2 contains an embedded code copy of bzip2. Please ask
> upstream to remove it from the tarball. Same for src/grib/zlib-1.2.3.
> Software maintained elsewhere should not be in the tarball. Same for
> src/grib. All 3 are already in Debian:
>
> http://packages.debian.org/zygrib
> http://packages.debian.org/zlib
> http://packages.debian.org/bzip2
>
> src/mygdal, src/myiso8211, src/nmea0183 all look like embedded code
> copies too. If the reason that they are in the tarball is for
> operating systems like Windows that don't have good
> packaging/repository/dependency systems, I'd suggest making a separate
> tarball containing tarballs for all the dependencies. warzone2100 does
> this and it seems to work well.
You guessed well; bzip2 and zlib are there for the MS Windows port [Grib
plugin]. The cmake rules are patched to use the system's zlib, bzip2 if
UNIX. It is expected this patch will be applied upstream in the near
future. Released tarballs are actively targeted at Linux, Mac OSX,
and MS Windows builds; I am not going to ask upstream to issue a
special stripped down version of the tarball every release just for us,
or maintain such a thing myself. If there was some sort of license
issue I would, but for the sake of an unused 200kb(uncompressed) in
the .orig.tar.gz, it just ain't worth the effort.
Grib code has been moved to a plugin. ZyGrib does not supply a lib
package, nor even use libaries within its main package- it's a stand
alone binary. So OpenCPN clone some of their reader code. It is beyond
the scope of this packaging effort to refactor a 3rd party's application.
(aside: I suspect their debian packaging needs a little work too)
So far just two plugins ship with the code. It is expected that more
will come along (see the opencpn download page for some works in
progress), and when that happens we will most likely split them out
into their own binary package. But for now it's not worth the effort.
As for src/mygdal and src/myiso8211, GDAL does supply nice library
packages but OpenCPN has modified and adapted them. Dave (the OpenCPN
lead author) had this to say when asked:
Dave wrote:
> > Hamish....
> >
> > We could use libgdal, with what I would call "moderate"
> > effort, with some loss of functionality.
> >
> > There are good reasons not to. libgdal is very capable, but
> > not optimum for dynamic GUI based systems like ours. Its
> > great for generating static data from S57 cells.
> >
> > I tweaked some of the code to compile on Windows, eliminate
> > redundancy, reduce memory footprint, fix some obvious bugs
> > present in the version I started with, and add call backs to
> > support Progress Dialogs on SENC creation. In theory, these
> > fixes should go upstream to GDAL. In practice, though.....
> >
> > IMHO, this kind of improvement for a specific application
> > should not block debian repo inclusion. Otherwise we wear a
> > ball and chain, and innovation is stifled.
> >
> > Thanks Dave
As for src/nmea0183, AFAIK it does not exist in library form elsewhere
in Debian, and if copyright+Author header comments are anything to go
by has been specifically tailored/reworked by the OpenCPN lead dev.
> src/nmea0183 has no copyright information and this as a license:
>
> You can use it any way you like.
>
> Unfortunately this does not give Debian permission to copy, modify or
> distribute.
AFAIU the original author was contacted by upstream, the reply was
"It is BSD license, do with it what you will"
which is now given in the header comments there.
> Another embedded code copy, Stackwalker.cpp, probably could be
> replaced by valgrind.
AFAICT that's no longer in the source tree.
> bbox.cpp looks like it was copied from a really old version of wxWidgets;
>
> http://wxwindows2.4.sourcearchive.com/documentation/2.4.4.1.1ubuntu2/bbox_8cpp-source.html
>
> Surely newer versions of wxWidgets contain similar functionality that
> could be used instead of this code copy. There are other files that
> look like they are similar; inpcons.h dymemdc.h inphand.h uniparts.h
> and anything containing the phrase "Purpose: Optimized".
This one I haven't looked into very hard. Only that bbox.cpp seems
like a pretty minimal bit of code, and I'm aware that between now and
the next stable release one main set of changes planned is to rewrite
some of the windowing/toolbar code.
aka waiting for the dust to settle before worrying about that too much.
> The OpenCPN code contains this template:
>
> * Copyright (C) $YEAR$ by $AUTHOR$ *
> * $EMAIL$ *
>
> Upstream really needs to replace these by real information. Some files
> have the right info but still have the template.
AFAICT $AUTHOR$+$YEAR$ fixed upstream. Some $EMAIL$ remain. I've filed
that as a low-priority bug upstream, but as email is not a required part
of the copyright I don't think it's a blocker for this packaging effort,
and will be fixed in due course.
> BTW, the template is wrong since (C) is not a valid symbol for
> copyright, IIRC only "Copyright", "Copyr." and "©" are.
"Copyright" is there, which is enough to get the job done. I'll leave
the fight about copyright symbols to others; just to note AFAIK in the
history of copyright law no one has ever had the chutzpah to try and
claim invalid copyright based on that in front of a judge.
> src/georef.c has this non-free license (non-commercial use/etc is non-free):
>
> /* Permission to use, copy, modify, and distribute this software and its */
> /* documentation for non-commercial purpose, is hereby granted without fee, */
> /* providing that the copyright notice appear in all copies and that both */
> /* the copyright notice and this permission notice appear in supporting */
> /* documentation. I make no representations about the suitability of this */
> /* software for any purpose. It is provides "as is" without express or */
> /* implid warranty. */
>
> Similar for gpc.h georef.h
AFAIU the original author was contacted here as well, and the headers
have been updated accordingly.
gpc.h no longer exists.
> macsercomm.cpp looks slightly suspect. I'd like to know its lineage,
> copyright and license. Same for macutils.c
src/about.cpp lists seriallib as being GPL.
We can request better header comments.
> routeprop.cpp
author/copyright now added (written by the opencpn lead dev)
> scrollingdialog.cpp
> scrollingdialog.h
now listed as wxWindows licence (LGPL)
> sercomm.cpp macsercomm.h macutils.h
> sercomm.h
I expect it's from serialib & so GPL; see above.
> routeprop.h tcmgr.h
author/copyright now added (written by the opencpn lead dev)
> triangulate.h
author/copyright now added (public domain)
> data/s57data/*
author/copyright now added: from Sylvain Duclos's libS52 (GPL)
libS52 can be found in the the contrib section of the OpenEV
cvs repo at SourceForge. (years ago openev was the semi-reference
data viewer for GDAL)
> data/sounds/alarm2.wav contain these comments:
>
> Copyright . Cinematronics 1995
> Microsoft Plus! . for Windows 95
>
> It is highly likely that it is non-free, and I'd wager that alarm1.wav is too.
replaced by new from-scratch versions. see /usr/share/doc/opencpn-data/README.bells.
> The source code for HARMONIC.IDX does not seem to be distributed in
> the package. It says "This file is automatically generated by
> Tide2idx", so source code should exist.
The version opencpn ships is an ASCII text file.
Here's the contents of /usr/share/doc/opencpn-data/README.harmonics:
===========================
The opencpn-data package currently ships the ASCII version of xtide's
tidal harmonics tables, while the xtide-data package ships a binary-
blob version.
The tide and current harmonic data contained herein are derived from the
corresponding XTIDE harmonic data. The file data have been tweaked to
work nicely in the US, especially fixing current direction reporting. We
have also added additional global tide stations, and corrected calculation/
display units where sensible. A generic XTIDE harmonic file set will be
functional, but less useful than the harmonic data packaged with OpenCPN.
On 21 Apr 2011, Dave (bdbcat) wrote:
"""
Comment on xtide data:
The harmonic data currently shipping in sourceforge git have been
tweaked (by me and others) to work nicely in the US, especially
fixing current direction reporting. We are also soon adding
Scandinavian tide staion support. Using a generic xtide harmonic
file will be less useful for many users. I think we need to continue
to package our own harmonic files.
...
Related point: I tested some code to incorporate the newer binary
xtide data sets. This could be made to work, but I have no immediate
motivation to pursue.
"""
===========================
> data/wvsdata/ is claimed to be open source, but is a subset. I'd
> suggest that it be packaged separately since it is from wxTide. It is
> also in a binary format so I wonder how it is supposed to be edited or
> what the source code really is.
wvsdata (world vector shoreline) has now been dropped from the opencpn .deb
in favour of recommending the version supplied by the existing
xtide-coastline package. afaik the data format is published publicly.
Convincing OpenCPN, Xtide, ZyGrib, and GMT, ..? to all consolidate on the
same world vector coastline data remains a TODO (I'd humbly suggest
focusing on GMT's version(s) of GSHHS as the standard), but remains beyond
the scope of this [OpenCPN] packaging effort.
> debian/copyright needs to document all the licenses and copyright
> information. You may also want to consider DEP5 (Machine-readable
> debian/copyright):
>
> http://dep.debian.net/deps/dep5/
see latest version of debian/copyright, and also have a look in
src/about.cpp. Probably more could be done here, although it is
unclear to me exactly what is needed where.
> Unfortunately source.debian.net is down otherwise I'd try to find
> other packages containing some of the above files.
>
> opencp.cpp is empty, weird.
no longer exists?
> Normally, .mo files wouldn't be distributed in the source package.
now only .po are shipped.
> lintian complaints not mentioned:
>
> I: opencpn: arch-dep-package-has-big-usr-share 16884kB 88%
> I: opencpn: desktop-entry-contains-encoding-key
> /usr/share/applications/opencpn.desktop:4 Encoding
> I: opencpn: possible-documentation-but-no-doc-base-registration
the package is now lintian clean on squeeze.
> dpkg-shlibdeps warnings. Some of these are possibly issues in
> wxWidgets so you may not be able to do anything about them short of
> filing a bug.
>
> dpkg-shlibdeps: warning: dependency on libwx_gtk2u_richtext-2.8.so.0
> could be avoided if "debian/opencpn/usr/bin/opencpn" were not
> uselessly linked against it (they use none of its symbols).
> dpkg-shlibdeps: warning: dependency on libwx_gtk2u_html-2.8.so.0 could
> be avoided if "debian/opencpn/usr/bin/opencpn" were not uselessly
> linked against it (they use none of its symbols).
> dpkg-shlibdeps: warning: dependency on libwx_gtk2u_aui-2.8.so.0 could
> be avoided if "debian/opencpn/usr/bin/opencpn" were not uselessly
> linked against it (they use none of its symbols).
> dpkg-shlibdeps: warning: dependency on libwx_gtk2u_xrc-2.8.so.0 could
> be avoided if "debian/opencpn/usr/bin/opencpn" were not uselessly
> linked against it (they use none of its symbols).
> dpkg-shlibdeps: warning: dependency on libwx_gtk2u_qa-2.8.so.0 could
> be avoided if "debian/opencpn/usr/bin/opencpn" were not uselessly
> linked against it (they use none of its symbols).
> dpkg-shlibdeps: warning: dependency on libGL.so.1 could be avoided if
> "debian/opencpn/usr/bin/opencpn" were not uselessly linked against it
> (they use none of its symbols).
I have not looked at those, nor know how they could be addressed.
> Some gcc warnings you might want to investigate and forward upstream,
> patches preferred of course :)
>
> In file included from src/chart1.cpp:266:
> ././include/grib.h:83: warning: 'typedef' was ignored in this declaration
> src/cutil.c:106: warning: 'cvsid_aw' defined but not used
> src/georef.c:59: warning: 'cvsid_aw' defined but not used
> src/cm93.cpp: In member function 'bool
> cm93compchart::RenderNextSmallerCellOutlines(wxDC*, ViewPort&, bool)':
> src/cm93.cpp:4581: warning: suggest parentheses around '&&' within '||'
> src/s57chart.cpp: In member function 'void
> s57chart::CreateSENCRecord(OGRFeature*, FILE*, int, S57Reader*)':
> src/s57chart.cpp:4746: warning: format '%d' expects type 'int', but
> argument 3 has type 'size_t'
> src/tri.c:125: warning: 'cvsid_aw' defined but not used
> src/myiso8211/ddfrecord.cpp: In member function 'int DDFRecord::Write()':
> src/myiso8211/ddfrecord.cpp:285: warning: format '%05d' expects type
> 'int', but argument 3 has type 'size_t'
> src/myiso8211/ddfrecord.cpp:289: warning: format '%05d' expects type
> 'int', but argument 3 has type 'size_t'
I have not attempted to review or address compiler warnings, but note that
the code has changed significantly since the above, so best to look at a
fresh build.
> Upstream is using CVS, you might want to consider suggesting that they
> switch to a more modern VCS like git, bzr or SVN.
A matter of personal preference IMO, but upstream has now switched to git.
> In addition, I found some spam on the upstream website:
>
> http://opencpn.org/node/67
> http://opencpn.org/node/73
> http://opencpn.org/node/72
> http://opencpn.org/node/69
> http://opencpn.org/node/65
> http://opencpn.org/node/68
>
> Some former spam pages still show up in menus:
>
> http://opencpn.org/node/70
> http://opencpn.org/node/71
> http://opencpn.org/node/66
long gone.
> You have enough work above, but have you also considered packaging
> wxTide, CapCode and other FLOSS nautical apps?
Xtide is already packaged (AFAIK wxTide is a Windows port of it).
CapCode is alive! http://sourceforge.net/apps/trac/capcode/timeline
regards,
Hamish
Reply to: