Bug 483696 - Review Request: libdmapsharing - library for DAAP and DPAP
Review Request: libdmapsharing - library for DAAP and DPAP
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michael Schwendt
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-02-02 21:47 EST by W. Michael Petullo
Modified: 2009-03-10 21:03 EDT (History)
3 users (show)

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


Attachments (Terms of Use)

  None (edit)
Description W. Michael Petullo 2009-02-02 21:47:44 EST
Spec URL: http://www.flyn.org/SRPMS/libdmapsharing.spec
SRPM URL: http://www.flyn.org/SRPMS/libdmapsharing-1.9.0.1-1.fc10.src.rpm
Description:
libdmapsharing implements the DMAP protocols. This includes support for DAAP and DPAP.
Comment 1 W. Michael Petullo 2009-02-07 10:04:30 EST
Spec URL: http://www.flyn.org/SRPMS/libdmapsharing.spec
SRPM URL: http://www.flyn.org/SRPMS/libdmapsharing-1.9.0.1-2.fc10.src.rpm
Description:
libdmapsharing implements the DMAP protocols. This includes support for DAAP
and DPAP.
Comment 2 Michael Schwendt 2009-03-06 14:43:18 EST
* Noticed you're [involved] upstream.


* Hint: Run rpmlint not just on the src.rpm, but also on the built rpms.


> Requires: glib2, libsoup, avahi-glib

Should really be dropped in favour of the automatic SONAME dependencies.
However: You don't add the proper LDFLAGS to link with these libraries.
libdmapsharing-1.9.so.1.0.9 contains undefined symbols!
That's why you don't get rpmbuild's automatic SONAME deps.


> %post
> /sbin/ldconfig
>
> %postun
> /sbin/ldconfig

Prefer 

%post -p /sbin/ldconfig
%postun -p /sbin/ldconfig

to run this command directly instead of via /bin/sh.


* -devel pkg ought to "Requires: pkgconfig", because it stores a file in %_libdir/pkgconfig/ and because libdmapsharing API users will likely evaluate the pkg-config --cflags/--libs values for this library.


* %doc file "INSTALL" is irrelevant to RPM package users


* The installed pkg-config file contains a hardcoded /lib in libdir. This breaks on 64-bit archs. Use @libdir@ in the .pc.in template file.


* The major library version as part of the SONAME is intentional?
(libdmapsharing-1.9.so.1.0.9)


* What "license issue" does the TODO file refer to?
Comment 3 W. Michael Petullo 2009-03-06 20:54:33 EST
New version:

Spec URL: http://www.flyn.org/SRPMS/libdmapsharing.spec
SRPM URL: http://www.flyn.org/SRPMS/libdmapsharing-1.9.0.3-1.fc10.src.rpm

>> Requires: glib2, libsoup, avahi-glib
 
> Should really be dropped in favour of the automatic SONAME dependencies.
> However: You don't add the proper LDFLAGS to link with these libraries.
> libdmapsharing-1.9.so.1.0.9 contains undefined symbols!
> That's why you don't get rpmbuild's automatic SONAME deps.

Fixed.
 
>> %post
>> /sbin/ldconfig
>>
>> %postun
>> /sbin/ldconfig

> Prefer 
> 
> %post -p /sbin/ldconfig
> %postun -p /sbin/ldconfig
> 
> to run this command directly instead of via /bin/sh.

Fixed
 
> * -devel pkg ought to "Requires: pkgconfig", because it stores a file in
> %_libdir/pkgconfig/ and because libdmapsharing API users will likely evaluate
> the pkg-config --cflags/--libs values for this library.

Fixed.
 
> * %doc file "INSTALL" is irrelevant to RPM package users

Fixed.

> * The installed pkg-config file contains a hardcoded /lib in libdir. This
> breaks on 64-bit archs. Use @libdir@ in the .pc.in template file.

Fixed.

> * The major library version as part of the SONAME is intentional?
> (libdmapsharing-1.9.so.1.0.9)

Fixed.

> * What "license issue" does the TODO file refer to?  

This issue is resolved. I have received permission directly from the rhythmbox/DAAP developers for a GPL to LGPL license change for their DAAP code. Previously, I had received second hand information from the old libdmapsharing maintainer. The AUTHORS file now documents this.
Comment 4 Michael Schwendt 2009-03-07 09:47:18 EST
Okay. libdmapsharing-1.9.0.3-1.fc10.src.rpm gets my APPROVAL.

[...]

Except (!) for one problem introduced in the .pc file, which you want to fix upstream and in pkg cvs. Here's the diff:

-Libs: -L${libdir} -ldmapsharing-@LIBDMAPSHARING_MAJORMINOR@
-Cflags: -I${includedir}/libdmapsharing-@LIBDMAPSHARING_MAJORMINOR@
+Libs: -L${libdir} -ldmapsharing
+Cflags: -I${includedir}/libdmapsharing

The Cflags point to a non-existant directory now. The headers are found in /usr/include/libdmapsharing-1.9/libdmapsharing/ instead. As you want API users to include <libdmapsharing/foo.h> instead of <foo.h>, the previous Cflags line was correct.
Comment 5 W. Michael Petullo 2009-03-07 14:10:11 EST
New Package CVS Request
=======================
Package Name: libdmapsharing
Short Description: libdmapsharing implements the DMAP protocols.
Owners: mikep
Branches: F-9 F-10
InitialCC:
Comment 6 Kevin Fenzi 2009-03-09 11:57:06 EDT
cvs done.

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