Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225253
Summary: Merge Review: apr Product: Fedora Extras Version: devel Platform: All OS/Version: Linux Status: NEW Severity: normal Priority: normal Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: nobody@fedoraproject.org QAContact: fedora-package-review@redhat.com CC: jorton@redhat.com
Fedora Merge Review: apr
http://cvs.fedora.redhat.com/viewcvs/devel/apr/
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: apr
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225253
bugzilla@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Severity|normal |medium
jeremy@hinegardner.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-review?
------- Additional Comments From jeremy@hinegardner.org 2007-03-29 22:47 EST ------- I'll be happy to review this package, please look for a forthcoming full review.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: apr
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225253
jeremy@hinegardner.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED AssignedTo|nobody@fedoraproject.org |jeremy@hinegardner.org
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: apr
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225253
------- Additional Comments From jeremy@hinegardner.org 2007-03-30 00:31 EST ------- 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/%%7Bname%7D/%%7Bname%7D-%%7Bversion%7D... 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)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: apr
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225253
------- Additional Comments From redhat-bugzilla@linuxnetz.de 2007-03-30 00:50 EST ------- 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?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: apr
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225253
------- Additional Comments From jeremy@hinegardner.org 2007-03-30 00:55 EST ------- Issue 10 - missed a .la file in apr-devel. This should be removed
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: apr
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225253
------- Additional Comments From mtasaka@ioa.s.u-tokyo.ac.jp 2007-03-30 00:59 EST ------- (In reply to comment #2)
- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: apr
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225253
------- Additional Comments From jeremy@hinegardner.org 2007-03-30 01:10 EST ------- (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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: apr
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225253
jorton@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |MODIFIED
------- Additional Comments From jorton@redhat.com 2007-04-05 12:16 EST ------- Thanks for the review! Please see apr-1.2.8-6 in CVS/Raw Hide.
- Build root should be one of the recommended build roots:
The buildroot used meets the mandatory requirements.
- The %{?dist} tag should be used in Release:
This is not mandatory.
- BuildPrereq should not be used, use BuildRequires instead
Fixed.
- Conflicts: is used and should not be. Perhaps change Conflicts: to
The Conflicts was correct but really no longer necessary; dropped as suggested.
- There are outstanding bugs for apr please address them.
This is not relevant to the packaging review process.
- 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.
- Source0: should be the upstream source location. Possibly,
Fixed.
- .a files should be in a separate %{name}-static package or removed.
Removed.
- rpmlint output
Fixed.
- missed a .la file in apr-devel. This should be removed
This is part of the build interface and cannot be removed.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: apr
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225253
------- Additional Comments From jeremy@hinegardner.org 2007-04-08 01:51 EST ------- (In reply to comment #7) Thanks for the changes. Everything looks good to except for (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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: apr
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225253
------- Additional Comments From redhat-bugzilla@linuxnetz.de 2007-04-08 05:57 EST ------- 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).
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: apr
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225253
------- Additional Comments From redhat-bugzilla@linuxnetz.de 2007-04-08 05:59 EST ------- 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).
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: apr
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225253
------- Additional Comments From rdieter@math.unl.edu 2007-04-08 20:36 EST ------- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: apr
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225253
------- Additional Comments From rdieter@math.unl.edu 2007-04-09 07:55 EST ------- 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}
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: apr
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225253
------- Additional Comments From rdieter@math.unl.edu 2007-04-13 09:48 EST ------- 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-82d97fc4a3421310f4e2... http://fedoraproject.org/wiki/PackagingDrafts/StaticLinkage
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: apr
https://bugzilla.redhat.com/show_bug.cgi?id=225253
bugzilla@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Priority|normal |medium Product|Fedora Extras |Fedora Version|devel |rawhide
jeremy@hinegardner.org changed:
What |Removed |Added ---------------------------------------------------------------------------- AssignedTo|jeremy@hinegardner.org |nobody@fedoraproject.org Status|MODIFIED |ASSIGNED Flag|fedora-review? |
------- Additional Comments From jeremy@hinegardner.org 2008-01-20 12:30 EST ------- I'm unable to continue reviewing this package. Someone else should probably take it over.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=225253
Bojan Smojver bojan@rexursive.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |bojan@rexursive.com
--- Comment #15 from Bojan Smojver bojan@rexursive.com 2008-08-22 22:18:55 EDT --- CVS admins, please remove tag apr-1_3_3-2_fc10. It was created by mistake.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=225253
--- Comment #16 from Kevin Fenzi kevin@tummy.com 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'.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=225253
Jussi Lehtola jussi.lehtola@iki.fi changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |NEW
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=225253
Till Maas opensource@till.name changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |opensource@till.name Status Whiteboard| |NotReady
--- Comment #17 from Till Maas opensource@till.name 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_a...
Please remove the NotReady from the status Whiteboard once these issues are addressed.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=225253
--- Comment #18 from Bojan Smojver bojan@rexursive.com 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=225253
--- Comment #19 from Till Maas opensource@till.name 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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=225253
--- Comment #20 from Till Maas opensource@till.name 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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=225253
--- Comment #21 from Bojan Smojver bojan@rexursive.com 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=225253
Jon Ciesla limburgher@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |limburgher@gmail.com AssignedTo|nobody@fedoraproject.org |limburgher@gmail.com Status Whiteboard|NotReady | Flag| |fedora-review?
--- Comment #22 from Jon Ciesla limburgher@gmail.com 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!
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=225253
--- Comment #23 from Joe Orton jorton@redhat.com 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=225253
Jon Ciesla limburgher@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution| |ERRATA Flag|fedora-review? |fedora-review+ Last Closed| |2012-04-10 08:23:01
--- Comment #24 from Jon Ciesla limburgher@gmail.com 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.
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla