Bug 225742

Summary:	Merge Review: expat
Product:	[Fedora] Fedora	Reporter:	Nobody's working on this, feel free to take it <nobody>
Component:	Package Review	Assignee:	Joe Orton <jorton>
Status:	CLOSED RAWHIDE	QA Contact:	Fedora Package Reviews List <fedora-package-review>
Severity:	medium	Docs Contact:
Priority:	medium
Version:	rawhide	CC:	jorton, redhat-bugzilla, ruben
Target Milestone:	---	Flags:	ruben: fedora-review+
Target Release:	---
Hardware:	All
OS:	Linux
Whiteboard:
Fixed In Version:	1.95.8-10	Doc Type:	Bug Fix
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2007-03-16 17:37:35 UTC	Type:	---
Regression:	---	Mount Type:	---
Documentation:	---	CRM:
Verified Versions:		Category:	---
oVirt Team:	---	RHEL 7.3 requirements from Atomic Host:
Cloudforms Team:	---	Target Upstream Version:
Embargoed:

Description Nobody's working on this, feel free to take it 2007-01-31 18:34:54 UTC

Fedora Merge Review: expat

http://cvs.fedora.redhat.com/viewcvs/devel/expat/
Initial Owner: jorton

Comment 1 Ruben Kerkhof 2007-02-03 20:03:20 UTC

* RPM name is OK
* Source expat-1.95.8.tar.gz is the same as upstream
* Builds fine in mock
* File list of expat looks OK
* File list of expat-devel looks OK

Needs work:
* BuildRoot should be %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
  (wiki: PackagingGuidelines#BuildRoot)
* The %makeinstall macro should not be used
  (wiki: PackagingGuidelines#MakeInstall)
* rpmlint of expat: W: expat summary-ended-with-dot A library for parsing XML.
* rpmlint of expat-devel: W: expat-devel summary-ended-with-dot Libraries and include files to 
develop XML applications with expat.

Minor:
* Duplicate BuildRequires: autoconf (by automake), automake (by libtool)

Notes:
* Please use {?dist} in Release tag
* You should Require(post) and Require(postun) ldconfig
* Is it necessary to include static binaries (see the wiki: PackageGuidelines/Exclusion of Static Libraries

Comment 2 Joe Orton 2007-02-04 13:28:22 UTC

Thanks for the review.

Fixed in -10:
- Summary trailing dot removed
- switch to preferred BuildRoot

Won't fix:
- %makeinstall is necessary for this non-DESTDIR-aware Makefile
- dist tag use is not mandatory
- explicit BuildRequires is a good thing
- rpm automatically adds correct Requires for scriptlet interpreters
- yes, static libraries are necessary for the static build of rpm

Comment 3 Ruben Kerkhof 2007-02-04 14:05:10 UTC

Hi Joe,

> - %makeinstall is necessary for this non-DESTDIR-aware Makefile

Agreed, from the guidelines:
Fedora's RPM includes a %makeinstall macro but it must NOT be used when make install DESTDIR=%
{buildroot} will work.
So in this case it's not a blocker.

> - dist tag use is not mandatory

That's right

> - explicit BuildRequires is a good thing

True.

> - rpm automatically adds correct Requires for scriptlet interpreters

True as well.

> - yes, static libraries are necessary for the static build of rpm

There's no official policy on the inclusion of static libraries as far as I know, so this is no blocker either.

This package is APPROVED.

Comment 4 Robert Scheck 2007-02-04 14:52:32 UTC

%{?dist} is no must? Since when if true or did I misunderstand something?

Comment 5 Ruben Kerkhof 2007-02-04 15:59:50 UTC

Uh, look trought the list of MUST items in the Review Guidelines... nothing there.

From http://fedoraproject.org/wiki/Packaging/DistTag:

Do I Have To Use the Dist Tag?
No. It is documented and standardized so that maintainers who wish to use it can do so, but it is not 
mandatory.

Comment 6 Ruben Kerkhof 2007-03-18 10:07:33 UTC

Assigning ticket to myself as per http://fedoraproject.org/wiki/PackageReviewProcess