Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 225742 - Merge Review: expat
Summary: Merge Review: expat
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Joe Orton
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
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:
Clone Of:
Environment:
Last Closed: 2007-03-16 17:37:35 UTC
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.