Bug 504469

Summary: Review Request: rubygem-term-ansicolor - Ruby library that colors strings using ANSI escape sequences
Product: [Fedora] Fedora Reporter: Lubomir Rintel <lkundrak>
Component: Package ReviewAssignee: Mamoru TASAKA <mtasaka>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: fedora-package-review, mtasaka, notting
Target Milestone: ---Flags: mtasaka: fedora-review+
j: 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: 2009-07-01 14:20:12 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 504479, 507761    

Description Lubomir Rintel 2009-06-07 11:44:23 UTC
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 14:42:02 UTC
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 15:00:25 UTC
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 15:06:14 UTC
(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 11:41:12 UTC
(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 15:50:15 UTC
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 15:57:05 UTC
(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 16:41:15 UTC
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 16:42:16 UTC
ping?

Comment 9 Mamoru TASAKA 2009-06-26 13:21:05 UTC
ping again?

Comment 10 Lubomir Rintel 2009-06-26 13:56:37 UTC
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 14:48:51 UTC
-----------------------------------------------------------------
  This package (rubygem-term-ansicolor) is APPROVED by mtasaka
-----------------------------------------------------------------

Comment 12 Lubomir Rintel 2009-06-26 15:28:03 UTC
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-27 00:43:20 UTC
CVS done.

Comment 14 Mamoru TASAKA 2009-06-27 07:06:59 UTC
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 14:20:12 UTC
Closing this one.