Bug 225742 - Merge Review: expat
Summary: Merge Review: expat
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review   
(Show other bugs)
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Joe Orton
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Keywords:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-01-31 18:34 UTC by Nobody's working on this, feel free to take it
Modified: 2007-11-30 22:11 UTC (History)
3 users (show)

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: ---
ruben: fedora-review+


Attachments (Terms of Use)

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@redhat.com

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


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