Bug 565858

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.

(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

Your description is too short. You could use what you used here, just without formatting (" - ") and personal views ("with all humility...").

(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

* 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

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

Well, wait, this package still needs some fix...

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

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

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

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)

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.

(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

(In reply to comment #14)
> SRPM URL: http://mifo.sk/rubygem-thin-1.2.5-5.fc12.src.rpm    

Looks 404...

Oh, I'm sorry, I uploaded it into wrong directory. Now it will work.

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/
---------------------------------------------------------------------

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

Okay.

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

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

CVS done (by process-cvs-requests.py).

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

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

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.

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.

Package Change Request
======================
Package Name: rubygem-thin
New Branches: EL-6
Owners: mfojtik

Package Change Request
======================
Package Name: rubygem-thin
New Branches: F15
Owners: mfojtik

This package already has an f15 branch.