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 225771

Summary: Merge Review: fribidi
Product: [Fedora] Fedora Reporter: Nobody's working on this, feel free to take it <nobody>
Component: Package ReviewAssignee: Caolan McNamara <caolanm>
Status: CLOSED RAWHIDE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: caolanm, michel, roozbeh
Target Milestone: ---Flags: roozbeh: fedora-review-
kevin: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2007-05-16 12:40:46 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:

Description Nobody's working on this, feel free to take it 2007-01-31 18:40:15 UTC
Fedora Merge Review: fribidi

http://cvs.fedora.redhat.com/viewcvs/devel/fribidi/
Initial Owner: caolanm@redhat.com

Comment 1 Roozbeh Pournader 2007-02-03 23:23:18 UTC
Taking this for review, as I maintain the pyfribidi package that depends on this.

Comment 2 Roozbeh Pournader 2007-02-04 00:37:38 UTC
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}


Comment 3 Caolan McNamara 2007-02-05 09:43:51 UTC
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

Comment 4 Mamoru TASAKA 2007-02-05 10:07:58 UTC
(
 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.
)

Comment 5 Ralf Corsepius 2007-02-05 10:10:43 UTC
(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.




Comment 6 Mamoru TASAKA 2007-02-05 10:28:49 UTC
(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.



Comment 7 Mamoru TASAKA 2007-02-05 10:56:37 UTC
Note: on rpm 4.4.8, _id_u macro is for redhat _only_.

Comment 8 Michel Alexandre Salim 2009-10-18 03:43:45 UTC
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!

Comment 9 Michel Alexandre Salim 2010-07-05 19:16:43 UTC
Package Change Request
======================
Package Name: fribidi
New Branches: EL-5 EL-6
Owners: salimma

See comment above -- needed for newer fbreader releases

Comment 10 Kevin Fenzi 2010-07-08 01:11:16 UTC
CVS done (by process-cvs-requests.py).