[Bug 1315801] Review Request: rubygem-nio4r - New IO for Ruby

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=1315801



--- Comment #3 from Jun Aruga <jaruga@xxxxxxxxxx> ---
Hi, Vit

Thank you for your review!! This is in detail, and clear for me.
I updated my spec and srpm files for your review. The URLs is same with
previous review.

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

Can you do review again?

Also let me comment for your review comment.

(In reply to Vít Ondruch from comment #2)
> * Use virtual rubygem provides
>   - In you spec file, you are using "BuildRequires: rubygem-rspec", but this
>     would be nice to replace by "BuildRequires: rubygem(rspec)"

I got it. I updated to use "BuildRequires: rubygem(rspec)".


> * Simlify %prep section
>   - First of all, it is the best to do all the custom steps you need at the
> end 
>     of the %prep section. In that case, you are already in the right
> directory
>     and you can remove the pushd/popd stuff.
>   - Secondly, I would suggest to move the rpmlint fixes to %install section.
>     In this specific case, they are harmless, but it might happen that
> different
>     modification will cause troubles with gem rebuild done in %build section

Yes. I moved the rpmlint fixes to end of %install section.


> * rpmlint fixes
>   - I would suggest to delete the shebang ling instead of commenting it out.
> It
>     might be completely harmless, but I have the feeling that it might cause
> some
>     issues (but I might be totall wrong as well ;))
>   - It would be nice to report the fixes upstream. In case you have already
>     reported them, please provide link, so anybody can check next time what
>     is the status.

I deleted the shebang line in Rakefile by sed command.
And add the reported URL as a comment to spec file.


> * Bundled library
>   - It seems that this package bundles libev. Is there any chance to use the
>     system version of libev instead [1]? Or in the worst case provide the
>     "bundled" virtual provide.

I tried to separate bundled libev and using system libev (libev-devel). And I
suceeded to compile the nio4r with system libev, and to pass almost all the
rspec test cases.
But when I compared the bundled libev (version 4.22) and original source [1], I
found that the bundled libev had individually modified. [2][3]

So, finally I made a choice to use "bundled(<libname>) = <version>".

Also let me post the difference from previous review, as a reference. [4]


[1] http://dist.schmorp.de/libev/libev-4.22.tar.gz
[2]
https://github.com/celluloid/nio4r/commit/680143345726c5a64bb22376ca8fc3c6857019ae
[3]
https://github.com/celluloid/nio4r/commit/fba5c68ad25404b51c5e179111d91ca7c3fc073b
[4] The difference from previous review

--- a/rubygem-nio4r.spec
+++ b/rubygem-nio4r.spec
@@ -12,7 +12,15 @@ Source0:
https://rubygems.org/gems/%{gem_name}-%{version}.gem
 BuildRequires: ruby(release)
 BuildRequires: rubygems-devel
 BuildRequires: ruby-devel
-BuildRequires: rubygem-rspec
+BuildRequires: rubygem(rspec)
+
+# Bundled libev ev.c is modified from original version 4.22.
+# We have to use the bundled libev
+# intead of separating the bundled libev and using system libev.
+# See the modificaiton for ev.c
+#
https://github.com/celluloid/nio4r/commit/680143345726c5a64bb22376ca8fc3c6857019ae
+#
https://github.com/celluloid/nio4r/commit/fba5c68ad25404b51c5e179111d91ca7c3fc073b
+Provides: bundled(libev) = 4.22

 %description
 New IO for Ruby.
@@ -30,13 +38,6 @@ Documentation for %{name}.
 %prep
 gem unpack %{SOURCE0}

-PACKED_DIR=`basename %{SOURCE0} | sed 's/\.gem$//'`
-pushd ./${PACKED_DIR}
-# Fix the issues for rpmlint
-sed -i '/#!\/usr\/bin\/env rake/ s/^/#/' Rakefile
-chmod +x examples/echo_server.rb
-popd
-
 %setup -q -D -T -n  %{gem_name}-%{version}

 gem spec %{SOURCE0} -l --ruby > %{gem_name}.gemspec
@@ -50,6 +51,7 @@ gem build %{gem_name}.gemspec
 %gem_install

 %install
+
 mkdir -p %{buildroot}%{gem_dir}
 cp -a .%{gem_dir}/* \
         %{buildroot}%{gem_dir}/
@@ -60,6 +62,17 @@ cp -a .%{gem_extdir_mri}/{gem.build_complete,*.so}
%{buildroot}%{gem_extdir_mri}
 # Prevent dangling symlink in -debuginfo (rhbz#878863).
 rm -rf %{buildroot}%{gem_instdir}/ext/

+# Fix the issue for rpmlint
+# I reported it to upstream, and its fix was merged to master branch.
+# https://github.com/celluloid/nio4r/pull/86
+sed -i 's|^#!/usr/bin/env rake$||' %{buildroot}%{gem_instdir}/Rakefile
+
+# Fix the issue for rpmlint
+# I reported it to upstream, and its fix was merged to master branch.
+# https://github.com/celluloid/nio4r/pull/85
+chmod 755 %{buildroot}%{gem_instdir}/examples/echo_server.rb
+
+
 # Run the test suite
 %check
 pushd .%{gem_instdir}

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/package-review




[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]