Bug 225253 - Merge Review: apr
Merge Review: apr
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jon Ciesla
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-29 16:01 EST by Nobody's working on this, feel free to take it
Modified: 2012-04-10 08:23 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-04-10 08:23:01 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
limburgher: fedora‑review+
kevin: fedora‑cvs-


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-29 16:01:42 EST
Fedora Merge Review: apr

http://cvs.fedora.redhat.com/viewcvs/devel/apr/
Comment 1 Jeremy Hinegardner 2007-03-29 22:47:04 EDT
I'll be happy to review this package, please look for a forthcoming full review.
Comment 2 Jeremy Hinegardner 2007-03-30 00:31:49 EDT
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
ISSUE (6) - Spec has consistent macro usage.
ISSUE (4) - Meets Packaging Guidelines.
OK - License
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
814f19528d9cfc79aef188dd752e04d8  rpmbuild/SOURCES/apr-1.2.8.tar.gz
814f19528d9cfc79aef188dd752e04d8  reviews/apr/apr-1.2.8.tar.gz
ISSUE (7) - Source URL should go to downloadable source.

ISSUE (3) - BuildRequires correct
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
ISSUE (1) - Package has correct buildroot
     
OK - Package is code or permissible content.
OK - Doc subpackage needed/used.
OK - Packages %doc files don't affect runtime.

OK - Headers/static libs in -devel subpackage.
OK - Spec has needed ldconfig in post and postun
OK - .pc files in -devel subpackage/requires pkgconfig
OK - .so files in -devel subpackage.
ISSUE (8) .a files in -static subpackage
OK - -devel package Requires: %{name} = %{version}-%{release}
OK - .la files are removed.

OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
ISSUE (9) - rpmlint output.
OK - final provides and requires are sane:
SHOULD Items:

OK - Should build in mock.
OK - Should function as described.
OK - Should have sane scriptlets.
OK  - Should have subpackages require base package with fully versioned depend.
ISSUE (2) - Should have dist tag
OK - Should package latest version
ISSUE (5) - check for outstanding bugs on package. (For core merge reviews)

Issues:

1. Build root should be one of the recommended build roots:

    %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

2. The %{?dist} tag should be used in Release:

3. BuildPrereq should not be used, use BuildRequires instead

4. Conflicts: is used and should not be.  Perhaps change Conflicts: to
   Requires: subversion >= 0.20.1-2
   Requires: subversion-devel >= 0.20.1-2

   http://fedoraproject.org/wiki/PackagingDrafts/Conflicts

5. There are outstanding bugs for apr please address them.

6. In %configure it should not be necessary to set CC and CXX.  If the
   are required to be set, use %{__cc} and %{__cxx} instead of gcc and
   g++

7. Source0: should be the upstream source location. Possibly,
    Source0: http://www.eng.lsu.edu/mirrors/apache/%{name}/%{name}-%{version}.tar.gz
8. .a files should be in a separate %{name}-static package or removed.

9. rpmlint output

W: apr buildprereq-use autoconf, libtool, e2fsprogs-devel, python

    See Issue (3)


Comment 3 Robert Scheck 2007-03-30 00:50:04 EDT
Jeremy, I think the conflict should exist further on, because apr doesn't 
depend on subversion, it only conflicts with older verions of subversion. Or
am I wrong somehow with this thinking?
Comment 4 Jeremy Hinegardner 2007-03-30 00:55:54 EDT
Issue 10 - missed a .la file in apr-devel.  This should be removed
Comment 5 Mamoru TASAKA 2007-03-30 00:59:49 EDT
(In reply to comment #2)

> 4. Conflicts: is used and should not be.  Perhaps change Conflicts: to
>    Requires: subversion >= 0.20.1-2
>    Requires: subversion-devel >= 0.20.1-2

Please make it sure if
* apr _really_ needs subversion
* apr does not need subversion, and cause some problems
  if some older subversion is installed
For this case, it doesn't seem to be the formar case.
Comment 6 Jeremy Hinegardner 2007-03-30 01:10:48 EDT
(In reply to comment #3)
> Jeremy, I think the conflict should exist further on, because apr doesn't 
> depend on subversion, it only conflicts with older verions of subversion. Or
> am I wrong somehow with this thinking?

I don't see any reason for the Conflict to exist further on.  According to the
ChangeLog subversion has been >= 0.20.1-2 since May of 2003.  Which means this
Conflicts rule is older than Fedora itself.  I would say it should just be
completely removed.
Comment 7 Joe Orton 2007-04-05 12:16:30 EDT
Thanks for the review!  Please see apr-1.2.8-6 in CVS/Raw Hide.

> 1. Build root should be one of the recommended build roots:

The buildroot used meets the mandatory requirements.

> 2. The %{?dist} tag should be used in Release:

This is not mandatory.
 
> 3. BuildPrereq should not be used, use BuildRequires instead

Fixed.

> 4. Conflicts: is used and should not be.  Perhaps change Conflicts: to

The Conflicts was correct but really no longer necessary; dropped as suggested.

> 5. There are outstanding bugs for apr please address them.

This is not relevant to the packaging review process.

> 6. In %configure it should not be necessary to set CC and CXX.  If the
>    are required to be set, use %{__cc} and %{__cxx} instead of gcc and
>    g++

This is a left-over from some old build/libtool issue; dropped.

> 7. Source0: should be the upstream source location. Possibly,

Fixed.

> 8. .a files should be in a separate %{name}-static package or removed.

Removed.

> 9. rpmlint output

Fixed.

> 10. missed a .la file in apr-devel.  This should be removed

This is part of the build interface and cannot be removed.


Comment 8 Jeremy Hinegardner 2007-04-08 01:51:25 EDT
(In reply to comment #7)
Thanks for the changes.  Everything looks good to except for (10)

> 
> > 10. missed a .la file in apr-devel.  This should be removed
> 
> This is part of the build interface and cannot be removed.

Accoding to http://fedoraproject.org/wiki/Packaging/ReviewGuidelines 

- MUST: Packages must NOT contain any .la libtool archives, these should be
removed in the spec.

Accordingly I cannot give final approval until this is addressed.
Comment 9 Robert Scheck 2007-04-08 05:57:03 EDT
Jeremy, you're wrong. There ARE cases, where you NEED a .la file (e.g. neon, 
ImageMagick just to name a few, to avoid breakings of e.g. rpm or convert).
Comment 10 Robert Scheck 2007-04-08 05:59:51 EDT
And if Joe says .la is needed, we IMHO really should trust him (he maintains 
neon package and knows how to handle .la files, because rpm depends on neon and 
you can't simply drop the .la file there, too).
Comment 11 Rex Dieter 2007-04-08 20:36:50 EDT
Re: comment #7, 
Joe, could you clarify/justify that with a few more details?  

I can understand keeping the .la files around for static linking, and if that is
the case, they should be packaged in -static along with the static libs.
Comment 12 Rex Dieter 2007-04-09 07:55:58 EDT
I noticed the 
# Trim exported dependencies 
section, but it only touched *.la files and *-config, but not *.pc, this patch
effectively does that for the included pkgconfig file (which can/should be
upstreamed):

--- apr-1.2.8/apr.pc.in.private 2007-04-09 06:50:37.000000000 -0500
+++ apr-1.2.8/apr.pc.in 2007-04-09 06:52:21.000000000 -0500
@@ -8,5 +8,6 @@
 Name: APR
 Description: The Apache Portable Runtime library
 Version: @APR_DOTTED_VERSION@
-Libs: -L${libdir} -l@APR_LIBNAME@ @EXTRA_LIBS@
+Libs: -L${libdir} -l@APR_LIBNAME@
+Libs.private: @EXTRA_LIBS@
 Cflags: ${CPPFLAGS} @EXTRA_CFLAGS@ -I${includedir}
Comment 13 Rex Dieter 2007-04-13 09:48:21 EDT
Re: comment #11
Regardless, if you want to keep static libs/.la files, they should to be in
*-static pkgs:
http://fedoraproject.org/wiki/Packaging/Guidelines#head-82d97fc4a3421310f4e2971180e4165965b65662
http://fedoraproject.org/wiki/PackagingDrafts/StaticLinkage
Comment 14 Jeremy Hinegardner 2008-01-20 12:30:23 EST
I'm unable to continue reviewing this package.  Someone else should probably
take it over.
Comment 15 Bojan Smojver 2008-08-22 22:18:55 EDT
CVS admins, please remove tag apr-1_3_3-2_fc10. It was created by mistake.
Comment 16 Kevin Fenzi 2008-08-23 00:26:09 EDT
We don't remove tags from cvs. Just bump the release and retag, or use 'make force-tag'.
Comment 17 Till Maas 2009-09-16 19:20:16 EDT
- The .la issue is still unsolved
I asked on fedora-packaging about this

- The patches are missing an explanation of their upstream status:
http://fedoraproject.org/wiki/Packaging/Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment

Please remove the NotReady from the status Whiteboard once these issues are addressed.
Comment 18 Bojan Smojver 2009-09-16 19:52:51 EDT
The answer to the .la question is that apr-1-config script has an option --link-libtool, which returns the .la file. If the .la file is not shipped, software that uses this option will fail to build. Ditto for apr-util.

Upstream project defines "source compatibility" here:

http://apr.apache.org/versioning.html#source

If .la file is removed, we'll be breaking that versioning rule set by upstream, because some application will fail to build against such apr/apr-util package.
Comment 19 Till Maas 2009-09-16 20:05:28 EDT
Another unresolved issue, there is a static library included:
https://fedoraproject.org/wiki/Packaging/Guidelines#StaticLibraries

So afaics either it must be removed or fesco needs to approve it.


And yet another issue:
There is still a Conflicts: for the -devel subpakage:
Conflicts: subversion-devel < 0.20.1-2
Comment 20 Till Maas 2009-09-16 20:08:14 EDT
(In reply to comment #19)
> Another unresolved issue, there is a static library included:
> https://fedoraproject.org/wiki/Packaging/Guidelines#StaticLibraries
> 
> So afaics either it must be removed or fesco needs to approve it.

I might be wrong here, but then the spec is kind of obfuscated:

%files contains this:
%{_libdir}/libapr-%{aprver}.*a

But there is also this in %install:
 rm -f $RPM_BUILD_ROOT%{_libdir}/apr.exp \
$RPM_BUILD_ROOT%{_libdir}/libapr-*.a

Therefore I guess the entry in %files can be changed to:
%{_libdir}/libapr-%{aprver}.la
Comment 21 Bojan Smojver 2009-09-16 20:17:04 EDT
(In reply to comment #19)
> Another unresolved issue, there is a static library included:
> https://fedoraproject.org/wiki/Packaging/Guidelines#StaticLibraries
> 
> So afaics either it must be removed or fesco needs to approve it.

There is no static libraries in apr or apr-util, as far as I can see.
Comment 22 Jon Ciesla 2012-04-03 12:38:21 EDT
Fresh review.

Good:

- rpmlint checks return: 

apr-devel.x86_64: E: rpath-in-buildconfig /usr/bin/apr-1-config lines ['46']
This build configuration file contains rpaths which will be introduced into
dependent packages.

Fix.

apr-devel.x86_64: W: no-manual-page-for-binary apr-1-config
Each executable in standard binary directories should have a man page.

Fix if possible.

- package meets naming guidelines
- package meets packaging guidelines
- license ( ASL 2.0 ) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file
- devel package ok
- no .la files  

There is one .la file, in -devel, %{_libdir}/libapr-1.la.

This needs to go either in -static, or -devel needs Provides: apr-static = %{version}-%{release}

- post/postun ldconfig ok
- devel requires base package n-v-r 

Let me know if you want me to make any of these fixes.  Thanks!
Comment 23 Joe Orton 2012-04-10 06:17:36 EDT
(In reply to comment #22)
> Fresh review.
> 
> Good:
> 
> - rpmlint checks return: 
> 
> apr-devel.x86_64: E: rpath-in-buildconfig /usr/bin/apr-1-config lines ['46']
> This build configuration file contains rpaths which will be introduced into
> dependent packages.

This isn't correct, when -rpath passed to libtool when linking a library it does not introduce RPATHs; that is the intended use here.
 
> apr-devel.x86_64: W: no-manual-page-for-binary apr-1-config
> Each executable in standard binary directories should have a man page.
> 
> Fix if possible.

Patches welcome for that one!

> There is one .la file, in -devel, %{_libdir}/libapr-1.la.
> 
> This needs to go either in -static, or -devel needs Provides: apr-static =
> %{version}-%{release}

Why?  There is no static library in this package, per comment 21.
Comment 24 Jon Ciesla 2012-04-10 08:23:01 EDT
(In reply to comment #23)
> (In reply to comment #22)
> > Fresh review.
> > 
> > Good:
> > 
> > - rpmlint checks return: 
> > 
> > apr-devel.x86_64: E: rpath-in-buildconfig /usr/bin/apr-1-config lines ['46']
> > This build configuration file contains rpaths which will be introduced into
> > dependent packages.
> 
> This isn't correct, when -rpath passed to libtool when linking a library it
> does not introduce RPATHs; that is the intended use here.

Ok, reasonable.

> > apr-devel.x86_64: W: no-manual-page-for-binary apr-1-config
> > Each executable in standard binary directories should have a man page.
> > 
> > Fix if possible.
> 
> Patches welcome for that one!

If it doesn't exist, it doesn't exist. :)

> 
> > There is one .la file, in -devel, %{_libdir}/libapr-1.la.
> > 
> > This needs to go either in -static, or -devel needs Provides: apr-static =
> > %{version}-%{release}
> 
> Why?  There is no static library in this package, per comment 21.

Ah, I see.  Thanks, that should be it then.

APPROVED.

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