Bugzilla will be upgraded to version 5.0 on a still to be determined date in the near future. The original upgrade date has been delayed.
Bug 504469 - Review Request: rubygem-term-ansicolor - Ruby library that colors strings using ANSI escape sequences
Review Request: rubygem-term-ansicolor - Ruby library that colors strings usi...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 504479 507761
  Show dependency treegraph
 
Reported: 2009-06-07 07:44 EDT by Lubomir Rintel
Modified: 2009-07-01 10:20 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-07-01 10:20:12 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Lubomir Rintel 2009-06-07 07:44:23 EDT
SPEC: http://v3.sk/~lkundrak/gdc-ruby-stack/SPECS/rubygem-term-ansicolor.spec
SRPM: http://v3.sk/~lkundrak/gdc-ruby-stack/SRPMS/rubygem-term-ansicolor-1.0.3-1.fc11.src.rpm

Description:

Small Ruby library that colors strings using ANSI escape sequences.
Comment 1 Mamoru TASAKA 2009-06-07 10:42:02 EDT
Some basic comments (please also consider to apply the following
comments to the rest of your rubygem related review requests).

- Use %global instead of %define:
  https://fedoraproject.org/wiki/Packaging/Ruby#Pure_Ruby_packages
  https://fedoraproject.org/wiki/PackagingDrafts/global_preferred_over_define

- Requires: ruby(abi) = 1.8 is missing
  https://fedoraproject.org/wiki/Packaging/Ruby#Ruby_Packaging_Guidelines
  And for consistency I recommend to add 
  BuildRequires: ruby(abi) = 1.8

- As you have already defined %geminstdir, please use it also at
  %files and so on

- %geminstdir/[A-Z]* should be marked as %doc

- Please check if Rakefile or install.rb are needed for binary rpm.

---------------------------------------------------------------------
# Examples are documentation
mv $RPM_BUILD_ROOT%{gemdir}/gems/%{gemname}-%{version}/examples \
        $RPM_BUILD_ROOT%{gemdir}/doc/%{gemname}-%{version}
---------------------------------------------------------------------
- Note that
---------------------------------------------------------------------
$ gem contents term-ansicolor
---------------------------------------------------------------------
  expects that examples/ directory should be under %geminstdir.
  While we allow (don't forbid) to delete some files listed in 
  "$ gem contents <gemname>") if packagers think they are not needed,
  I don't think moving examples/ directory under %gemdir/doc is
  needed.
Comment 2 Lubomir Rintel 2009-06-07 11:00:25 EDT
Thanks for picking this up

(In reply to comment #1)
> Some basic comments (please also consider to apply the following
> comments to the rest of your rubygem related review requests).
> 
> - Use %global instead of %define:
>   https://fedoraproject.org/wiki/Packaging/Ruby#Pure_Ruby_packages
>   https://fedoraproject.org/wiki/PackagingDrafts/global_preferred_over_define
>
> - Requires: ruby(abi) = 1.8 is missing
>   https://fedoraproject.org/wiki/Packaging/Ruby#Ruby_Packaging_Guidelines
>   And for consistency I recommend to add 
>   BuildRequires: ruby(abi) = 1.8
> 
> - As you have already defined %geminstdir, please use it also at
>   %files and so on
> 
> - %geminstdir/[A-Z]* should be marked as %doc

Will fix. These four (and maybe others) will be common for most other rubygem packages I have submitted today. I'm wondering if it would make sense if I copied this to other reviews; or what can I do to prevent duplicate reviewer work.

Also, this was all generated by gem2rpm, we probably should fix the tool as well.

> - Please check if Rakefile or install.rb are needed for binary rpm.

Will do.

> ---------------------------------------------------------------------
> # Examples are documentation
> mv $RPM_BUILD_ROOT%{gemdir}/gems/%{gemname}-%{version}/examples \
>         $RPM_BUILD_ROOT%{gemdir}/doc/%{gemname}-%{version}
> ---------------------------------------------------------------------
> - Note that
> ---------------------------------------------------------------------
> $ gem contents term-ansicolor
> ---------------------------------------------------------------------
>   expects that examples/ directory should be under %geminstdir.
>   While we allow (don't forbid) to delete some files listed in 
>   "$ gem contents <gemname>") if packagers think they are not needed,
>   I don't think moving examples/ directory under %gemdir/doc is
>   needed.  

Will revert. This applies to more packages I submitted today as well.
Comment 3 Mamoru TASAKA 2009-06-07 11:06:14 EDT
(In reply to comment #2)
> Thanks for picking this up
> 
> (In reply to comment #1)
> > Some basic comments (please also consider to apply the following
> > comments to the rest of your rubygem related review requests).
> > 
<snip>
> 
> Will fix. These four (and maybe others) will be common for most other rubygem
> packages I have submitted today. I'm wondering if it would make sense if I
> copied this to other reviews; or what can I do to prevent duplicate reviewer
> work.

I think you can just modify your rest srpm and don't have to
copy my comments.
Comment 4 Lubomir Rintel 2009-06-08 07:41:12 EDT
(In reply to comment #1)
> - Please check if Rakefile or install.rb are needed for binary rpm.

See below.

> ---------------------------------------------------------------------
> $ gem contents term-ansicolor
> ---------------------------------------------------------------------
>   expects that examples/ directory should be under %geminstdir.
>   While we allow (don't forbid) to delete some files listed in 
>   "$ gem contents <gemname>") if packagers think they are not needed,
>   I don't think moving examples/ directory under %gemdir/doc is
>   needed.  

I adjusted all the packages to contain all the files "gem contents" list (reverted the deletes). I feel that it should always be consistent with the actual contents and don't think it's worth patching the lists for any of the files I used to remove.

SPEC: http://v3.sk/~lkundrak/gdc-ruby-stack/SPECS/rubygem-term-ansicolor.spec
SRPM:
http://v3.sk/~lkundrak/gdc-ruby-stack/SRPMS/rubygem-term-ansicolor-1.0.3-2.fc11.src.rpm
Comment 5 Mamoru TASAKA 2009-06-08 11:50:15 EDT
Well, 
- Now build.log complains:
-------------------------------------------------------------
    68  warning: File listed twice: /usr/lib/ruby/gems/1.8/gems/term-ansicolor-1.0.3/CHANGES
    69  warning: File listed twice: /usr/lib/ruby/gems/1.8/gems/term-ansicolor-1.0.3/GPL
    70  warning: File listed twice: /usr/lib/ruby/gems/1.8/gems/term-ansicolor-1.0.3/README.en
    71  warning: File listed twice: /usr/lib/ruby/gems/1.8/gems/term-ansicolor-1.0.3/VERSION
--------------------------------------------------------------
  And as you did so before %geminstdir/examples should be marked as
  %doc.
  So %files should be:
--------------------------------------------------------------
%files
%defattr(-,root,root,-)

%dir %{geminstdir}
%doc %{geminstdir}/[A-Z]*
%doc %{geminstdir}/examples/
%{geminstdir}/*.rb
%{geminstdir}/lib/

%doc %{gemdir}/doc/%{gemname}-%{version}/
%{gemdir}/cache/%{gemname}-%{version}.gem
%{gemdir}/specifications/%{gemname}-%{version}.gemspec
---------------------------------------------------------------
  thoughts?
Comment 6 Lubomir Rintel 2009-06-08 11:57:05 EDT
(In reply to comment #5)
> Well, 
> - Now build.log complains:
> -------------------------------------------------------------
>     68  warning: File listed twice:
> /usr/lib/ruby/gems/1.8/gems/term-ansicolor-1.0.3/CHANGES
>     69  warning: File listed twice:
> /usr/lib/ruby/gems/1.8/gems/term-ansicolor-1.0.3/GPL
>     70  warning: File listed twice:
> /usr/lib/ruby/gems/1.8/gems/term-ansicolor-1.0.3/README.en
>     71  warning: File listed twice:
> /usr/lib/ruby/gems/1.8/gems/term-ansicolor-1.0.3/VERSION
> --------------------------------------------------------------

I am aware of this and believe that it is ok. Makes no real harm and saves me from enumerating the contents of the directory just to avoid duplicate listings.

>   And as you did so before %geminstdir/examples should be marked as
>   %doc.

Will fix (for other packages as well).

>   So %files should be:
> --------------------------------------------------------------
> %files
> %defattr(-,root,root,-)
> 
> %dir %{geminstdir}
> %doc %{geminstdir}/[A-Z]*
> %doc %{geminstdir}/examples/
> %{geminstdir}/*.rb
> %{geminstdir}/lib/
> 
> %doc %{gemdir}/doc/%{gemname}-%{version}/
> %{gemdir}/cache/%{gemname}-%{version}.gem
> %{gemdir}/specifications/%{gemname}-%{version}.gemspec
> ---------------------------------------------------------------
>   thoughts?  

I can do this (and for other packages as well) if you insist on eliminate of "listed twice" warning but I'll prefer not to.
Comment 7 Mamoru TASAKA 2009-06-08 12:41:15 EDT
Well, I suggest to remove duplicate %files entry as
- Actually it is MUST ;)
  https://fedoraproject.org/wiki/Packaging/ReviewGuidelines#cite_ref-12

- And I am not sure if in this case rpmbuild marks these duplicated files
  as %doc or normal files.
  I actually checked the rebuilt binary rpm and it seems that
  %{geminstdir}/[A-Z]* are marked as %doc, however I don't know
  any references.
Comment 8 Mamoru TASAKA 2009-06-16 12:42:16 EDT
ping?
Comment 9 Mamoru TASAKA 2009-06-26 09:21:05 EDT
ping again?
Comment 10 Lubomir Rintel 2009-06-26 09:56:37 EDT
Sorry, Mamoru; I've been quite busy with rest of $daywork these day (which is quite a bad excuse for not responding). I've integrated your suggestions:

SPEC: http://v3.sk/~lkundrak/gdc-ruby-stack/SPECS/rubygem-term-ansicolor.spec
SRPM:
http://v3.sk/~lkundrak/gdc-ruby-stack/SRPMS/rubygem-term-ansicolor-1.0.3-3.fc11.src.rpm
Comment 11 Mamoru TASAKA 2009-06-26 10:48:51 EDT
-----------------------------------------------------------------
  This package (rubygem-term-ansicolor) is APPROVED by mtasaka
-----------------------------------------------------------------
Comment 12 Lubomir Rintel 2009-06-26 11:28:03 EDT
Much thanks! I'm currently in the process of fixing the duplicate file listing in other rubygem packages as well.

New Package CVS Request
=======================
Package Name: rubygem-term-ansicolor
Short Description: Ruby library that colors strings using ANSI escape sequences
Owners: hpejakle lkundrak
Branches: F-10 F-11 EL-5
Comment 13 Jason Tibbitts 2009-06-26 20:43:20 EDT
CVS done.
Comment 14 Mamoru TASAKA 2009-06-27 03:06:59 EDT
Not applied to this package, however some notes:
- If rubygem based rpm package contains %{geminstdir}/test , would
  you try to add %check section and execute some test program like
  below?

  http://cvs.fedoraproject.org/viewvc/rpms/rubygem-hpricot/devel/rubygem-hpricot.spec?view=co
  https://fedoraproject.org/wiki/PackagingDrafts/Gem_expand_stage_change
  (The latter one is a draft and currently I am waiting from the feedback
  from any person. If you have time I will appreciate it if you
  comment on:
  https://www.redhat.com/archives/fedora-packaging/2009-June/msg00069.html
  )
Comment 15 Mamoru TASAKA 2009-07-01 10:20:12 EDT
Closing this one.

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