Bug 565858 - Review Request: rubygem-thin - A thin and fast web server
Summary: Review Request: rubygem-thin - A thin and fast web server
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-02-16 15:02 UTC by Michal Fojtik
Modified: 2011-03-01 14:53 UTC (History)
5 users (show)

Fixed In Version: rubygem-thin-1.2.5-5.fc12
Clone Of:
Environment:
Last Closed: 2010-02-22 11:41:43 UTC
Type: ---
Embargoed:
mtasaka: fedora-review+


Attachments (Terms of Use)

Description Michal Fojtik 2010-02-16 15:02:58 UTC
Spec URL: http://mifo.sk/rubygem-thin.spec
SRPM URL: http://mifo.sk/rubygem-thin-1.2.5-1.fc12.src.rpm
Description: 

Thin is a Ruby web server that glues together 3 of the best Ruby libraries in web history:

- The Mongrel parser, the root of Mongrel speed and security
- Event Machine, a network I/O library with extremely high scalability, performance and stability
- Rack, a minimal interface between webservers and Ruby frameworks

Which makes it, with all humility, the most secure, stable, fast and extensible Ruby web server bundled in an easy to use gem for your own pleasure.

Comment 1 Mamoru TASAKA 2010-02-16 15:56:24 UTC
Some quick notes

- build fails, at least BR: ruby-devel is needed
  http://koji.fedoraproject.org/koji/taskinfo?taskID=1990997

- C extension modules should be installed under %ruby_sitearch,
  not under %geminstdir
  https://fedoraproject.org/wiki/Packaging/Ruby#Ruby_packages_with_binary_content.2Fshared_libraries

- To create debuginfo rpms correctly, you once have to install gem file
  under %_builddir (i.e. you cannot install this gem file under
  %buildroot directory, otherwise creating debuginfo rpm fails:
  https://fedoraproject.org/wiki/Packaging/Ruby#Ruby_Gem_with_extension_libraries_written_in_C

- As this gem contains spec/ directory, please add %check
  section and execute some test program (like $ rake spec ) there.

- Please use defined %geminstdir macro in %files

- "CHANGELOG" "COPYING" "README" (and usually also "Rakefile") should
  correctly marked as %doc.
  Also benchmark/ example/ spec/ tasks/ directories can perhaps be
  marked as %doc.

- ext/ directory are to compile C extention module (thin_parser.so)
  and need not be packaged into binary rpm.

- It seems that license tag should be "MIT and BSD and (Ruby or GPLv2)",
  however I will recheck this later.

Comment 2 Michal Fojtik 2010-02-17 13:13:41 UTC
(In reply to comment #1)
> Some quick notes
> 
> - build fails, at least BR: ruby-devel is needed
>   http://koji.fedoraproject.org/koji/taskinfo?taskID=1990997

FIXED:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1993283

> 
> - C extension modules should be installed under %ruby_sitearch,
>   not under %geminstdir

FIXED (this applies only to thin_parse.so)

> - To create debuginfo rpms correctly, you once have to install gem file
>   under %_builddir (i.e. you cannot install this gem file under
>   %buildroot directory, otherwise creating debuginfo rpm fails:
>  
> 

FIXED (?) Not sure.

> 
> - As this gem contains spec/ directory, please add %check
>   section and execute some test program (like $ rake spec ) there.

Actually, 'rake spec' produce 1 failure, which is relevant to Ruby version. We could omit this safely for now IMHO. 
(Also it raising some weird Ruby crashes [1])

> 
> - Please use defined %geminstdir macro in %files

FIXED

> 
> - "CHANGELOG" "COPYING" "README" (and usually also "Rakefile") should
>   correctly marked as %doc.
>   Also benchmark/ example/ spec/ tasks/ directories can perhaps be
>   marked as %doc.

All doc-relevant files was marked as 'doc'

> - ext/ directory are to compile C extention module (thin_parser.so)
>   and need not be packaged into binary rpm.

FIXED

> - It seems that license tag should be "MIT and BSD and (Ruby or GPLv2)",
>   however I will recheck this later.    

Regarding to thin web page[2], licence is: "Ruby License, www.ruby-lang.org/en/LICENSE.txt."

Spec URL: http://mifo.sk/rubygem-thin.spec
SRPM URL: http://mifo.sk/rubygem-thin-1.2.5-2.fc12.src.rpm

[1] https://bugzilla.redhat.com/show_bug.cgi?id=566153)
[2] http://code.macournoyer.com/thin/doc/files/README.html

Comment 3 Lubomir Rintel 2010-02-17 18:53:47 UTC
Your description is too short. You could use what you used here, just without formatting (" - ") and personal views ("with all humility...").

Comment 4 Michal Fojtik 2010-02-17 19:21:53 UTC
(In reply to comment #3)
> Your description is too short. You could use what you used here, just without
> formatting (" - ") and personal views ("with all humility...").    

Fixed ;)

Spec URL: http://mifo.sk/rubygem-thin.spec
SRPM URL: http://mifo.sk/rubygem-thin-1.2.5-3.fc12.src.rpm

Comment 5 Lubomir Rintel 2010-02-18 08:00:16 UTC
* Correctly named and versioned
* License correct, shipped with distribution
* SPEC file clean and legible
* Requires/provides sane
* Filelist ok, no duplicates
* Builds fine in mock
* Macros used consistently
* RPMlint silent

APPROVED

Comment 7 Michal Fojtik 2010-02-18 08:06:23 UTC
New Package CVS Request
=======================
Package Name: rubygem-thin
Short Description: A thin and fast web server
Owners: mfojtik
Branches: F-11 F-12 F-13 EL-5

Comment 8 Mamoru TASAKA 2010-02-18 08:17:54 UTC
Well, wait, this package still needs some fix...

Comment 9 Mamoru TASAKA 2010-02-18 08:27:33 UTC
@Lubomir
Sorry for interrupting you, however as this review request
still leaves at least one very critical issue to fix, I revoked
your approval. I appreciate your help anyway.

Comment 10 Mamoru TASAKA 2010-02-18 08:38:20 UTC
For -3:
(Maybe you seem to based your spec file on rubygem-nokogiri spec file)

* Unneeded recompilation
-----------------------------------------------------------------
pushd ./%{geminstdir}
find . -name \*.so -or -name \*.o -exec rm -f {} \;
rake -v compile --trace
-----------------------------------------------------------------
  - is unneeded. Building C module (thin_parser.so) is already done
    in %prep correctly
    ( for rubygem-nokogiri, gem install uses -O3 optimization level, while
      Fedora uses -O2, so recompilation is needed )

* "Moving" C module to %ruby_sitearch
------------------------------------------------------------------
    59  cp %{buildroot}%{geminstdir}/lib/*.so %{buildroot}%{ruby_sitearch}/%{gemname}/
------------------------------------------------------------------
  - This should be:
------------------------------------------------------------------
mv %{buildroot}%{geminstdir}/lib/*.so %{buildroot}%{ruby_sitearch}/
------------------------------------------------------------------
    ( arch specific binary should be "moved" to %ruby_sitearch. Also
      Originally this C module (thin_parser.so is under %geminstdir/lib,
      not under %geminstdir/lib/%gemname )

(In reply to comment #2)
> (In reply to comment #1)
> > - As this gem contains spec/ directory, please add %check
> >   section and execute some test program (like $ rake spec ) there.
> 
> Actually, 'rake spec' produce 1 failure, which is relevant to Ruby version. We
> could omit this safely for now IMHO. 
> (Also it raising some weird Ruby crashes [1])

- Then please kill these tests for now and enable tests.

* License
(In reply to comment #2)
> (In reply to comment #1)
> > - It seems that license tag should be "MIT and BSD and (Ruby or GPLv2)",
> >   however I will recheck this later.    
> 
> Regarding to thin web page[2], licence is: "Ruby License,
> www.ruby-lang.org/en/LICENSE.txt."

- However "COPYING" file is clearly MIT, and actually
-------------------------------------------------------------------
MIT
./COPYING
./spec/rails_app/public/

BSD
./lib/thin/stats.html.erb 

GPLv2 or Ruby (note: ruby is licensed under "GPLv2 or Ruby" in
Fedora's term)
./README
And some others
-------------------------------------------------------------------

Comment 11 Mamoru TASAKA 2010-02-18 09:08:41 UTC
Forgot one more critical thing

# env LANG=C rpm -ivh --test rubygem-thin-1.2.5-2.fc13.i686.rpm
error: Failed dependencies:
        /usr/local/bin/ruby is needed by rubygem-thin-1.2.5-2.fc13.i686

- Please fix invalid interpreter

Comment 12 Michal Fojtik 2010-02-18 11:10:13 UTC
Hopefully, I fixed all things. 

- Please fix invalid interpreter

FIXED (using sed)

- * License

FIXED (added multiple licenses + marked different licences in comments under %files)

- Unneeded recompilation

FIXED (I based my gem on nokogiri gem, my bad next time i'll double check this stuff)

- "Moving" C module to %ruby_sitearch

FIXED (Thanks for suggestion)

- Tests

FIXED (I switched to 'rake spec2', which tests basic functions and works great)

Spec URL: http://mifo.sk/rubygem-thin.spec
SRPM URL: http://mifo.sk/rubygem-thin-1.2.5-4.fc12.src.rpm 

Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=1996112
(builded fine on all 'Primary architectures', not sure about ppc64)

Comment 13 Mamoru TASAKA 2010-02-18 17:11:05 UTC
For -4:

* %exclude
  - Well, perhaps you used %exclude to group files by license,
    however %exclude completely removes listed files from
    the binary rpm, even if %exclude'd files are listed later
    (in the same subpackage %files list, %exclude'd files
     can appear in other subpackages).

    You'll see that %exclude'd files are actually not in
    rebuilt binary rpm.

* Directory ownership issue
  - The directory %{geminstdir}/bin itself is not owned by
    any packages.
    https://fedoraproject.org/wiki/Packaging/UnownedDirectories#Common_Mistakes

* License tag
  - In this case we should use "(GPLv2 or Ruby) and MIT and BSD" (i.e.
    need parentheses)

* ppc64
  - For rake spec2 failing issue, I think
-----------------------------------------------------------------------
%check
%ifarch ppc64
# Disable tests
exit 0
%endif
pushd .%{geminstdir}
....
....
-----------------------------------------------------------------------
    is preferable.

* Miscs
------------------------------------------------------------------------
rubygem-thin.src: E: description-line-too-long C Thin is a Ruby web server .....
------------------------------------------------------------------------
  - It is suggested that one line should not contain more than
    79 characters.

Comment 14 Michal Fojtik 2010-02-19 09:38:15 UTC
(In reply to comment #13)
> For -4:
> 
> * %exclude
>   - Well, perhaps you used %exclude to group files by license,
>     however %exclude completely removes listed files from
>     the binary rpm, even if %exclude'd files are listed later
>     (in the same subpackage %files list, %exclude'd files
>      can appear in other subpackages).
> 
>     You'll see that %exclude'd files are actually not in
>     rebuilt binary rpm.

FIXED

> 
> * Directory ownership issue
>   - The directory %{geminstdir}/bin itself is not owned by
>     any packages.
>     https://fedoraproject.org/wiki/Packaging/UnownedDirectories#Common_Mistakes

FIXED (hopefully ;)

> 
> * License tag
>   - In this case we should use "(GPLv2 or Ruby) and MIT and BSD" (i.e.
>     need parentheses)

FIXED (Sorry for this one)

> 
> * ppc64
>   - For rake spec2 failing issue, I think
> -----------------------------------------------------------------------
> %check
> %ifarch ppc64
> # Disable tests
> exit 0
> %endif
> pushd .%{geminstdir}
> ....
> ....
> -----------------------------------------------------------------------
>     is preferable.

FIXED. 

Anyway I'm not sure if this gem will work on this architecture at all. If basic test fails with SEGV I guess this gem will not work properly.

> 
> * Miscs
> ------------------------------------------------------------------------
> rubygem-thin.src: E: description-line-too-long C Thin is a Ruby web server
> .....
> ------------------------------------------------------------------------
>   - It is suggested that one line should not contain more than
>     79 characters.    

FIXED.

Spec URL: http://mifo.sk/rubygem-thin.spec
SRPM URL: http://mifo.sk/rubygem-thin-1.2.5-5.fc12.src.rpm

Comment 15 Mamoru TASAKA 2010-02-19 15:37:01 UTC
(In reply to comment #14)
> SRPM URL: http://mifo.sk/rubygem-thin-1.2.5-5.fc12.src.rpm    

Looks 404...

Comment 16 Michal Fojtik 2010-02-19 15:47:03 UTC
Oh, I'm sorry, I uploaded it into wrong directory. Now it will work.

Comment 17 Mamoru TASAKA 2010-02-19 16:05:56 UTC
Well,
* Directory ownership issue
  - Umm... isn't it enough that you write licenses of the included files
    as comments? Now the following directories are not owned:
---------------------------------------------------------------------
%{geminstdir}/lib/thin/
{geminstdir}/spec/rails_app/
---------------------------------------------------------------------

Comment 18 Michal Fojtik 2010-02-19 16:18:44 UTC
(In reply to comment #17)
> Well,
> * Directory ownership issue
>   - Umm... isn't it enough that you write licenses of the included files
>     as comments? Now the following directories are not owned:
> ---------------------------------------------------------------------
> %{geminstdir}/lib/thin/
> {geminstdir}/spec/rails_app/
> ---------------------------------------------------------------------    

Should be FIXED:

Spec URL: http://mifo.sk/rubygem-thin.spec
SRPM URL: http://mifo.sk/rubygem-thin-1.2.5-5.fc12.src.rpm 

Licences:
It's better to have files grouped by Licence.

Comment 19 Mamoru TASAKA 2010-02-19 17:18:37 UTC
Okay.

-------------------------------------------------------------
  This package (rubygem-thin) is APPROVED by mtasaka
-------------------------------------------------------------

Comment 20 Michal Fojtik 2010-02-19 17:32:19 UTC
Thank your Mamoru !

New Package CVS Request
=======================
Package Name: rubygem-thin
Short Description: A thin and fast web server
Owners: mfojtik
Branches: F-11 F-12 F-13 EL-5

Comment 21 Jason Tibbitts 2010-02-19 20:08:53 UTC
CVS done (by process-cvs-requests.py).

Comment 22 Fedora Update System 2010-02-22 11:30:39 UTC
rubygem-thin-1.2.5-5.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/rubygem-thin-1.2.5-5.fc12

Comment 23 Fedora Update System 2010-02-22 11:40:07 UTC
rubygem-thin-1.2.5-5.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/rubygem-thin-1.2.5-5.fc13

Comment 24 Fedora Update System 2010-03-04 00:04:21 UTC
rubygem-thin-1.2.5-5.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 25 Fedora Update System 2010-03-04 00:20:54 UTC
rubygem-thin-1.2.5-5.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 26 Michal Fojtik 2010-07-14 09:20:18 UTC
Package Change Request
======================
Package Name: rubygem-thin
New Branches: EL-6
Owners: mfojtik

Comment 27 Michal Fojtik 2011-03-01 13:53:48 UTC
Package Change Request
======================
Package Name: rubygem-thin
New Branches: F15
Owners: mfojtik

Comment 28 Jason Tibbitts 2011-03-01 14:53:36 UTC
This package already has an f15 branch.


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