Bug 504709

http://mtasaka.fedorapeople.org/Review_request/Ruby_on_Rails/rubygem-gettext_activerecord.spec
http://mtasaka.fedorapeople.org/Review_request/Ruby_on_Rails/rubygem-gettext_activerecord-2.0.4-2.fc.src.rpm

* Fri Jun 12 2009 Mamoru Tasaka <mtasaka.u-tokyo.ac.jp> - 2.0.4-2
- Also mark gettext mo files under test/ directory with %%lang

Here are my notes for this package:

! The timestamps in the source gem file are all wrong. Please ask upstream to correct this.

? rpmlint says
   rubygem-gettext_activerecord-doc.noarch: W: no-documentation
Shouldn't the contents of this package be marked %doc ?

* Please remove the binary .mo files in %prep

? The COPYING file you are packaging claims LGPL as the license. 
The source files say "You may redistribute it and/or modify it under the same license terms as Ruby."
Meanwhile the license tag says "GPLv2 or Ruby"
What's going on? :)

? What are these BuildRequires(check)'s for? (I never saw them before. And Fedora guidelines don't mention them)

* A package must not contain any duplicate files in the %files listing. build.log says
   warning: File listed twice: /usr/lib/ruby/gems/1.8/gems/gettext_activerecord-2.0.4/test/test_parser.rb
   warning: File listed twice: /usr/lib/ruby/gems/1.8/gems/gettext_activerecord-2.0.4/test/test_validations.rb

* The indentation seems wrong with this line
     * extract messages from models with the rake task.

* Ruby packaging guidelines say that the %build section of the specfile should be empty and the install should be performed with the command
   gem install --local --install-dir %{buildroot}%{gemdir} --force %{SOURCE0}
Any reason why you are doing it differently?

- koji rawhide build seems fine:
   http://koji.fedoraproject.org/koji/taskinfo?taskID=1414345

Hi:

(In reply to comment #2)
> ! The timestamps in the source gem file are all wrong. Please ask upstream to
> correct this.
- This is rather the issue on "gem", not this gem specific.

> ? rpmlint says
>    rubygem-gettext_activerecord-doc.noarch: W: no-documentation
> Shouldn't the contents of this package be marked %doc ?
- I don't think it is needed to mark as %doc the files in the rpm
  which is already declared as "document rpm"

> * Please remove the binary .mo files in %prep
- Done at %build

> ? The COPYING file you are packaging claims LGPL as the license. 
> The source files say "You may redistribute it and/or modify it under the same
> license terms as Ruby."
> Meanwhile the license tag says "GPLv2 or Ruby"
> What's going on? :)
- Actually from the source code this is licensed under the same
  license as ruby. Note that ruby is licensed under "GPLv2 or Ruby"
  (well, a little complicated...), so the license tag should
  be so.

> ? What are these BuildRequires(check)'s for? (I never saw them before. And
> Fedora guidelines don't mention them)
- The intention for this is that "These BuildRequires is needed only
  for %check".

> * A package must not contain any duplicate files in the %files listing.
> build.log says
>    warning: File listed twice:
> /usr/lib/ruby/gems/1.8/gems/gettext_activerecord-2.0.4/test/test_parser.rb
>    warning: File listed twice:
> /usr/lib/ruby/gems/1.8/gems/gettext_activerecord-2.0.4/test/test_validations.rb
- See the test case bug 505995. I thought this worked as expected.
  Workarround applied.

> * The indentation seems wrong with this line
>      * extract messages from models with the rake task.
- Fixed.

> * Ruby packaging guidelines say that the %build section of the specfile should
> be empty and the install should be performed with the command
>    gem install --local --install-dir %{buildroot}%{gemdir} --force %{SOURCE0}
> Any reason why you are doing it differently?
- Actually it is under discussion whether "gem install" should completely
  moved from %install to %prep or %build. This suggestion was from other people
  but now I also think that "gem install" should not done at %install
  directly
  because:
  - Actually when gem creates C module extension, rubygem guideline
    already says "gem --install" should be done at %build because
    of creating debuginfo rpm correctly (and this proposal was
    from me)
  - For this package, spec file contains %check. When gem is installed
    under %buildroot, %check must be done also under %buildroot.
    This is troublesome when %check (for this package "rake test") creates
    some additional files because this will cause some unneeded filed
    to be packaged or causes "installed but not packaged" error.

http://mtasaka.fedorapeople.org/Review_request/Ruby_on_Rails/rubygem-gettext_activerecord.spec
http://mtasaka.fedorapeople.org/Review_request/Ruby_on_Rails/rubygem-gettext_activerecord-2.0.4-3.fc.src.rpm

* Mon Jun 15 2009 Mamoru Tasaka <mtasaka.u-tokyo.ac.jp> - 2.0.4-3
- Recreate gettext mo files (BR: rubygem(gettext))
- Change BR: ruby(sqlite) -> rubygem(sqlite3-ruby)
- Some cleanups

(In reply to comment #3)
> Hi:

Hi, thanks for the update! 

> (In reply to comment #2)
> > ? The COPYING file you are packaging claims LGPL as the license. 
> > The source files say "You may redistribute it and/or modify it under the 
> > same license terms as Ruby."
> > Meanwhile the license tag says "GPLv2 or Ruby"
> > What's going on? :)
> - Actually from the source code this is licensed under the same
>   license as ruby. Note that ruby is licensed under "GPLv2 or Ruby"
>   (well, a little complicated...), so the license tag should
>   be so.
> 

Okay, but the COPYING file that is being packaged says LGPL, which is wrong according to what you say. Shouldn't we drop the wrong COPYING file?

> > ? What are these BuildRequires(check)'s for? (I never saw them before. And
> > Fedora guidelines don't mention them)
> - The intention for this is that "These BuildRequires is needed only
>   for %check".
> 

Thanks. Now the package fails to build on F-11
   + rake --trace makemo
   /var/tmp/rpm-tmp.qYI3ky: line 41: rake: command not found

I moved rubygem(rake) from BR(check) to regular BR, but then I got
   + rake --trace makemo              
   /usr/lib/ruby/site_ruby/1.8/rubygems/custom_require.rb:31: command not found: allison --path

I couldn't figure out what package provides allison on F-11.

> 
> > * Ruby packaging guidelines say that the %build section of the specfile 
> > should be empty and the install should be performed with the command
> >    gem install --local --install-dir %{buildroot}%{gemdir} --force 
> > %{SOURCE0}
> > Any reason why you are doing it differently?
> - Actually it is under discussion whether "gem install" should completely
>   moved from %install to %prep or %build. This suggestion was from other 
>   people but now I also think that "gem install" should not done at %install
>   directly
>   because:
>   - Actually when gem creates C module extension, rubygem guideline
>     already says "gem --install" should be done at %build because
>     of creating debuginfo rpm correctly (and this proposal was
>     from me)
>   - For this package, spec file contains %check. When gem is installed
>     under %buildroot, %check must be done also under %buildroot.
>     This is troublesome when %check (for this package "rake test") creates
>     some additional files because this will cause some unneeded filed
>     to be packaged or causes "installed but not packaged" error.
> 

Do you know by any chance when these changes in the guidelines will be approved? I can't find the relevant discussion. Maybe I'm looking at the wrong mailing lists.

(In reply to comment #4)
> > (In reply to comment #2)
> > > ? The COPYING file you are packaging claims LGPL as the license. 
> > > The source files say "You may redistribute it and/or modify it under the 
> > > same license terms as Ruby."
> > > Meanwhile the license tag says "GPLv2 or Ruby"
> > > What's going on? :)
> > - Actually from the source code this is licensed under the same
> >   license as ruby. Note that ruby is licensed under "GPLv2 or Ruby"
> >   (well, a little complicated...), so the license tag should
> >   be so.
> > 
> 
> Okay, but the COPYING file that is being packaged says LGPL, which is wrong
> according to what you say. Shouldn't we drop the wrong COPYING file?

- Removed.

> Thanks. Now the package fails to build on F-11
>    + rake --trace makemo
>    /var/tmp/rpm-tmp.qYI3ky: line 41: rake: command not found

- Ah.. fixed.

> I moved rubygem(rake) from BR(check) to regular BR, but then I got
>    + rake --trace makemo              
>    /usr/lib/ruby/site_ruby/1.8/rubygems/custom_require.rb:31: command not
> found: allison --path
> 
> I couldn't figure out what package provides allison on F-11.

- allison is not packaged on Fedora yet (and I have not checked
  the dependency...)

> Do you know by any chance when these changes in the guidelines will be
> approved? I can't find the relevant discussion. Maybe I'm looking at the wrong
> mailing lists.
- Well, I will re-propose this later on fedora-packaging (after I write
  a draft: formal proposal needs a draft).

http://mtasaka.fedorapeople.org/Review_request/Ruby_on_Rails/rubygem-gettext_activerecord-2.0.4-4.fc.src.rpm
http://mtasaka.fedorapeople.org/Review_request/Ruby_on_Rails/rubygem-gettext_activerecord.spec

* Tue Jun 16 2009 Mamoru Tasaka <mtasaka.u-tokyo.ac.jp> - 2.0.4-4
- Remove COPYING for now
- Change BR: now rubygem(rake) is needed to create gettext mo

Thank you. But this still fails to build on F-11:
   http://koji.fedoraproject.org/koji/taskinfo?taskID=1415248

I don't understand why, it builds fine on rawhide.

Ah..

http://mtasaka.fedorapeople.org/Review_request/Ruby_on_Rails/rubygem-gettext_activerecord.spec
http://mtasaka.fedorapeople.org/Review_request/Ruby_on_Rails/rubygem-gettext_activerecord-2.0.4-5.fc.src.rpm

* Tue Jun 16 2009 Mamoru Tasaka <mtasaka.u-tokyo.ac.jp> - 2.0.4-5
- More BR: rubygem(activerecord) for recreating mo files

dist-f12:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1415364
dist-f11-updates-candidate:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1415362

Thanks. One last question:
From the SPEC file:

   %global activerecord_req        2.3.2
   ...
   %description
   gettext_activerecord provides the localization for ActiveRecord-2.2 or later
   using Ruby-GetText-Package.

There is a discrepancy here on version number. Am I missing something?

Well, that is not a blocker. You can answer it later.

---------------------------------------------------------------
This package (rubygem-gettext_activerecord) is APPROVED by oget
---------------------------------------------------------------

Thank you for quick review!

(In reply to comment #8)
>    %global activerecord_req        2.3.2
>    ...
>    %description
>    gettext_activerecord provides the localization for ActiveRecord-2.2 or later
>    using Ruby-GetText-Package.

I will remove "-2.2 or later" part.


New Package CVS Request
=======================
Package Name:       rubygem-gettext_activerecord
Short Description:  Localization support for ActiveRecord by Ruby-GetText-Package
Owners:             mtasaka
Branches:           F-11
InitialCC:          (nobody)

(By the way I packaged rubygem-allison as bug 506168, not
 strictly needed for this package, though)

CVS done.

Rebuilt and push request submitted, closing.

Thank you for the review and CVS procedure!

Package Change Request
======================
Package Name: rubygem-gettext_activerecord
New Branches: el5 el6
Owners: stahnma
InitialCC: 
Mamoru has said I can maintain any of his ruby packages in EPEL.

Git done (by process-git-requests).