Bug 468631 - Review Request: libgarmin - C library to parse and use Garmin image files
Review Request: libgarmin - C library to parse and use Garmin image files
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 461849 navit
  Show dependency treegraph
 
Reported: 2008-10-26 19:13 EDT by Fabian Affolter
Modified: 2009-05-28 04:08 EDT (History)
7 users (show)

See Also:
Fixed In Version: 0-0.6.20090212svn.fc11
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-04-27 17:18:36 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
tibbs: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Fabian Affolter 2008-10-26 19:13:54 EDT
Spec URL: http://fab.fedorapeople.org/packages/SRPMS/libgarmin.spec
SRPM URL: http://fab.fedorapeople.org/packages/SRPMS/libgarmin-0-0.1.20081026svn.fc9.src.rpm

URL: http://libgarmin.sourceforge.net/

Description:
Libgarmin is an in C written library for Garmin image format maps. The 
library can be used to parse Garmin image files.

Koji scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=904966

[rpm@laptop024 SRPMS]$ rpmlint -i libgarmin*
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

[rpm@laptop024 i386]$ rpmlint libgarmin*
3 packages and 0 specfiles checked; 0 errors, 0 warnings.
Comment 1 Ralf Corsepius 2008-10-26 22:17:01 EDT
Just a couple of remarks:

(In reply to comment #0)
> [rpm@laptop024 i386]$ rpmlint libgarmin*
> 3 packages and 0 specfiles checked; 0 errors, 0 warnings.
Interesting, rpmlint is worse than I had thought:)

MUSTFIX
- libgarmin.a is missing from *-devel
Without, this *-devel rpm is widely useless.

- package doesn't support shared libraries
=> Must add *-static magic to *-devel

- Wrong License:
Your spec says: License:        GPLv2+
Several files (e.g. libgarmin.h and COPYING) inside of the sources say:
 GPLv2 *only*.
Comment 2 Fabian Affolter 2008-10-28 20:17:20 EDT
Thanks Ralf for the comments

(In reply to comment #1)
> Just a couple of remarks:
> 
> (In reply to comment #0)
> MUSTFIX
> - libgarmin.a is missing from *-devel
> Without, this *-devel rpm is widely useless.

fixed

> - package doesn't support shared libraries
> => Must add *-static magic to *-devel

fixed.  I added '--enable-static=no' to %configure
 
> - Wrong License:
> Your spec says: License:        GPLv2+
> Several files (e.g. libgarmin.h and COPYING) inside of the sources say:
>  GPLv2 *only*.

fixed

SPEC: http://fab.fedorapeople.org/packages/SRPMS/libgarmin.spec
SRPM: http://fab.fedorapeople.org/packages/SRPMS/libgarmin-0-0.2.20081026svn.fc9.src.rpm
Comment 3 Michael Schwendt 2008-11-09 19:38:20 EST
> %files
> %defattr(-,root,root,-)
> %doc AUTHORS COPYING INSTALL README TODO

Absolutely no need to include the same %doc files also in
the -devel pkg.

> %{_bindir}/gar*
> %{_datadir}/%{name}/garmintypes.txt

Don't forget the corresponding directory entry!
Add:  %dir %{_datadir}/%{name}

> %files devel
> %defattr(-,root,root,-)
> %doc AUTHORS COPYING INSTALL README TODO
> %{_datadir}/%{name}/doc/*

Here either replace your line with

   %{_datadir}/%{name}/doc/

to add that directory and its contents recursively, or add:

  %dir %{_datadir}/%{name}/doc

to the -devel pkg files section.

> %{_libdir}/pkgconfig/%{name}.pc

You put a file in there, so "Requires: pkgconfig".
Comment 4 Fabian Affolter 2008-11-10 15:06:28 EST
(In reply to comment #3)
> > %files
> > %defattr(-,root,root,-)
> > %doc AUTHORS COPYING INSTALL README TODO
> 
> Absolutely no need to include the same %doc files also in
> the -devel pkg.

fixed

> > %{_bindir}/gar*
> > %{_datadir}/%{name}/garmintypes.txt
> 
> Don't forget the corresponding directory entry!
> Add:  %dir %{_datadir}/%{name}

fixed

> > %files devel
> > %defattr(-,root,root,-)
> > %doc AUTHORS COPYING INSTALL README TODO
> > %{_datadir}/%{name}/doc/*
> 
> Here either replace your line with
> 
>    %{_datadir}/%{name}/doc/
> 
> to add that directory and its contents recursively, or add:
> 
>   %dir %{_datadir}/%{name}/doc
> 
> to the -devel pkg files section.

fixed

> > %{_libdir}/pkgconfig/%{name}.pc
> 
> You put a file in there, so "Requires: pkgconfig".

fixed

SPEC: http://fab.fedorapeople.org/packages/SRPMS/libgarmin.spec
SRPM:
http://fab.fedorapeople.org/packages/SRPMS/libgarmin-0-0.3.20081026svn.fc9.src.rpm
Comment 5 Fabian Affolter 2008-12-14 15:22:57 EST
* Sun Dec 14 2008 Fabian Affolter <fabian@bernewireless.net> - 0-0.4.20081214svn
- Updated from revision 307 to revision 315

SPEC: http://fab.fedorapeople.org/packages/SRPMS/libgarmin.spec
SRPM:
http://fab.fedorapeople.org/packages/SRPMS/libgarmin-0-0.4.20081214svn.fc9.src.rpm
Comment 6 Balint Cristian 2009-01-03 12:15:28 EST
Folks,

- I think this package violates Garmin (TM) proprietary format and their patents.We should be careful by adding such a package to Fedora, Garmin (TM) never released any specifications of their format, everything is a pure reverse 
engineering. On top of all it contains some patent subjected (not sure which one) which allows very smart routing information store.

- Legal folk, can state some points over this package ?
Comment 7 Peter Lemenkov 2009-01-03 13:03:09 EST
> I think this package violates Garmin (TM) proprietary format and their
patents

You're not the Garmin's legal representative to make such statement.
Comment 8 Fabian Affolter 2009-01-03 13:44:08 EST
Add Legal Blocker.  Let's see what's Fedora Legal says.
Comment 9 Tom "spot" Callaway 2009-01-12 15:46:35 EST
I can find no patents in a preliminary search that this would violate. While the format is clearly not documented publicly, there is no problem with a reverse engineered implementation, especially since this is being done for the explicit reason of providing interoperability with Linux environments.

Lifting FE-Legal.
Comment 11 Tom "spot" Callaway 2009-02-12 16:13:10 EST
Fabian, please keep the spec file name the same as the SRPM name.
Comment 13 Jason Tibbitts 2009-03-11 22:01:18 EDT
Builds fine and rpmlint is silent.

Since this package has only static libraries, it's OK that they're in the devel package but you must provide libgarmin-static (and any packages that need to link against the .a need build dependencies on it).

Most files don't seem to have a license block; those that do are GPLv2 (only) and the author adds a note at the head of the COPYING file which clarifies v2 only for the rest.

The description could use a little cleanup. I would suggest something like:
  Libgarmin is a library used to parse IMG files from Garmin GPS devices.

The COPYING, REAMDE and TODO files are duplicated.  You should choose one package to contain them.


* source files match upstream (manually compared with checkout).
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.
X description could use a bit of grammar work.
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* BuildRequires are proper.
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (rawhide, x86_64).
* package installs properly.
* debuginfo package looks complete.
* rpmlint is silent.
X final provides and requires:

  libgarmin-0-0.5.20090212svn.fc11.x86_64.rpm
   libgarmin = 0-0.5.20090212svn.fc11
   libgarmin(x86-64) = 0-0.5.20090212svn.fc11
  =
   (none)

  libgarmin-devel-0-0.5.20090212svn.fc11.x86_64.rpm
   pkgconfig(libgarmin) = 0.1
   libgarmin-devel = 0-0.5.20090212svn.fc11
   libgarmin-devel(x86-64) = 0-0.5.20090212svn.fc11
X  needs libgarmin-static 
  =
   /usr/bin/pkg-config
   libgarmin = 0-0.5.20090212svn.fc11
   pkgconfig

* no shared libraries are added to the regular linker search paths.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
X duplicates in %files.
* file permissions are appropriate.
* no generically named files
* code, not content.
* documentation is small, so no -doc subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* headers are in the -devel package.
* pkgconfig files are in the -devel package; pkgconfig dependency is present.
X static libraries are in the -devel package, but there is no -static provide.
* no libtool .la files.

The package review process needs reviewers!  If you haven't done any package
reviews recently, please consider doing one.
Comment 14 Fabian Affolter 2009-04-08 05:12:00 EDT
Jason, thanks for your input.  I will try to update this package soon.
Comment 15 Michael Schwendt 2009-04-08 07:58:32 EDT
> Group:          Applications/Productivity

System Environment/Libraries


> %configure --enable-static=no

Wrong or not recognised option it seems. Library is static-only, and as soon as it would build shared libs, build would fail anyway due to installed files not included in %files.


> %doc AUTHORS COPYING INSTALL README TODO

In case this is the standard "INSTALL" file, it is irrelevant to RPM package
users and typically is not included.
Comment 17 Jason Tibbitts 2009-04-16 14:00:45 EDT
Removing the duplicated documentation caused a no-documentation complaint from rpm on the -devel package, but that's not an issue.

Otherwise I think this is fine now.  APPROVED
Comment 18 Fabian Affolter 2009-04-16 18:12:07 EDT
New Package CVS Request
=======================
Package Name: libgarmin
Short Description: C library to parse and use Garmin image files
Owners: fab
Branches: F-9 F-10 F-11
InitialCC:
Comment 19 Kevin Fenzi 2009-04-17 12:23:57 EDT
cvs done.
Comment 20 Fedora Update System 2009-04-17 15:43:02 EDT
libgarmin-0-0.6.20090212svn.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/libgarmin-0-0.6.20090212svn.fc10
Comment 21 Fedora Update System 2009-04-17 15:43:07 EDT
libgarmin-0-0.6.20090212svn.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/libgarmin-0-0.6.20090212svn.fc11
Comment 22 Fedora Update System 2009-04-17 15:43:12 EDT
libgarmin-0-0.6.20090212svn.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/libgarmin-0-0.6.20090212svn.fc9
Comment 23 Fedora Update System 2009-04-21 20:50:37 EDT
libgarmin-0-0.6.20090212svn.fc9 has been pushed to the Fedora 9 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing-newkey update libgarmin'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F9/FEDORA-2009-3770
Comment 24 Fedora Update System 2009-04-21 20:55:08 EDT
libgarmin-0-0.6.20090212svn.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update libgarmin'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-3799
Comment 25 Fedora Update System 2009-04-27 17:18:28 EDT
libgarmin-0-0.6.20090212svn.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 26 Fedora Update System 2009-04-27 17:24:25 EDT
libgarmin-0-0.6.20090212svn.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 27 Fedora Update System 2009-05-28 04:08:11 EDT
libgarmin-0-0.6.20090212svn.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

Note You need to log in before you can comment on or make changes to this bug.