Bug 237380 (ruby-gettext-package) - Review Request: ruby-gettext-package - Localization Library and Tools for Ruby
Summary: Review Request: ruby-gettext-package - Localization Library and Tools for Ruby
Status: CLOSED NEXTRELEASE
Alias: ruby-gettext-package
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Chitlesh GOORAH
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Keywords:
Depends On:
Blocks: alexandria
TreeView+ depends on / blocked
 
Reported: 2007-04-21 17:03 UTC by Mamoru TASAKA
Modified: 2007-11-30 22:12 UTC (History)
0 users

(edit)
Clone Of:
(edit)
Last Closed: 2007-05-12 03:32:48 UTC
chitlesh: fedora-review+
wtogami: fedora-cvs+


Attachments (Terms of Use)

Description Mamoru TASAKA 2007-04-21 17:03:29 UTC
Spec URL: http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SPECS/ruby-gettext-package.spec
SRPM URL: http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SRPMS/ruby-gettext-package-1.9.0-1.fc7.src.rpm
Mock build log on FC-devel i386: http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/LOGS/MOCK-ruby-gettext-package.log

Description: 
Ruby-GetText-Package is a Localization(L10n) Library and Tools 
which is modeled after GNU gettext package.

The library converts the messages to localized messages properly 
using client-side locale information. And the tools for developers 
support to create, use, and modify localized message files
(message catalogs) easily.

Comment 1 Jasper O. Hartline 2007-05-01 14:58:39 UTC
Chitlesh Goorah has asked me to do an informal review of this package.
Upon installing all BuildRequires, the package builds cleanly in Fedora Core 6
with a single warning from rpmlint:
W: ruby-gettext-package invalid-license Ruby License/LGPL

This should be LGPL rather than Ruby License/LGPL

In examining your SPEC file, I see you can collapse some Requires and
BuildRequires using this format:
BuildRequires:  irb, ruby-devel, ruby(abi = %{rubyabi}
Requires:       irb, ruby(abi) = %{rubyabi}

rather than:
BuildRequires:  irb
BuildRequires:  ruby-devel
BuildRequires:  ruby(abi) = %{rubyabi}
Requires:       ruby(abi) = %{rubyabi}
Requires:       irb

I also notice in the SPEC, the %files section has:
%files -f %{name}.lang
You might want to look into using:
%{find_lang} 
instead.

See: rpmbuild --showrc | grep find_lang



Comment 2 Mamoru TASAKA 2007-05-01 15:09:27 UTC
(In reply to comment #1)
> with a single warning from rpmlint:
> W: ruby-gettext-package invalid-license Ruby License/LGPL
> 
> This should be LGPL rather than Ruby License/LGPL
See the real COPYING text. This is licensed under
LGPL or Ruby License. This type of dual license is very
common for ruby related modules

> 
> In examining your SPEC file, I see you can collapse some Requires and
> BuildRequires using this format:
> BuildRequires:  irb, ruby-devel, ruby(abi = %{rubyabi}
> Requires:       irb, ruby(abi) = %{rubyabi}
> 
> rather than:
> BuildRequires:  irb
> BuildRequires:  ruby-devel
> BuildRequires:  ruby(abi) = %{rubyabi}
> Requires:       ruby(abi) = %{rubyabi}
> Requires:       irb

I _strongly_ recommend the latter style (i.e. my style) and
also some other reviewers recommend the latter style
* This style makes it easy that what is really changed on
  taking diff when the dependency is changed.
* This style makes diff output smaller.
* For this package, the BuildRequires has only 3 packages so
  the difference is small.
  However, please consider the case in which one package has
  29 BuildRequires......

> I also notice in the SPEC, the %files section has:
> %files -f %{name}.lang
> You might want to look into using:
> %{find_lang} 
> instead.

Already I use %find_lang. Note that there is no 
%{name}.mo files
--------------------------------------------
%find_lang rails
%find_lang rgettext
%{__cat} *.lang >> %{name}.lang
---------------------------------------------

Comment 3 Mamoru TASAKA 2007-05-05 18:19:34 UTC
Any issues left on this package?

Comment 4 Chitlesh GOORAH 2007-05-06 16:40:02 UTC
Same as ruby-amazon, the folders samples and test were omitted. Their contents 
should be added to one or more sub packages.

Comment 5 Mamoru TASAKA 2007-05-06 18:24:55 UTC
Well:

http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SPECS/ruby-gettext-package.spec
http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SRPMS/ruby-gettext-package-1.9.0-2.fc7.src.rpm
------------------------------------------------
* Mon May  7 2007 Mamoru Tasaka <mtasaka@ioa.s.u-tokyo.ac.jp> - 1.9.0-2
- Create -doc subpackage

For rpmlint:
------------------------------------------------
1 W: ruby-gettext-package invalid-license Ruby License/LGPL
1 W: ruby-gettext-package-debuginfo invalid-license Ruby License/LGPL
2 E: ruby-gettext-package-doc zero-length
/usr/share/doc/ruby-gettext-package-doc-1.9.0/samples/rails/public/favicon.ico
3 W: ruby-gettext-package-doc spurious-executable-perm
/usr/share/doc/ruby-gettext-package-doc-1.9.0/test/test.sh
1 W: ruby-gettext-package-doc invalid-license Ruby License/LGPL
------------------------------------------------
1: This type of dual license is very common for ruby related
   software
2: Well, I am not familiar with "ruby on rails" and I am not sure
   if I should (can) remove this file or not......
3: This is okay because this script is actually executable and
   only /bin/sh dependency is added to -doc subpackage due to
   this permission.

Comment 6 Chitlesh GOORAH 2007-05-10 18:32:30 UTC
MUST Items:

- MUST: The package is named according to the Package Naming Guidelines.
- MUST: The spec file name matches the base package %{name}
- MUST: The package meets the Packaging Guidelines.
- MUST: The package is licensed with an open-source compatible license 
and meet other legal requirements as defined in the legal section of Packaging
Guidelines.
- MUST: The License field in the package spec file matches the actual license.
- MUST: the source package includes the text of the license(s) in its own 
file, then that file, containing the text of the license(s) for the package is
included in %doc.
- MUST: The spec file must be written in American English.
- MUST: The spec file for the package is be legible. 
- MUST: The sources used to build the package must matches the upstream 
source, as provided in the spec URL.
- MUST: The package successfully compiles and builds into binary rpms on at
least i386.
- MUST: All build dependencies is listed in BuildRequires.
- MUST: The spec file handles locales properly.
- MUST: If the package does not contain shared library files located in the
dynamic linker's default paths
- MUST: the package is not designed to be relocatable
- MUST: the package owns all directories that it creates.
- MUST: the package does not contain any duplicate files in the %files 
listing.
- MUST: Permissions on files are set properly.
- MUST: The package has a %clean section, which contains rm -rf %{buildroot} 
(or
$RPM_BUILD_ROOT).
- MUST: The package consistently uses macros, as described in the macros 
section of Packaging Guidelines.
- MUST: The package contains code, or permissable content. This is described 
in detail in the code vs. content section of Packaging Guidelines.
- MUST: There are no Large documentation files
- MUST: %doc does not affect the runtime of the application. To summarize: If 
it is in %doc, the program must run properly if it is not present.
- MUST: There are no Header files or static libraries 
- MUST: The package does not contain library files with a suffix 
- MUST: Package does NOT contain any .la libtool archives
- MUST: Package containing GUI applications includes a %{name}.desktop file, 
and that file must be properly installed with desktop-file-install in 
the %install section.
- MUST: Package does not own files or directories already owned by other 
packages. 

SHOULD Items:

 - SHOULD: The source package does include license text(s) as COPYING
 - SHOULD: mock builds successfully in i386.
 - SHOULD: The reviewer tested that the package functions as described. A
package should not segfault instead of running, for example.
 - SHOULD: No scriptlets were used, those scriptlets must be sane. 
 - SHOULD: No subpackages present.

APPROVED

Comment 7 Mamoru TASAKA 2007-05-10 18:36:43 UTC
Thank you!!

Request for CVS admin:
-----------------------------------------------
New Package CVS Request
=======================
Package Name:         ruby-gettext-package
Short Description:    Localization Library and Tools for Ruby
Owners:               mtasaka@ioa.s.u-tokyo.ac.jp
Branches:             devel FC-6 FC-5
InitialCC:            (nobody)
-----------------------------------------------

Comment 8 Mamoru TASAKA 2007-05-12 03:32:48 UTC
Rebuild done on all branches, closing

Thank you for review!!


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