Bug 497947

Hi Ant, long time no see!

I'll review your package tomorrow.

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.

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.

Ant, have you been sponsored yet?

(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.

(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?

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

(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!

(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}

(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} ?

The .la is a libtool archive, not a static library. Just remove it.
See http://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries

(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

(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 :)

(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

No problem Jussi, the more the merrier :-)

Will have a look at mulk as well.

This package is APPROVED.

New Package CVS Request
=======================
Package Name: libmetalink
Short Description: A Metalink C library
Owners: ant
Branches: F-9 F-10 F-11
InitialCC:

cvs done.

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

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

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

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.

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.

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.