Bug 225771 - Merge Review: fribidi
Merge Review: fribidi
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Caolan McNamara
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 13:40 EST by Nobody's working on this, feel free to take it
Modified: 2010-07-07 21:11 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-05-16 08:40:46 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
roozbeh: fedora‑review-
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 13:40:15 EST
Fedora Merge Review: fribidi

http://cvs.fedora.redhat.com/viewcvs/devel/fribidi/
Initial Owner: caolanm@redhat.com
Comment 1 Roozbeh Pournader 2007-02-03 18:23:18 EST
Taking this for review, as I maintain the pyfribidi package that depends on this.
Comment 2 Roozbeh Pournader 2007-02-03 19:37:38 EST
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 04:43:51 EST
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 05:07:58 EST
(
 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 05:10:43 EST
(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 05:28:49 EST
(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 05:56:37 EST
Note: on rpm 4.4.8, _id_u macro is for redhat _only_.
Comment 8 Michel Alexandre Salim 2009-10-17 23:43:45 EDT
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 15:16:43 EDT
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-07 21:11:16 EDT
CVS done (by process-cvs-requests.py).

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