Bug 1315801

I'll take this for a review!

* 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)"

* 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

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

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


Please resolve the issues prior we'll continue.


[1] https://fedoraproject.org/wiki/Packaging:Guidelines#Bundling_and_Duplication_of_system_libraries

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}

Just reminder. Waiting the response. :)

> > * 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>".

Once this gets into Fedora, could you please mention this virtual provide here:

https://fedoraproject.org/wiki/Bundled_Libraries_Virtual_Provides

Seems there was some changes in this policy like 6 weeks ago.




Also a few more nits:

* Please bump the release
  - It is good habit to bump the release every iteration, update %changelog
    as well as provide updated SRPM. That way, I can always check the progress
    since last time.(In reply to Jun Aruga from comment #3)

* Better inline comments
  - I would suggest to replace your comment:

      # 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

    by something more explanatory, e.g.:

      # Remove useless shebang.
      # https://github.com/celluloid/nio4r/pull/86

    I don't think that reference to rpmlint is useful there.

  - This note applies generally, also to other similar comments ;)

* License
  - Due to bundled libev, I think the license field should be updated. The
    libev license appears to be (BSD or GPLv2+) while the code of the rest of
    the package is MIT, the license tag should be:

      MIT and (BSD or GPLv2+)

Hi, Vit

I modified your mentioned points.
Could you check below updated files?

Spec URL: http://people.redhat.com/jaruga/git/rubygem-nio4r/rubygem-nio4r.spec
SRPM URL: http://people.redhat.com/jaruga/git/rubygem-nio4r/rubygem-nio4r-1.2.1-1.fc25.src.rpm
Description: New IO for Ruby.
Fedora Account System Username: jaruga
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=14712271

Thanks.

* Swap the description and summary
  - It seems that the description and summary are swapped. Could you swap them
    in the .spec file please?


But since this is only minor issue, I APPROVE the package. Please don't forget to update the "bundled libraries" page once you get this package into Fedora.


[1] https://fedoraproject.org/wiki/Bundled_Libraries_Virtual_Provides

Vit, thanks for your reivew.
I did swap the description and summary, bumped the release, and updated above files in the URL.

I am going to add this package "bundled libraries" after that.

Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/rubygem-nio4r

Pushed to master, built, and updated the wiki.

https://fedoraproject.org/wiki/Bundled_Libraries_Virtual_Provides