Bug 641295 - Review Request: rubygem-typhoeus - A library for interacting with web services at blinding speed
Summary: Review Request: rubygem-typhoeus - A library for interacting with web service...
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-10-08 10:15 UTC by Michal Fojtik
Modified: 2010-10-29 20:37 UTC (History)
2 users (show)

Fixed In Version: rubygem-typhoeus-0.1.31-3.fc13
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-10-20 17:13:57 UTC
mtasaka: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Michal Fojtik 2010-10-08 10:15:45 UTC
Spec URL: http://mifo.sk/RPMS/rubygem-typhoeus.spec
SRPM URL: http://mifo.sk/RPMS/rubygem-typhoeus-0.1.31-1.fc13.src.rpm
Description:

Like a modern code version of the mythical beast with 100 serpent heads,
Typhoeus runs HTTP requests in parallel while cleanly encapsulating handling
logic.

Comment 1 Michal Fojtik 2010-10-08 10:17:10 UTC
Additional info:

* -devel and -doc packages are created
* This gem includes binary extension
* rpmlint output:

rubygem-typhoeus.i686: W: unstripped-binary-or-object /usr/lib/ruby/site_ruby/1.8/i386-linux/typhoeus/native.so
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

* koji build:

http://koji.fedoraproject.org/koji/taskinfo?taskID=2521882

Comment 2 Mamoru TASAKA 2010-10-08 19:36:42 UTC
Some notes:

* Unused macro
  - Defined %ruby_sitelib macro is used nowhere.

* License
  - License tag should be "MIT".

* (Build)Requires
  - ">= 0" part is not needed.
  - "BR: libcurl" is written twice, and anyway this is not needed
    because "BR: libcurl-devel" also exists and libcurl-devel
    requires libcurl.

* Provides
  - Would you explain why rubygem-typhoeus-devel should have
    "Provides: ruby-typhoeus-devel" and 
    "Provides: ruby(typhoeus-devel)"?
    ! Note that for example rubygem-glib2-devel has 
      Provides: ruby-glib2-devel is because there was actually
      "ruby-glib2-devel" package, and ruby-glib2-devel "wrongly"
      provided ruby(glib2-devel)" (so the latter Provides is
      only for compatibility for old Provides).
    ! Also, currently rubygem-typhoeus-devel has
      "Provides: ruby-%{name}-devel", where %{name} is expanded
      as rubygem-typhoeus (so as a whole it is expanded as
      ruby-rubygem-typhoeus-devel)

* Fedora specific compilation flags
-------------------------------------------------------------
    88  make
    89  gcc -I. -I. -I/usr/lib/ruby/1.8/i386-linux -I. -D_FILE_OFFSET_BITS=64  -fPIC -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i686 -mtune=atom -fasynchronous-unwind-tables  -g -DXP_UNIX -O3 -Wall -Wcast-qual -Wwrite-strings -Wconversion -Wmissing-noreturn -Winline  -c typhoeus_multi.c
-------------------------------------------------------------
  - Here Fedora specific compilation flags (%optflags) uses -O2
    but this is replaced by the latter -O3 flag, which needs fixing.
    This "-O3" flag comes from ext/typhoeus/extconf.rb, so this
    can be fixed only after executing "gem install", then
    fix ext/typhoeus/extconf.rb and recompiling (by $ ruby extconf.rb
    -> make clean all )

* Directory ownership issue
  - The directory %{ruby_sitearch}/typhoeus/ itself is not owned
    by any packages.

* ext/ directory
  - Would you explain why files in ext/ directory have to be installed?
    (usually this directory should not be needed)

! %check
  - Usually when the directory spec/ exists, I request to add %check section
    and execute $ rake spec there, however for this gem many test case
    fail with no rails server running...

Comment 3 Mamoru TASAKA 2010-10-08 19:39:34 UTC
By the way:

(In reply to comment #1)
> rubygem-typhoeus.i686: W: unstripped-binary-or-object
> /usr/lib/ruby/site_ruby/1.8/i386-linux/typhoeus/native.so

Would you check if you have surely installed redhat-rpm-config
on your system?

Also I would appreciate it if you would review my review
request (bug 637985)

Comment 4 Michal Fojtik 2010-10-13 13:50:09 UTC
(In reply to comment #2)

Thanks!

> Some notes:
> 
> * Unused macro
>   - Defined %ruby_sitelib macro is used nowhere.

FIXED

> 
> * License
>   - License tag should be "MIT".

FIXED

> 
> * (Build)Requires
>   - ">= 0" part is not needed.
>   - "BR: libcurl" is written twice, and anyway this is not needed
>     because "BR: libcurl-devel" also exists and libcurl-devel
>     requires libcurl.

FIXED

> 
> * Provides
>   - Would you explain why rubygem-typhoeus-devel should have
>     "Provides: ruby-typhoeus-devel" and 
>     "Provides: ruby(typhoeus-devel)"?
>     ! Note that for example rubygem-glib2-devel has 
>       Provides: ruby-glib2-devel is because there was actually
>       "ruby-glib2-devel" package, and ruby-glib2-devel "wrongly"
>       provided ruby(glib2-devel)" (so the latter Provides is
>       only for compatibility for old Provides).

Yes I'm sorry I looked at this package (glib2) and I thought that it's a 'recommended' way howto do it.

> 
> * Fedora specific compilation flags
> -------------------------------------------------------------
>     88  make
>     89  gcc -I. -I. -I/usr/lib/ruby/1.8/i386-linux -I. -D_FILE_OFFSET_BITS=64 
> -fPIC -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
> --param=ssp-buffer-size=4 -m32 -march=i686 -mtune=atom
> -fasynchronous-unwind-tables  -g -DXP_UNIX -O3 -Wall -Wcast-qual
> -Wwrite-strings -Wconversion -Wmissing-noreturn -Winline  -c typhoeus_multi.c
> -------------------------------------------------------------
>   - Here Fedora specific compilation flags (%optflags) uses -O2
>     but this is replaced by the latter -O3 flag, which needs fixing.
>     This "-O3" flag comes from ext/typhoeus/extconf.rb, so this
>     can be fixed only after executing "gem install", then
>     fix ext/typhoeus/extconf.rb and recompiling (by $ ruby extconf.rb
>     -> make clean all )

Fixed.

> 
> * Directory ownership issue
>   - The directory %{ruby_sitearch}/typhoeus/ itself is not owned
>     by any packages.

Fixed

> 
> * ext/ directory
>   - Would you explain why files in ext/ directory have to be installed?
>     (usually this directory should not be needed)

You are right. I'll omit -devel package and ext directory as well.

> 
> ! %check
>   - Usually when the directory spec/ exists, I request to add %check section
>     and execute $ rake spec there, however for this gem many test case
>     fail with no rails server running...

I removed failing tests a keep those which don't require Rails/Hydra connection.

> Also I would appreciate it if you would review my review
> request (bug 637985)

Sure thing.

====================== 0.1.31-2 ===================

* Wed Oct 13 2010 Michal Fojtik <mfojtik@redhat.com> - 0.1.31-2
- Fixed License to MIT
- Fixed libcurl BuildRequire
- Gem now recompiles with correct GCC flags
- Directory issues should be fixed
- Removed -devel subpackage
- Added tests

http://koji.fedoraproject.org/koji/taskinfo?taskID=2532378


Spec URL: http://mifo.sk/RPMS/rubygem-typhoeus.spec
SRPM URL: http://mifo.sk/RPMS/rubygem-typhoeus-0.1.31-2.fc13.src.rpm

Comment 5 Mamoru TASAKA 2010-10-13 20:12:47 UTC
For -2:

* Requires
  - "Requires: libcurl" is redundant and should be removed, because
    rpmbuild detects and adds this dependency automatically based on
    the dependency of the libraries:
    https://fedoraproject.org/wiki/Packaging/Guidelines#Requires

* Applying patch
  - It is better that patches are applied in %prep using %patchX
    macro.

? Tests needing Rails or so to be running
  - Well, I am not sure if it is preferable to remove tests
    which needs Rails or so to be running. 
    - When people makes Rails or so running, he/she may want
      to check typhoeus using these test scripts.
    Maybe it is better that
    - we don't remove such tests
    - and ignore the failure case for now
    - Or:
----------------------------------------------------------
%check
pushd .%{geminstdir}

NEEDSKIP=""
for needskip in \
  spec/typhoeus/request_spec.rb \
  spec/typhoeus/hydra_spec.rb \
  spec/typhoeus/remote_spec.rb \
  spec/typhoeus/multi_spec.rb \
  spec/typhoeus/easy_spec.rb \
  spec/typhoeus/remote_proxy_object_spec.rb
do
  NEEDSKIP="$NEEDSKIP $needskip"
done
	
for needskip in $NEEDSKIP
do
  mv $needskip ${needskip}.save
done

rake spec --trace

for needskip in $NEEDSKIP
do
  mv ${needskip}.save $needskip
done
----------------------------------------------------------
  ! By the way, $ rake spec needs "BuildRequires: rubygem(rake)".

* Consistent macro usage
  - Please use macros consistently. For example:
-----------------------------------------------------------
%files
%dir %{ruby_sitearch}/%{gemname}
%{ruby_sitearch}/typhoeus/*.so
-----------------------------------------------------------
    please use %{gemname} also in the third line.

* Miscs
-----------------------------------------------------------
    56  %install
    57  rm -rf %{buildroot}
    58  mkdir -p %{buildroot}%{ruby_sitearch}/%{gemname}
    59  mkdir -p %{buildroot}%{gemdir}
    60  mkdir -p %{buildroot}%{_prefix} 
-----------------------------------------------------------
  - The line 68 is unneeded.

Comment 6 Michal Fojtik 2010-10-14 10:49:25 UTC
(In reply to comment #5)
> For -2:
> 
> * Requires
>   - "Requires: libcurl" is redundant and should be removed, because
>     rpmbuild detects and adds this dependency automatically based on
>     the dependency of the libraries:
>     https://fedoraproject.org/wiki/Packaging/Guidelines#Requires

Fixed. Thanks for info btw, I didn't know that there is an autodetection.

> * Applying patch
>   - It is better that patches are applied in %prep using %patchX
>     macro.

Agree. Fixed.

> 
> ? Tests needing Rails or so to be running
>   - Well, I am not sure if it is preferable to remove tests
>     which needs Rails or so to be running. 
>     - When people makes Rails or so running, he/she may want
>       to check typhoeus using these test scripts.
>     Maybe it is better that
>     - we don't remove such tests
>     - and ignore the failure case for now
>     - Or:
> ----------------------------------------------------------
> %check
> pushd .%{geminstdir}
> 
> NEEDSKIP=""
> for needskip in \
>   spec/typhoeus/request_spec.rb \
>   spec/typhoeus/hydra_spec.rb \
>   spec/typhoeus/remote_spec.rb \
>   spec/typhoeus/multi_spec.rb \
>   spec/typhoeus/easy_spec.rb \
>   spec/typhoeus/remote_proxy_object_spec.rb
> do
>   NEEDSKIP="$NEEDSKIP $needskip"
> done
> 
> for needskip in $NEEDSKIP
> do
>   mv $needskip ${needskip}.save
> done
> 
> rake spec --trace
> 
> for needskip in $NEEDSKIP
> do
>   mv ${needskip}.save $needskip
> done

Fixed. Thanks for contributing this. I fully agree that those tests should be preserved in -doc package for futher testing.

> ----------------------------------------------------------
>   ! By the way, $ rake spec needs "BuildRequires: rubygem(rake)".

Fixed.

> 
> * Consistent macro usage
>   - Please use macros consistently. For example:
> -----------------------------------------------------------
> %files
> %dir %{ruby_sitearch}/%{gemname}
> %{ruby_sitearch}/typhoeus/*.so
> -----------------------------------------------------------
>     please use %{gemname} also in the third line.

Fixed.

> * Miscs
> -----------------------------------------------------------
>     56  %install
>     57  rm -rf %{buildroot}
>     58  mkdir -p %{buildroot}%{ruby_sitearch}/%{gemname}
>     59  mkdir -p %{buildroot}%{gemdir}
>     60  mkdir -p %{buildroot}%{_prefix} 
> -----------------------------------------------------------
>   - The line 68 is unneeded.

Fixed.

======================= 0.1.31-3 ====================

* Thu Oct 14 2010 Michal Fojtik <mfojtik@redhat.com> - 0.1.31-3
- Preserved failing test files (thx. to mtasaka)
- Fixed macros usage
- Replaced path with macro
- Removed libcurl from requires

Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=2534502

Spec URL: http://mifo.sk/RPMS/rubygem-typhoeus.spec
SRPM URL: http://mifo.sk/RPMS/rubygem-typhoeus-0.1.31-3.fc13.src.rpm

Comment 7 Mamoru TASAKA 2010-10-14 17:55:36 UTC
Some misc issue

* rpmlint issue
------------------------------------------------------------------
rubygem-typhoeus-doc.i686: E: non-executable-script /usr/lib/ruby/gems/1.8/gems/typhoeus-0.1.31/profilers/valgrind.rb 0644L /usr/bin/env
------------------------------------------------------------------
  - Please remove shebang

! About this redundant line
------------------------------------------------------------------
    61  %install
    62  rm -rf %{buildroot}
    63  mkdir -p %{buildroot}%{gemdir}
    64  mkdir -p %{buildroot}%{_prefix} 
------------------------------------------------------------------
  - What I mean here is that the line 64 is redundant because %gemdir = /usr/lib/ruby/gems/1.8
    and %_prefix = /usr, so the line 63 already creates %buildroot%_prefix.

Please fix the above issue before importing this package
into Fedora SCM.

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

Comment 8 Michal Fojtik 2010-10-17 14:30:46 UTC
Thanks ! I fixed both issues.

New Package SCM Request
=======================
Package Name:      rubygem-typhoeus
Short Description: A library for interacting with web services at blinding speed
Owners:            mfojtik
Branches:          f12 f13 f14

Comment 9 Kevin Fenzi 2010-10-19 04:03:29 UTC
Git done (by process-git-requests).

Comment 10 Fedora Update System 2010-10-19 11:11:33 UTC
rubygem-typhoeus-0.1.31-3.fc13 has been submitted as an update for Fedora 13.
https://admin.fedoraproject.org/updates/rubygem-typhoeus-0.1.31-3.fc13

Comment 11 Fedora Update System 2010-10-19 11:17:10 UTC
rubygem-typhoeus-0.1.31-3.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/rubygem-typhoeus-0.1.31-3.fc14

Comment 12 Fedora Update System 2010-10-20 03:09:53 UTC
rubygem-typhoeus-0.1.31-3.fc14 has been pushed to the Fedora 14 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update rubygem-typhoeus'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/rubygem-typhoeus-0.1.31-3.fc14

Comment 13 Mamoru TASAKA 2010-10-20 17:13:57 UTC
Closing this.

Comment 14 Fedora Update System 2010-10-28 06:20:15 UTC
rubygem-typhoeus-0.1.31-3.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 15 Fedora Update System 2010-10-29 20:37:27 UTC
rubygem-typhoeus-0.1.31-3.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.


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