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 - Merge Review: fribidi
Summary: Merge Review: fribidi
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Caolan McNamara
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-01-31 18:40 UTC by Nobody's working on this, feel free to take it
Modified: 2010-07-08 01:11 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-05-16 12:40:46 UTC
roozbeh: fedora-review-
kevin: fedora-cvs+


Attachments (Terms of Use)

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).


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