Fedora Merge Review: apr http://cvs.fedora.redhat.com/viewcvs/devel/apr/
I'll be happy to review this package, please look for a forthcoming full review.
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)
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?
Issue 10 - missed a .la file in apr-devel. This should be removed
(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.
(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.
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.
(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.
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).
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).
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.
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}
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
I'm unable to continue reviewing this package. Someone else should probably take it over.
CVS admins, please remove tag apr-1_3_3-2_fc10. It was created by mistake.
We don't remove tags from cvs. Just bump the release and retag, or use 'make force-tag'.
- 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.
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.
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
(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
(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.
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!
(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.
(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.