Bug 204140 - Review Request: libmtp - MTP client library
Review Request: libmtp - MTP client library
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-08-25 16:30 EDT by Linus Walleij
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-10-03 06:11:09 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Linus Walleij 2006-08-25 16:30:06 EDT
Spec URL: http://www.df.lth.se/~triad/krad/fc/libmtp.spec
SRPM URL: http://www.df.lth.se/~triad/krad/fc/libmtp-0.0.13-1.src.rpm
Description:
libmtp is a client library to access devices using the MTP
(Media Transfer Protocol) for file and metadata transfer to
different digital media players. This package is currently used
by gnomad2 and amarok, both in FE and users are mailing me bug 
reports..

If you have questions for upstream then post them here because
as with libnjb I *am* upstream. :-)
Comment 1 Linus Walleij 2006-08-27 17:59:58 EDT
New upstream version:
Spec URL: http://www.df.lth.se/~triad/krad/fc/libmtp.spec
SRPM URL: http://www.df.lth.se/~triad/krad/fc/libmtp-0.0.15-1.src.rpm
Comment 2 Parag AN(पराग) 2006-09-14 00:44:54 EDT
{Not Official Reviewer}
packaging looks ok.
+ Mockbuild is successfull for i386 FC6 
+ rpmlint on binary rpm is silent
- rpmlint in SRPM is NOT silent
rpmlint -iv ../SRPMS/libmtp-0.0.15-1.src.rpm 
I: libmtp checking
W: libmtp mixed-use-of-spaces-and-tabs
The specfile mixes use of spaces and tabs for indentation, which is a
cosmetic annoyance.  Use either spaces or tabs for indentation, not both.

+ dist tag is present
+ Buildroot is correct
+ source URL is correct
+ BR is correct
+ License used is LGPL
+ License file COPYING is included
+ devel package is handled correctly
+ MD5 sum on tarball is matching upstream tarball
5d22b16cb7e6a117cdf489669821df09  libmtp-0.0.15.tar.gz
+ No duplicate files


Comment 3 Kevin Fenzi 2006-09-25 23:38:22 EDT
OK - Package name
OK - Spec file matches base package name.
OK - Meets Packaging Guidelines.
OK - License (LGPL)
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
5d22b16cb7e6a117cdf489669821df09  libmtp-0.0.15.tar.gz
5d22b16cb7e6a117cdf489669821df09  libmtp-0.0.15.tar.gz.1
OK - Package compiles and builds on at least one arch.
OK - BuildRequires correct
OK - Spec has needed ldconfig in post and postun
OK - Package owns all the directories it creates.
OK - Package has no duplicate files in %files.
See below - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Spec has consistant macro usage.
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
OK - Headers/static libs in -devel subpackage.
OK - .pc files in -devel subpackage.
OK - .so files in -devel subpackage.
OK - -devel package Requires: %{name} = %{version}-%{release}
OK - .la files are removed.
OK - Package doesn't own any directories other packages own.
See below - No rpmlint output.

SHOULD Items:

OK - Should include License or ask upstream to include it.
OK - Should build in mock.
OK - Should have subpackages require base package with fully versioned depend.

Issues:

1. 0.0.19 is now available.

2. Might change the defattr from:
%defattr(-, root, root)
to
%defattr(-, root, root,-)

3. rpmlint says:

W: libmtp no-documentation
W: libmtp-examples no-documentation

Can be ignored.

W: libmtp mixed-use-of-spaces-and-tabs

Should clean up spec to only use tabs or spaces, not both.
Comment 4 Ralf Corsepius 2006-09-26 01:09:27 EDT
MUST FIX:
* *-devel ships a *.pc
=> Missing "Requires: pkgconfig" in *-devel

BUG:
* The *.pc being shipped should "Require: libusb"
instead of hard-coding -lusb in Libs.

SHOULD:
* Use make DESTDIR=... install instead of %makeinstall
Comment 5 Rex Dieter 2006-09-26 10:00:06 EDT
> * The *.pc being shipped should "Require: libusb"
> instead of hard-coding -lusb in Libs.

It is likely that -lusb is only required for static linking, and if that is 
the case (it appears so to me), then a better fix would be to remove -lusb 
from Libs: and add:
Libs.private: -lusb
that way not all libmtp-dependant apps (using shared libs) will get needlessly 
linked against libusb.
Comment 6 Linus Walleij 2006-09-26 13:20:52 EDT
Fixed all the review problems, by changing upstream and by fixing the spec:
Spec URL: http://www.df.lth.se/~triad/krad/fc/libmtp.spec
SRPM URL: http://www.df.lth.se/~triad/krad/fc/libmtp-0.0.20-1.src.rpm

libusb is supposed to be dynamically linked so I have added Requires: libusb
to the .pc file and removed -lusb
Comment 7 Rex Dieter 2006-09-26 13:32:41 EDT
> libusb is supposed to be dynamically linked 

I'd argue that
Libs.private: -lusb
should still be used, and both changes should be pushed upstream.  This latter 
change isn't relevant here, since the static libs aren't being included.
Comment 8 Linus Walleij 2006-09-26 16:36:56 EDT
OK I've changed it in upstream, seeing the point.

However it's not in Fedora Extras guidelines right now so shouldn't
be holding up the review I hope.
Comment 9 Rex Dieter 2006-09-26 16:42:38 EDT
> OK I've changed it in upstream, seeing the point.

Fabulous.

> shouldn't be holding up the review I hope

nope.  Just make Kevin happy. (:
Comment 10 Kevin Fenzi 2006-09-27 01:03:09 EDT
All the issues above seem to have been solved. 

This package is APPROVED. 
Don't forget to close this bug with NEXTRELEASE once you have imported and 
built it. 
Comment 11 Kevin Fenzi 2006-10-02 20:42:44 EDT
ok, this appears to be in owners.list and imported and built. 
Is anything keeping this review request from being closed now? 
Comment 12 Linus Walleij 2006-10-03 06:11:09 EDT
Yes, sorry for forgetting about this. Imported and build fine.

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