Bug 461849 - Review Request: garmintools - Communication tools for Garmin devices
Review Request: garmintools - Communication tools for Garmin devices
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Peter Lemenkov
Fedora Extras Quality Assurance
:
Depends On: 468631
Blocks:
  Show dependency treegraph
 
Reported: 2008-09-10 17:57 EDT by Fabian Affolter
Modified: 2009-10-15 18:42 EDT (History)
8 users (show)

See Also:
Fixed In Version: 0.10-2.fc10
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-10-13 21:41:32 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
lemenkov: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Fabian Affolter 2008-09-10 17:57:30 EDT
Spec URL: http://fab.fedorapeople.org/packages/SRPMS/garmintools.spec
SRPM URL: http://fab.fedorapeople.org/packages/SRPMS/garmintools-0.09-1.fc9.src.rpm

Description: 
This software provides Linux users with the ability to communicate with the 
Garmin Forerunner 305 via the USB interface. All of the documented Garmin 
protocols as of Rev C (May 19, 2006) for the USB physical link implemented.
This means that if you have a Garmin with a USB connection to a PC, you 
ought to be able to use this software to communicate with it.

Scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=818386

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

I'm not sponsored.  Thanks in advance for the review.
Comment 1 Brian Pepple 2008-09-10 20:11:16 EDT
Couple of quick comments:
1. In the files section, your using '%{_libdir}/*', which is causing your main rpm to pull in the debuginfo.  What you need to do is explicitly list your shared libs, that way you'll get a seperate rpm with the debuginfo.  You should use something like '%{_libdir}/libgarmin*.so.*'.

2. libtool archives should not be included.  You should add the following in the %install section after your 'make install':

find %{buildroot} -name '*.la' -exec rm -f {} ';'

2. Static libs -  currently your building them, and I'm pretty sure that's not necessary.  To prevent this, you can add '--enable-static=no' to your configure line in the %build section.  For more info refer to the static libs section in the packaging guidelines. http://fedoraproject.org/wiki/Packaging/Guidelines#Exclusion_of_Static_Libraries

3. missing -devel subpackage.  the package headers in the %{_includedir} and the *.so should be included  in this sub package.

4. After a quick look the source it looks like this package should have a license tag of 'GPLv2+'

If you need help or have additional questions, don't hesitate to contact me.
Comment 2 Fabian Affolter 2008-09-17 14:19:36 EDT
(In reply to comment #1)

Thanks for your input and comments

> 1. In the files section, your using '%{_libdir}/*', which is causing your main
> rpm to pull in the debuginfo.  What you need to do is explicitly list your
> shared libs, that way you'll get a seperate rpm with the debuginfo.  You should
> use something like '%{_libdir}/libgarmin*.so.*'.

fixed

> 2. libtool archives should not be included.  You should add the following in
> the %install section after your 'make install':
> 
> find %{buildroot} -name '*.la' -exec rm -f {} ';'

fixed
 
> 2. Static libs -  currently your building them, and I'm pretty sure that's not
> necessary.  To prevent this, you can add '--enable-static=no' to your configure
> line in the %build section.  For more info refer to the static libs section in
> the packaging guidelines.
> http://fedoraproject.org/wiki/Packaging/Guidelines#Exclusion_of_Static_Libraries

fixed

> 3. missing -devel subpackage.  the package headers in the %{_includedir} and
> the *.so should be included  in this sub package.
> 
> 4. After a quick look the source it looks like this package should have a
> license tag of 'GPLv2+'

fixed
 
I hope that I fixed all points in the right way because I'm still in the 'learn-how-to-make-packages' phase.  Unfortunately this packages depends on 'libgarmin' that is no available in Fedora.  So I have to pack 'libgarmin' first and then we can go on with this package.
Comment 3 Brian Pepple 2008-10-08 20:01:12 EDT
(In reply to comment #2)
> I hope that I fixed all points in the right way because I'm still in the
> 'learn-how-to-make-packages' phase.  Unfortunately this packages depends on
> 'libgarmin' that is no available in Fedora.  So I have to pack 'libgarmin'
> first and then we can go on with this package.

Sorry about getting back to you late, but I've been swamped the last few weeks.  Anyway, has libgarmin been submitted for a package review?
Comment 4 Fabian Affolter 2008-10-26 19:18:26 EDT
Review request for libgarmin:
https://bugzilla.redhat.com/show_bug.cgi?id=468631
Comment 5 Fabian Affolter 2008-12-01 07:01:22 EST
Unfortunately there is no progress with the review of libgarmin.
Comment 6 Fabian Affolter 2009-05-03 07:14:34 EDT
- libgarmin is now in Fedora.
- New upstream version 0.10

Updated files
Spec URL: http://fab.fedorapeople.org/packages/SRPMS/garmintools.spec
SRPM URL:
http://fab.fedorapeople.org/packages/SRPMS/garmintools-0.10-1.fc10.src.rpm
Comment 7 Peter Lemenkov 2009-08-19 09:48:11 EDT
Brian, I'm sorry for shameless breaking of your review process, but it seems, that you forgot about this ticket :)

REVIEW:

- rpmlint is not silent (I added custom message numbering for the sake of simplicity)

[petro@Sulaco SPECS]$ rpmlint ~/Desktop/garmintools-
1. garmintools.ppc: W: shared-lib-calls-exit /usr/lib/libgarmintools.so.4.2.0 exit@GLIBC_2.0
2. garmintools.ppc: E: postin-without-ldconfig /usr/lib/libgarmintools.so.4.2.0
3. garmintools.ppc: E: library-without-ldconfig-postun /usr/lib/libgarmintools.so.4.2.0
4. garmintools.ppc: W: devel-file-in-non-devel-package /usr/lib/libgarmintools.so
3 packages and 0 specfiles checked; 2 errors, 2 warnings.
[petro@Sulaco SPECS]$

The 1st message is indicated a possible design flaw in garmintools. You should consider asking upstream about it.

The 2nd and the 3rd messaged should be fixed. Please remove "devel" from the %post and %postun sections (looks like the leftover), and be careful - don't forget the %post section which contains "rmmod garmin_gps &>/dev/null || true"

The 4th message should be fixed - you accidentally listed file %{_libdir}/libgarmintools.so twice - in main package and in devel-subpackage. I advise you to change in %files section this line 

%{_libdir}/lib*.so*

to that one:

%{_libdir}/lib*.so.*

+ The package is named according to the Package Naming Guidelines .
+ The spec file name matches the base package %{name}, in the format %{name}.spec .

- The package must meet the Packaging Guidelines, but there is an issue, in the %files section, with owned directories which are already owned by other applications. See my note below.

+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines .
+ The License field in the package spec file matches the actual license.
+ The file, containing the text of the license(s) for the package, is included in %doc.
+ The spec file is written in American English.
+ The spec file for the package is legible.
+ The sources used to build the package matches the upstream source, as provided in the spec URL.

[petro@Sulaco SOURCES]$ sha256sum garmintools-0.10.tar.gz*
ffd50b7f963fa9b8ded3223c4786b07906c887ed900de64581a24ff201444cee  garmintools-0.10.tar.gz
ffd50b7f963fa9b8ded3223c4786b07906c887ed900de64581a24ff201444cee  garmintools-0.10.tar.gz.1
[petro@Sulaco SOURCES]$

+ The package successfully compiles and builds into binary rpms on at least one primary architecture.

http://koji.fedoraproject.org/koji/taskinfo?taskID=1614626

+ All build dependencies are listed in BuildRequires.

- Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun. See my notes, regarding rpmlint messages above.

+ The package owns all directories that it creates (none, actually).

- A Fedora package must not list a file more than once in the spec file's %files listings. See my notes, regarding rpmlint messages above.

+ Permissions on files were set properly.
+ The package has a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
+ The package consistently uses macros.
+ The package contains code, or permissible content.
+ Everything, the package includes as %doc, does not affect the runtime of the application.
+ Header files are in a -devel package.
+ No static libraries.

- If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package. See my notes, regarding rpmlint messages above.

+ The devel subpackage requires the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release}
+ The package does NOT contain any .la libtool archives.

- The package must not own files or directories already owned by other packages. Unfortunately, main package owns /etc/udev/rules.d and /etc/modprobe.d
Please use the correct form (note the asterisk mark at the end of the strings)):

%config(noreplace) %{_sysconfdir}/udev/rules.d/*
%config(noreplace) %{_sysconfdir}/modprobe.d/*

+ At the beginning of %install, the package runs rm -rf %{buildroot} (or $RPM_BUILD_ROOT). 
+ All filenames in rpm package are valid UTF-8. 

Please, fix issues noted above.
Comment 8 Fabian Affolter 2009-09-22 17:24:35 EDT
Thanks Peter for your help with this package.

(In reply to comment #7)
> 
> The 1st message is indicated a possible design flaw in garmintools. You should
> consider asking upstream about it.

The issue was opened on May 2009
http://code.google.com/p/garmintools/issues/detail?id=11
 
> The 2nd and the 3rd messaged should be fixed. Please remove "devel" from the
> %post and %postun sections (looks like the leftover), and be careful - don't
> forget the %post section which contains "rmmod garmin_gps &>/dev/null || true"

fixed

> The 4th message should be fixed - you accidentally listed file
> %{_libdir}/libgarmintools.so twice - in main package and in devel-subpackage. 

fixed

> - Every binary RPM package (or subpackage) which stores shared library files
> (not just symlinks) in any of the dynamic linker's default paths, must call
> ldconfig in %post and %postun. See my notes, regarding rpmlint messages above.

fixed
 
> - A Fedora package must not list a file more than once in the spec file's
> %files listings. See my notes, regarding rpmlint messages above.

fixed

> - The package must not own files or directories already owned by other
> packages. Unfortunately, main package owns /etc/udev/rules.d and
> /etc/modprobe.d
> Please use the correct form (note the asterisk mark at the end of the
> strings)):
> 
> %config(noreplace) %{_sysconfdir}/udev/rules.d/*
> %config(noreplace) %{_sysconfdir}/modprobe.d/*

fixed

rpath was removed.

rpmlint output:
[fab@laptop016 SRPMS]$ rpmlint garmintools-0.10-2.fc11.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

[fab@laptop016 x86_64]$ rpmlint garmintools*
garmintools.x86_64: W: shared-lib-calls-exit /usr/lib64/libgarmintools.so.4.2.0 exit@GLIBC_2.2.5
3 packages and 0 specfiles checked; 0 errors, 1 warnings.

Updated files
Spec URL: http://fab.fedorapeople.org/packages/SRPMS/garmintools.spec
SRPM URL:
http://fab.fedorapeople.org/packages/SRPMS/garmintools-0.10-2.fc11.src.rpm
Comment 9 Peter Lemenkov 2009-09-23 11:05:16 EDT
Ok, I'll review it shortly
Comment 10 Peter Lemenkov 2009-09-23 11:15:40 EDT
I can't find any other issues.
This package is 

APPROVED.
Comment 11 Fabian Affolter 2009-10-03 11:22:22 EDT
Thanks for the review.

New Package CVS Request
=======================
Package Name: garmintools
Short Description: Communication tools for Garmin devices
Owners: fab
Branches: F-10 F-11
InitialCC:
Comment 12 Kevin Fenzi 2009-10-03 17:22:46 EDT
cvs done with F-12 branch added.
Comment 13 Fabian Affolter 2009-10-04 14:01:41 EDT
Thanks Kevin
Comment 14 Fedora Update System 2009-10-04 14:28:07 EDT
garmintools-0.10-2.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/garmintools-0.10-2.fc11
Comment 15 Fedora Update System 2009-10-04 14:28:13 EDT
garmintools-0.10-2.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/garmintools-0.10-2.fc12
Comment 16 Fedora Update System 2009-10-04 14:28:19 EDT
garmintools-0.10-2.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/garmintools-0.10-2.fc10
Comment 17 Fedora Update System 2009-10-06 05:56:55 EDT
garmintools-0.10-2.fc11 has been pushed to the Fedora 11 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 garmintools'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-10240
Comment 18 Fedora Update System 2009-10-06 06:06:34 EDT
garmintools-0.10-2.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 garmintools'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-10278
Comment 19 Fedora Update System 2009-10-13 21:41:21 EDT
garmintools-0.10-2.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 20 Fedora Update System 2009-10-15 18:42:02 EDT
garmintools-0.10-2.fc10 has been pushed to the Fedora 10 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.