* RPM name is OK
* Source curl-7.16.1.tar.bz2 is the same as upstream
* This is the latest version
* Builds fine in mock
* File list of curl-devel looks OK
* File list of curl looks OK

Needs work:
* Missing SMP flags. If it doesn't build with it, please add a comment
  (wiki: PackagingGuidelines#parallelmake)
* Spec file: some paths are not replaced with RPM macros
  (wiki: QAChecklist item 7)
* The %makeinstall macro should not be used
  (wiki: PackagingGuidelines#MakeInstall)
* rpmlint of curl-devel: rpmlint not clean: W: curl-devel summary-ended-with-dot Files needed for 
building applications with libcurl.
* rpmlint of curl: rpmlint not clean: W: curl one-line-command-in-%post /sbin/ldconfig.

Minor:
* Duplicate BuildRequires: pkgconfig (by libidn-devel)

Notes:
* Requires: openssl is not needed (Wiki: Extras/FullExceptionList)
* in %package devel: Requires should probably be BuildRequires
* Please use $RPM_OPT_FLAGS

> Needs work:
> * Missing SMP flags. If it doesn't build with it, please add a comment
>   (wiki: PackagingGuidelines#parallelmake)

Fixed.

> * Spec file: some paths are not replaced with RPM macros
>   (wiki: QAChecklist item 7)

Fixed.

> * The %makeinstall macro should not be used
>   (wiki: PackagingGuidelines#MakeInstall)

cURL won't build without the %makeinstall

> * rpmlint of curl-devel: rpmlint not clean: W: curl-devel
summary-ended-with-dot Files needed for 
> building applications with libcurl.
> * rpmlint of curl: rpmlint not clean: W: curl one-line-command-in-%post
/sbin/ldconfig.

Fixed.

> Minor:
> * Duplicate BuildRequires: pkgconfig (by libidn-devel)

I don't think this is something we want to fix, as we are going to have troubles
if the libidn maintainer drops the pkgconfig dependency.

> Notes:
> * Requires: openssl is not needed (Wiki: Extras/FullExceptionList)

Fixed.

> * in %package devel: Requires should probably be BuildRequires

Nope, there should be AFAIK only Requires for the subpackages.

> * Please use $RPM_OPT_FLAGS

Fixed.

Hi Jindrich,

Sorry about the late response, I missed your reply.

Two questions:
- Could you preserve timestamps when installing files with install -p?
- Is the static library necessary for some other package, and if not, could you disable it?

Thanks,

Ruben

Hi Ruben,

(In reply to comment #3)
> Two questions:
> - Could you preserve timestamps when installing files with install -p?

what install -p do you have in mind? I don't see any in the spec file. Do you
mean those in Makefiles?

> - Is the static library necessary for some other package, and if not, could
you disable it?

Not sure whether it is needed or not, but definitely we should get rid of it.
The question is whether to move it to a -static subpackage or to remove it
completely. What's your optionon on it?

Hi Jindrich,

> what install -p do you have in mind? I don't see any in the spec file.
> Do you mean those in Makefiles?

Yes, in the Makefile.

> Not sure whether it is needed or not, but definitely we should get rid of it.
> The question is whether to move it to a -static subpackage or to remove it
> completely. What's your optionon on it?

I would say remove it completely, you can probably disable static libs in the
configure stage.
There's more on static libs in the PackagingGuidelines.

(In reply to comment #2)
> > * The %makeinstall macro should not be used
> >   (wiki: PackagingGuidelines#MakeInstall)
> 
> cURL won't build without the %makeinstall

I've been rolling my own curl packages for older distros and using "make
DESTDIR=..." hasn't shown up any problems that I've noticed for a long time -
what doesn't build unless you use %makeinstall? If anything, I'd think it'd be a
problem introduced by one of the patches.

> > Minor:
> > * Duplicate BuildRequires: pkgconfig (by libidn-devel)
> 
> I don't think this is something we want to fix, as we are going to have troubles
> if the libidn maintainer drops the pkgconfig dependency.

Not only that, but the curl spec directly invokes pkg-config and not just
implicitly when checking how to build with other libraries. So that's another
good reason to keep the buildreq.

> > Notes:
> > * Requires: openssl is not needed (Wiki: Extras/FullExceptionList)
> 
> Fixed.

The exception list is about BuildRequires, not Requires. The Fedora curl package
uses the certificate revocation list from the openssl package rather than
shipping the version bundled with curl, so I would argue that the runtime
dependency on openssl should stay, even though it will be redundant due to
autogenerated dynamic library dependencies.

(In reply to comment #4)
> Hi Ruben,
> 
> (In reply to comment #3)
> > Two questions:
> > - Could you preserve timestamps when installing files with install -p?
> 
> what install -p do you have in mind? I don't see any in the spec file. Do you
> mean those in Makefiles?

I think the suggestion is to use something like:
make DESTDIR=%{buildroot} INSTALL="%{__install} -p" install
so that all instances of "install" called in the Makefiles get the "-p"
parameter to preserve the timestamps.

> > - Is the static library necessary for some other package, and if not, could
> you disable it?
> 
> Not sure whether it is needed or not, but definitely we should get rid of it.
> The question is whether to move it to a -static subpackage or to remove it
> completely. What's your optionon on it?

I'd say to remove it altogether.

A few other questions:
Does curl-7.14.1-nousr.patch do anything useful?
What's curl-7.15.0-curl_config-version.patch for?

Without these patches, running autotools during package build wouldn't be necessary

Perhaps %ldap_version could be figured out at build time rather than being
hard-coded?

%define ldap_version %(readlink %{_libdir}/libldap.so | sed
's,.*libldap-\\([0-9.]*\\)\\.so\\..*,\\1,')

Thanks for suggestions, most of them are now applied in the rawhide cURL.

None of the remaining patches touch autotools input files, so running aclocal
etc. is no longer needed.

The "make install" command can be simplified down to this:
make DESTDIR=%{buildroot} INSTALL="%{__install} -p" install
That might also get rid of the rpmlint warning from the SRPM about mixed use of
spaces and tabs.

The configure option for the CA bundle needs to be specified without reference
to %{buildroot}, because the packaged /usr/bin/curl-config has a reference to
the buildroot in it as things stand.

Please standardize on either %{buildroot} or $RPM_BUILD_ROOT; either is fine but
don't mix both in the same spec.

The packaged libcurl.pc has a Libs.private: that references -L/usr/lib64 (in the
64-bit version); I'm not sure if that's a problem but I prefer to get rid of
refences to standard library directories by adding a quick sed after running the
configure script:

# Remove -L options for standard library directories
sed -i -e 's,-L/usr/lib ,,g;s,-L/usr/lib64
,,g;s,-L/usr/lib$,,g;s,-L/usr/lib64$,,g' \
        Makefile libcurl.pc

Perhaps docs/CONTRIBUTE could be added to the devel package?

P.S. It's "Paul Howarth", not "Paul Horwath"...

Applied, please check rawhide curl-7.16.3-1.

Thanks!

ping

Still here, just very busy with $DAYJOB at the moment. I'll do a formal review
as soon as I can.

A couple of comments on the latest changes for building with nss rather than
openssl:

1. The package is still configured to use /etc/pki/tls/certs/ca-bundle.crt,
which is provided by openssl and hence that's why there was a dependency on
openssl. Is there an equivalent cert bundle provided by nss?

2. The curl-devel requirement for libidn-devel is real (or at least it was): see
http://www.redhat.com/archives/fedora-list/2004-November/msg07551.html (the
pkgconfig file for curl does reference -lidn too, which would suggest the
dependency is still valid)

Maybe a "Provides: webclient" should be added, wget does this.

curl provides webclient for a while already.

A few more quickies:

1. curl no longer dlopens the LDAP libraries; instead it links to them
conventionally. So the ldap_version macro definition and
--with-ldap-lib/--with-lber-lib configure options are no longer needed. A
buildreq of openldap-devel needs adding though.

2. How about enabling LDAPS support (--enable-ldaps)?

3. A buildreq of krb5-devel needs to be added to re-enable GSSAPI support; this
used to get pulled in by openssl-devel but nss-devel doesn't do it.

4. The conditional "if pkg-config nss" seems redundant since the nss-devel
buildreq will ensure that it's always true. I'd simplify things by removing the
if statement entirely and replace it with:

export CPPFLAGS="$(pkg-config --cflags nss) -DHAVE_PK11_CREATEGENERICOBJECT"

This way, CFLAGS doesn't need to be specified in the subsequent "make" command
either.

Applied. Thanks Paul!

curl-7.17.1-2.fc8 has been pushed to the Fedora 8 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 curl'

Regarding the patch changes for 7.18.0:

 * the curl-config patch isn't needed anymore because the equivalent
functionality is included in the @SSL_ENABLED@ autoconf substitution

 * I think the multilib patch needs to remove the --static-libs option entirely,
since (a) we don't ship the static libs, and (b) there's a remaining reference
to @libdir@ there, which causes a multilib conflict

curl-7.17.1-2.fc8 has been pushed to the Fedora 8 stable repository.  If problems still persist, please make note of it in this bug report.

Paul, I applied changes you propose in rawhide, thanks for noticing.

Approving the review by setting the fedora-review flag.

Created attachment 304760 [details]
Patch against devel cvs for minor spec issues

Last few nitpicks:

1. _GNU_SOURCE is no longer needed for NI_MAXHOST visibility.

2. Buildreq libtool isn't needed.

3. The CHANGES and README files aren't UTF-8.

4. The x86_64 package has a bogus rpath of /usr/lib64.

Attached patch fixes all of these.

Applied, thanks!

curl-7.18.2-1.fc9 has been submitted as an update for Fedora 9

curl-7.18.2-1.fc8 has been submitted as an update for Fedora 8

curl-7.18.2-1.fc8 has been pushed to the Fedora 8 stable repository.  If problems still persist, please make note of it in this bug report.

curl-7.18.2-1.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.