Taking this for review, as I maintain the pyfribidi package that depends on this.

GOOD
====
MUST: rpmlint output fine:
  $ rpmlint fribidi-devel-0.10.7-5.1.i386.rpm 
  W: fribidi-devel no-documentation
MUST: spec filename matches %{name}
MUST: package is free software
MUST: License matches actual license (LGPL)
MUST: text of license in both upstream and package as %doc
MUST: spec file legible
MUST: no ExcludeArch
MUST: BuildRequires assumed fine
MUST: no localization
MUST: ldconfig used fine
MUST: not relocatable
MUST: owned dirs fine (-devel creates /usr/lib and depends on pkgconfig which
      owns it)
MUST: no dup files
MUST: clean section fine
MUST: macro use consistent
MUST: contains code
MUST: no large documentation
MUST: %docs don't affect runtime
MUST: header files and static libs in -devel
MUST: -devel which has *.pc Req's pkgconfig\
MUST: *.la explicitly removed
MUST: not GUI
MUST: does not own other's dirs
SHOULD: no scriptlets
SHOULD: no subpackages other than -devel
SHOULD: *.pc files in -devel

BAD
===
MUST: Package Naming Guidelines

  * release should use integers and dist-tags. should be changed to 6%{?dist}

MUST: Packaging Guidelines

  * BuildRoot should be:
    %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

  * "make" should be changed to "make %{?_smp_mflags}"
  
  * "%makeinstall" should be changed to "make DESTDIR=$RPM_BUILD_ROOT install"

MUST: US English

  * FriBidi should be spelled with capital F and B in summary and description
  field of -devel subpackage.
  
  * Hebrew and Arabic should be spelled with capital H and A in descriptionp
  of main package.
  
  * "eg." should be changed to "e.g." or better, "for example": "for example
  Arabic and Hebrew").

  * Static library (*.a) should not be packaged unless there is a very good
  reason for packaging it.

MUST: source to match upstream

  * source matches upstream (md5sum checked) but Source: should be change
  to include the full URL: http://fribidi.org/download/fribidi-[...]

MUST: file permissions

  * Please use %defattr(-,root,root,-) instead of %defattr(-,root,root)
    (it's used twice).

MUST: devel packages must require fully versioned dependency

  * -devel should do this instead of just %{name} = %{version}:
     Requires: %{name} = %{version}-%{release}

a) actually I think people are gone a little mad on using the dist tag:
http://fedoraproject.org/wiki/Packaging/NamingGuidelines says "If you wish to
use a single spec file to build for multiple distributions, you can use the
%{?dist} tag in the Release field", implies that it's not a requirement, but
allowed if you want to use it.

Anyway, doesn't bother me either way, so added along with the rest of the review
points (I think I got them all) as fribidi-0_10_7-6_fc7

(
 Just a note:
 * As written in http://fedoraproject.org/wiki/Packaging/DistTag ,
   using ?dist tag is "preferred", _not_ mandatory.
   There are some cases in that ?dist tag is of no means.
 * As written in http://fedoraproject.org/wiki/Packaging/Guidelines ,
   setting buildroot as 
   %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
   is "preferred", and also this is _not_ mandatory.
   Making to use %__id_u mandatory is still under discussion,
   some reviewers and sponsors strongly disagree this.
)

(In reply to comment #4)

>  * As written in http://fedoraproject.org/wiki/Packaging/Guidelines ,
>    setting buildroot as 
>    %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
>    is "preferred", and also this is _not_ mandatory.
>    Making to use %__id_u mandatory is still under discussion,
>    some reviewers and sponsors strongly disagree this.
This has changed: last week, the FPC has decided to make it mandatory.

(In reply to comment #5)
> (In reply to comment #4)
> 
> >  * As written in http://fedoraproject.org/wiki/Packaging/Guidelines ,
> >    setting buildroot as 
> >    %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
> >    is "preferred", and also this is _not_ mandatory.
> >    Making to use %__id_u mandatory is still under discussion,
> >    some reviewers and sponsors strongly disagree this.
> This has changed: last week, the FPC has decided to make it mandatory.

I think this bug is not a preferred place to discuss,
however, while I don't disagree (rather I recommend) to use
__id_u, what I fear that the macro for _id_u is
as written in  /usr/lib/rpm/macros,
------------------------------------------------
#
# fixowner, fixgroup, and fixperms are run at the end of hardcoded setup
# These macros are necessary only for legacy compatibility, and have moved
# to per-platform macro configuration (i.e. /usr/lib/rpm/<arch>-<os>/macros)
#
# Note: These are no longer enabled by default.
#%__id_u                %{__id} -u
#%__chown_Rhf           %{__chown} -Rhf
#%__chgrp_Rhf           %{__chgrp} -Rhf
#%_fixowner             [ `%{__id_u}` = '0' ] && %{__chown_Rhf} root
#%_fixgroup             [ `%{__id_u}` = '0' ] && %{__chgrp_Rhf} root
#%_fixperms             %{__chmod} -Rf a+rX,u+w,g-w,o-w
#
--------------------------------------------------
... it seems that this is written for _legacy_ support,
and these macros may be removed in the future.

Note: on rpm 4.4.8, _id_u macro is for redhat _only_.

Hello,

Is this review officially completed? Also, are the maintainers interested in branching it for EL-5? I need it to update EL-5's fbreader.

Thanks!

Package Change Request
======================
Package Name: fribidi
New Branches: EL-5 EL-6
Owners: salimma

See comment above -- needed for newer fbreader releases

CVS done (by process-cvs-requests.py).