Bug 738590 - Review request: rubygem-gettext_i18n_rails - Simple FastGettext Rails integration
Summary: Review request: rubygem-gettext_i18n_rails - Simple FastGettext Rails integra...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Vít Ondruch
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 727155
Blocks: 705509
TreeView+ depends on / blocked
 
Reported: 2011-09-15 10:04 UTC by Bohuslav "Slavek" Kabrda
Modified: 2015-04-03 13:08 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-01-04 14:54:29 UTC
vondruch: fedora-review+
puiterwijk: fedora-cvs+


Attachments (Terms of Use)

Description Bohuslav "Slavek" Kabrda 2011-09-15 10:04:24 UTC
SPEC: http://bkabrda.fedorapeople.org/gettext_i18n_rails/rubygem-gettext_i18n_rails.spec
SRPM: http://bkabrda.fedorapeople.org/gettext_i18n_rails/rubygem-gettext_i18n_rails-0.2.20-1.fc15.src.rpm

Note:
To build you currently need rubygem-fast_gettext from https://bugzilla.redhat.com/show_bug.cgi?id=727155

Additional info: the ruby_gettext_extractor.rb file seems to be a bundled library, but it has been changed after bundling, so I think it's neither appropriate nor necessary to try to remove it and package it on its own. See the discussion at https://github.com/retoo/ruby_gettext_extractor/issues/4

Comment 1 Vít Ondruch 2011-09-21 06:13:52 UTC
Taking this for a review.

Comment 2 Vít Ondruch 2011-10-25 11:48:08 UTC
* Update to the latest version
  - Please consider updating to the latest version of gem. It seems it should
    incorporate the failed tests fix.
* License is not clear
  - The license should be "Public Domain and MIT", however I rose the question
    upstream to clarify [1]
* Please keep the note [2] about the "bundled" ruby_gettext_extractor
  in the spec file
* Exclude the cached gem
  - The cached gem has no meaning in Fedora. I would suggest to use the 
    following line in your spec:
    
    %exclude %{gemdir}/cache/%{gemname}-%{version}.gem
* Keep Readme.md in the main package
  - The file contains the license information. It would be fine to keep it
    in the main package.
* Keep the VERSION file in main package
  - This file is required by runtime:

    irb(main):002:0> require 'rubygems'
    => true
    irb(main):003:0> require 'gettext_i18n_rails'
    Errno::ENOENT: No such file or directory - /usr/lib/ruby/gems/1.8/gems
            /gettext_i18n_rails-0.2.20/lib/../VERSION
	from /usr/lib/ruby/gems/1.8/gems/gettext_i18n_rails-0.2.20
            /lib/gettext_i18n_rails.rb:2:in `read'
	from /usr/lib/ruby/gems/1.8/gems/gettext_i18n_rails-0.2.20
            /lib/gettext_i18n_rails.rb:2
	from /usr/lib/ruby/site_ruby/1.8/rubygems/custom_require.rb:59:in
            `gem_original_require'
	from /usr/lib/ruby/site_ruby/1.8/rubygems/custom_require.rb:59:in
            `require'
	from (irb):3
	from /usr/lib64/ruby/1.8/x86_64-linux/rbconfig.rb:172
  - Note that it should not be marked by %doc macro in this case.

[1] https://github.com/grosser/gettext_i18n_rails/issues/37
[2] https://github.com/retoo/ruby_gettext_extractor/issues/4

Comment 3 Bohuslav "Slavek" Kabrda 2011-10-26 07:13:30 UTC
(In reply to comment #2)
> * Update to the latest version
>   - Please consider updating to the latest version of gem. It seems it should
>     incorporate the failed tests fix.

Done.

> * License is not clear
>   - The license should be "Public Domain and MIT", however I rose the question
>     upstream to clarify [1]

You are right, modified to Public Domain and MIT.

> * Please keep the note [2] about the "bundled" ruby_gettext_extractor
>   in the spec file

Done.

> * Exclude the cached gem
>   - The cached gem has no meaning in Fedora. I would suggest to use the 
>     following line in your spec:
> 
>     %exclude %{gemdir}/cache/%{gemname}-%{version}.gem

Done.

> * Keep Readme.md in the main package
>   - The file contains the license information. It would be fine to keep it
>     in the main package.

Done.

> * Keep the VERSION file in main package
>   - This file is required by runtime:
> 
>     irb(main):002:0> require 'rubygems'
>     => true
>     irb(main):003:0> require 'gettext_i18n_rails'
>     Errno::ENOENT: No such file or directory - /usr/lib/ruby/gems/1.8/gems
>             /gettext_i18n_rails-0.2.20/lib/../VERSION
>  from /usr/lib/ruby/gems/1.8/gems/gettext_i18n_rails-0.2.20
>             /lib/gettext_i18n_rails.rb:2:in `read'
>  from /usr/lib/ruby/gems/1.8/gems/gettext_i18n_rails-0.2.20
>             /lib/gettext_i18n_rails.rb:2
>  from /usr/lib/ruby/site_ruby/1.8/rubygems/custom_require.rb:59:in
>             `gem_original_require'
>  from /usr/lib/ruby/site_ruby/1.8/rubygems/custom_require.rb:59:in
>             `require'
>  from (irb):3
>  from /usr/lib64/ruby/1.8/x86_64-linux/rbconfig.rb:172
>   - Note that it should not be marked by %doc macro in this case.
> 

Good catch! Done.

> [1] https://github.com/grosser/gettext_i18n_rails/issues/37
> [2] https://github.com/retoo/ruby_gettext_extractor/issues/4

SPEC: http://bkabrda.fedorapeople.org/gettext_i18n_rails/rubygem-gettext_i18n_rails.spec
SRPM: bkabrda.fedorapeople.org/gettext_i18n_rails/rubygem-gettext_i18n_rails-0.3.0-1.fc15.src.rpm
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=3460621

Comment 4 Vít Ondruch 2011-10-26 11:33:43 UTC
(In reply to comment #3)

> > * License is not clear
> >   - The license should be "Public Domain and MIT", however I rose the question
> >     upstream to clarify [1]
> 
> You are right, modified to Public Domain and MIT.

It is not that simple. The original project contains MIT LICENSE file, while the source code itself says "the same license as Ruby". That is pretty confusing ...

Comment 5 Bohuslav "Slavek" Kabrda 2011-10-26 11:37:07 UTC
Yes, you are right, I'm currently trying to solve this issue: https://github.com/retoo/ruby_gettext_extractor/issues/5.

Comment 6 Vít Ondruch 2011-10-26 12:20:00 UTC
One more nit. The "%{geminstdir}/init.rb" file seems to be Rails plugin initializer, which is not used in gem version of the library. If that is true, it should be moved into -doc subpackage or completely excluded. But this is not blocker.

Otherwise, the package looks good. So once the license is clarified, the package could be approved.

Comment 7 Bohuslav "Slavek" Kabrda 2012-01-02 10:13:45 UTC
- The license has been clarified - the gettext_i18n_rails is now the official upstream and the problematic license note has been removed from ruby_gettext_extractor.rb, so everything is now under Public Domain.
- I moved the %{geminstdir}/init.rb to -doc subpackage as suggested, the package works fine without it.
- I also upgraded to the newest version and fixed dependencies accordingly.

SPEC: http://bkabrda.fedorapeople.org/pkgs/gettext_i18n_rails/rubygem-gettext_i18n_rails.spec
SRPM: http://bkabrda.fedorapeople.org/pkgs/gettext_i18n_rails/rubygem-gettext_i18n_rails-0.3.6-1.fc17.src.rpm
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=3613621

Comment 8 Vít Ondruch 2012-01-02 15:02:07 UTC
rpmlint reports following warning:

rubygem-gettext_i18n_rails.src: W: strange-permission rubygem-gettext_i18n_rails.spec 0666L
A file that you listed to include in your package has strange permissions.
Usually, a file should have 0644 permissions.

* Please consider removing of "Requires: rubygem(gettext)", since it is rails
  application development dependency, not a runtime think.
* Similarly "Requires: rubygem(ruby_parser)" should be removed for the same
  reason. Unfortunately, this dependency is now enforced by upstream. I have
  already opened issue [1] because of this.

Nevertheless, since these are just minor nits, I APPROVE the package. Please fix them before commit if possible.


[1] https://github.com/grosser/gettext_i18n_rails/issues/49

Comment 9 Bohuslav "Slavek" Kabrda 2012-01-04 07:22:26 UTC
New Package SCM Request
=======================
Package Name: rubygem-gettext_i18n_rails
Short Description: Simple FastGettext Rails integration
Owners: bkabrda
Branches: 
InitialCC:

Comment 10 Gwyn Ciesla 2012-01-04 13:38:42 UTC
Git done (by process-git-requests).

Comment 11 Akira TAGOH 2015-04-03 03:01:45 UTC
Package Change Request
======================
Package Name: rubygem-gettext_i18n_rails
New Branches: el6
Owners: tagoh
InitialCC:

talked to vondruch and got an approval to request a branch.

Comment 12 Patrick Uiterwijk 2015-04-03 13:08:17 UTC
Git done (by process-git-requests).

The current package PoC, vondruch, is in http://fedoraproject.org/wiki/EPEL/ContributorStatusNo


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