Bug 225671 - Merge Review: curl
Merge Review: curl
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jindrich Novy
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 12:54 EST by Nobody's working on this, feel free to take it
Modified: 2013-07-02 19:19 EDT (History)
6 users (show)

See Also:
Fixed In Version: 7.17.1-2.fc8
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-02-15 21:09:56 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
ruben: fedora‑review+


Attachments (Terms of Use)
Patch against devel cvs for minor spec issues (1.56 KB, patch)
2008-05-07 08:48 EDT, Paul Howarth
no flags Details | Diff

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 12:54:25 EST
Fedora Merge Review: curl

http://cvs.fedora.redhat.com/viewcvs/devel/curl/
Initial Owner: jnovy@redhat.com
Comment 1 Ruben Kerkhof 2007-02-03 11:08:26 EST
* 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

Comment 2 Jindrich Novy 2007-02-05 09:14:35 EST
> 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.
Comment 3 Ruben Kerkhof 2007-02-18 15:40:16 EST
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
Comment 4 Jindrich Novy 2007-02-19 06:10:17 EST
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?
Comment 5 Ruben Kerkhof 2007-02-19 10:35:54 EST
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.


Comment 6 Paul Howarth 2007-06-08 08:15:47 EDT
(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,')

Comment 7 Jindrich Novy 2007-06-19 07:37:50 EDT
Thanks for suggestions, most of them are now applied in the rawhide cURL.
Comment 8 Paul Howarth 2007-06-22 07:02:46 EDT
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"...
Comment 9 Jindrich Novy 2007-06-25 07:30:28 EDT
Applied, please check rawhide curl-7.16.3-1.

Thanks!
Comment 10 Jindrich Novy 2007-07-24 04:22:39 EDT
ping
Comment 11 Paul Howarth 2007-07-24 05:14:46 EDT
Still here, just very busy with $DAYJOB at the moment. I'll do a formal review
as soon as I can.
Comment 12 Paul Howarth 2007-08-28 06:42:56 EDT
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)
Comment 13 Till Maas 2007-09-09 08:48:44 EDT
Maybe a "Provides: webclient" should be added, wget does this.
Comment 14 Jindrich Novy 2007-11-22 05:55:36 EST
curl provides webclient for a while already.
Comment 15 Paul Howarth 2007-11-30 05:27:46 EST
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.
Comment 16 Jindrich Novy 2007-11-30 06:25:19 EST
Applied. Thanks Paul!
Comment 17 Fedora Update System 2008-01-24 16:57:11 EST
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'
Comment 18 Paul Howarth 2008-01-30 06:27:52 EST
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
Comment 19 Fedora Update System 2008-02-15 21:09:50 EST
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.
Comment 20 Jindrich Novy 2008-02-16 00:55:06 EST
Paul, I applied changes you propose in rawhide, thanks for noticing.
Comment 21 Ruben Kerkhof 2008-05-07 02:30:10 EDT
Approving the review by setting the fedora-review flag.
Comment 22 Paul Howarth 2008-05-07 08:48:34 EDT
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.
Comment 23 Jindrich Novy 2008-05-08 11:44:08 EDT
Applied, thanks!
Comment 24 Fedora Update System 2008-06-18 03:08:50 EDT
curl-7.18.2-1.fc9 has been submitted as an update for Fedora 9
Comment 25 Fedora Update System 2008-06-19 02:11:50 EDT
curl-7.18.2-1.fc8 has been submitted as an update for Fedora 8
Comment 26 Fedora Update System 2008-06-20 15:11:17 EDT
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.
Comment 27 Fedora Update System 2008-06-20 15:12:33 EDT
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.

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