Bug 497947 - Review Request: libmetalink - A Metalink C library
Summary: Review Request: libmetalink - A Metalink C library
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Ruben Kerkhof
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 497948
TreeView+ depends on / blocked
 
Reported: 2009-04-28 02:54 UTC by Ant Bryan
Modified: 2009-05-19 02:03 UTC (History)
4 users (show)

Fixed In Version: 0.0.3-4.fc9
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-05-14 07:24:59 UTC
Type: ---
Embargoed:
ruben: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Ant Bryan 2009-04-28 02:54:16 UTC
Spec URL: http://pastebin.ca/1405673
SRPM URL: http://www.metalinker.org/mirrors/libmetalink/libmetalink-0.0.3-1.fc10.src.rpm
Description: libmetalink is a Metalink C library. It adds Metalink functionality such as parsing Metalink XML files to programs written in C.

Comment 1 Ruben Kerkhof 2009-04-28 21:37:35 UTC
Hi Ant, long time no see!

I'll review your package tomorrow.

Comment 2 Ant Bryan 2009-04-29 04:39:18 UTC
Hi Ruben! Cool running into you here! :)

Thanks in advance for the review. This is my first library package, & I based it off another so hopefully it's a good start.

I also packaged mulk (bug 497948) which depends on libmetalink here.

Comment 3 Susi Lehtola 2009-04-29 11:43:05 UTC
A few notes:

- .la files need to be removed, add
 find . -name *.la -exec rm {} \;
to the end of the install phase

- if possible, static library build should be disable (usually %configure --disable-static)

- devel package needs to Requires: pkgconfig

- you're running autoreconf, but not BRing the packages => the package will not build in mock. Autoreconf'ing is frowned upon, and should not be done unless it is specifically necessary.

Comment 4 Ruben Kerkhof 2009-04-29 13:37:30 UTC
Ant, have you been sponsored yet?

Comment 5 Ant Bryan 2009-04-29 15:30:34 UTC
(In reply to comment #3)
> A few notes:
> 
> - .la files need to be removed, add
>  find . -name *.la -exec rm {} \;
> to the end of the install phase
> 
> - if possible, static library build should be disable (usually %configure
> --disable-static)
> 
> - devel package needs to Requires: pkgconfig
> 
> - you're running autoreconf, but not BRing the packages => the package will not
> build in mock. Autoreconf'ing is frowned upon, and should not be done unless it
> is specifically necessary.  

Done.

Spec URL: http://pastebin.ca/1407423
SRPM URL: http://www.metalinker.org/mirrors/libmetalink/libmetalink-0.0.3-2.fc10.src.rpm

(In reply to comment #4)
> Ant, have you been sponsored yet?  

Yes.

Comment 6 Ant Bryan 2009-05-06 17:49:10 UTC
(In reply to comment #3)
> A few notes:
> 
> - .la files need to be removed, add
>  find . -name *.la -exec rm {} \;
> to the end of the install phase
> 
> - if possible, static library build should be disable (usually %configure
> --disable-static)
> 
> - devel package needs to Requires: pkgconfig
> 
> - you're running autoreconf, but not BRing the packages => the package will not
> build in mock. Autoreconf'ing is frowned upon, and should not be done unless it
> is specifically necessary.  

I may not have been clear before, but I addressed those 4 issues you found and they are resolved in my second round of rpms.

Spec URL: http://pastebin.ca/1407423
SRPM URL:
http://www.metalinker.org/mirrors/libmetalink/libmetalink-0.0.3-2.fc10.src.rpm

Any other issues I can fix?

Comment 7 Ruben Kerkhof 2009-05-06 18:38:51 UTC
Just a few:

replace /usr/share/doc with %{_docdir} in the %files section.

And:

rpmlint of libmetalink-devel:
libmetalink-devel.x86_64: W: summary-ended-with-dot A Metalink C library devel package.
libmetalink-devel.x86_64: W: no-documentation

And you have to own the directory /usr/include/metalink

Comment 8 Ant Bryan 2009-05-06 19:53:47 UTC
(In reply to comment #7)
> Just a few:
> 
> replace /usr/share/doc with %{_docdir} in the %files section.

Ok.

> And:
> 
> rpmlint of libmetalink-devel:
> libmetalink-devel.x86_64: W: summary-ended-with-dot A Metalink C library devel
> package.

Ok.

> libmetalink-devel.x86_64: W: no-documentation

I don't need to do anything about that, right?
 
> And you have to own the directory /usr/include/metalink  

%files
%dir /usr/include/metalink

Is that all?

Thanks Ruben!

Comment 9 Susi Lehtola 2009-05-06 20:07:09 UTC
(In reply to comment #8)
> (In reply to comment #7)> And you have to own the directory /usr/include/metalink  
> 
> %files
> %dir /usr/include/metalink

A lot neater is to

%files devel
%defattr(-,root,root,-)
%{_includedir}/metalink/

Also, according to your files listing there is still a static library in the devel package; you either have to exclude it from the package or make the devel package provide libmetalink-static = %{version}-%{release}

Comment 10 Ant Bryan 2009-05-06 23:44:39 UTC
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)> And you have to own the directory /usr/include/metalink  
> > 
> > %files
> > %dir /usr/include/metalink
> 
> A lot neater is to
> 
> %files devel
> %defattr(-,root,root,-)
> %{_includedir}/metalink/

Thank you, I'll go with your neater solution :)

> Also, according to your files listing there is still a static library in the
> devel package; you either have to exclude it from the package or make the devel
> package provide libmetalink-static = %{version}-%{release}  

I added this to the devel package:

Provides:      libmetalink-static = %{version}-%{release}

but the same files are included in the devel package.

libmetalink-devel-0.0.3-3.fc10.i386.rpm :
drwxr-xr-x  /usr/include/metalink
-rw-r--r--  /usr/include/metalink/metalink_error.h
-rw-r--r--  /usr/include/metalink/metalink_parser.h
-rw-r--r--  /usr/include/metalink/metalink_types.h
-rwxr-xr-x  /usr/lib/libmetalink.la
lrwxrwxrwx  /usr/lib/libmetalink.so
-rw-r--r--  /usr/lib/pkgconfig/libmetalink.pc

Which is the static library in the devel package?

Which is better, excluding it from the package or making the devel package provide libmetalink-static = %{version}-%{release} ?

Comment 11 Ruben Kerkhof 2009-05-07 03:49:46 UTC
The .la is a libtool archive, not a static library. Just remove it.
See http://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries

Comment 12 Ant Bryan 2009-05-07 05:04:17 UTC
(In reply to comment #11)
> The .la is a libtool archive, not a static library. Just remove it.
> See
> http://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries  

Ah, I had (almost) read that but missed the part about .la files. (I'd skipped down to the bold title).

I have already done this as suggested in comment #3:

- .la files need to be removed, add
 find . -name *.la -exec rm {} \;
to the end of the install phase


I looked at another .spec file & this appears to work:

find $RPM_BUILD_ROOT -name *.la -exec rm {} \;

Now /usr/lib/libmetalink.a is no longer packaged, I take it that's fine?

Spec URL: http://pastebin.ca/1415019
SRPM URL:
http://www.metalinker.org/mirrors/libmetalink/libmetalink-0.0.3-3.fc10.src.rpm

Comment 13 Susi Lehtola 2009-05-07 05:35:40 UTC
(In reply to comment #12)
> Now /usr/lib/libmetalink.a is no longer packaged, I take it that's fine?
> 
> Spec URL: http://pastebin.ca/1415019
> SRPM URL:
> http://www.metalinker.org/mirrors/libmetalink/libmetalink-0.0.3-3.fc10.src.rpm  

As there is no static library present now you must remove the 
 Provides: libmetalink-static = %{version}-%{release}
line :)

And Ruben, sorry for barging in your territory :)

Comment 14 Ant Bryan 2009-05-07 06:30:25 UTC
(In reply to comment #13)
> (In reply to comment #12)
> > Now /usr/lib/libmetalink.a is no longer packaged, I take it that's fine?
> > 
> > Spec URL: http://pastebin.ca/1415019
> > SRPM URL:
> > http://www.metalinker.org/mirrors/libmetalink/libmetalink-0.0.3-3.fc10.src.rpm  
> 
> As there is no static library present now you must remove the 
>  Provides: libmetalink-static = %{version}-%{release}
> line :)

Ah yes, forgot about that :) Thanks for the help, both of you!

Would it be possible to take a peek at my mulk packaging (bug 497948) once this one is finished?

Spec URL: http://pastebin.ca/1415078
SRPM URL:
http://www.metalinker.org/mirrors/libmetalink/libmetalink-0.0.3-4.fc10.src.rpm

Comment 15 Ruben Kerkhof 2009-05-07 14:40:29 UTC
No problem Jussi, the more the merrier :-)

Will have a look at mulk as well.

This package is APPROVED.

Comment 16 Ant Bryan 2009-05-08 21:04:32 UTC
New Package CVS Request
=======================
Package Name: libmetalink
Short Description: A Metalink C library
Owners: ant
Branches: F-9 F-10 F-11
InitialCC:

Comment 17 Kevin Fenzi 2009-05-09 20:56:05 UTC
cvs done.

Comment 18 Fedora Update System 2009-05-14 07:14:34 UTC
libmetalink-0.0.3-4.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/libmetalink-0.0.3-4.fc11

Comment 19 Fedora Update System 2009-05-14 07:16:16 UTC
libmetalink-0.0.3-4.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/libmetalink-0.0.3-4.fc10

Comment 20 Fedora Update System 2009-05-14 07:18:42 UTC
libmetalink-0.0.3-4.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/libmetalink-0.0.3-4.fc9

Comment 21 Fedora Update System 2009-05-19 02:00:46 UTC
libmetalink-0.0.3-4.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 22 Fedora Update System 2009-05-19 02:02:16 UTC
libmetalink-0.0.3-4.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 23 Fedora Update System 2009-05-19 02:03:28 UTC
libmetalink-0.0.3-4.fc9 has been pushed to the Fedora 9 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.