Bug 504469

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.

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.

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

(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

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?

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

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.

ping?

ping again?

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

-----------------------------------------------------------------
  This package (rubygem-term-ansicolor) is APPROVED by mtasaka
-----------------------------------------------------------------

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

CVS done.

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
  )

Closing this one.