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

[bugzilla]

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

Jun Aruga  changed:

   What|Removed |Added

 Status|MODIFIED|CLOSED
 Resolution|--- |NEXTRELEASE
Last Closed||2016-06-30 12:05:00



-- 
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@lists.fedoraproject.org
https://lists.fedoraproject.org/admin/lists/package-review@lists.fedoraproject.org

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

[bugzilla]

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

Jun Aruga  changed:

   What|Removed |Added

 Status|ASSIGNED|MODIFIED
   Fixed In Version||rubygem-nio4r-1.2.1-2.fc25
   Assignee|vondr...@redhat.com |jar...@redhat.com



--- Comment #10 from Jun Aruga  ---
Pushed to master, built, and updated the wiki.

https://fedoraproject.org/wiki/Bundled_Libraries_Virtual_Provides

-- 
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@lists.fedoraproject.org
https://lists.fedoraproject.org/admin/lists/package-review@lists.fedoraproject.org

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

[bugzilla]

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



--- Comment #9 from Jon Ciesla  ---
Package request has been approved:
https://admin.fedoraproject.org/pkgdb/package/rpms/rubygem-nio4r

-- 
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@lists.fedoraproject.org
https://lists.fedoraproject.org/admin/lists/package-review@lists.fedoraproject.org

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

[bugzilla]

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



--- Comment #8 from Jun Aruga  ---
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.

-- 
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@lists.fedoraproject.org
https://lists.fedoraproject.org/admin/lists/package-review@lists.fedoraproject.org

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

[bugzilla]

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

Vít Ondruch  changed:

   What|Removed |Added

  Flags|fedora-review?  |fedora-review+



--- Comment #7 from Vít Ondruch  ---
* 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

-- 
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@lists.fedoraproject.org
https://lists.fedoraproject.org/admin/lists/package-review@lists.fedoraproject.org

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

[bugzilla]

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



--- Comment #5 from Vít Ondruch  ---
> > * 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() = ".

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

-- 
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@lists.fedoraproject.org
https://lists.fedoraproject.org/admin/lists/package-review@lists.fedoraproject.org

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

[bugzilla]

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



--- Comment #4 from Jun Aruga  ---
Just reminder. Waiting the response. :)

-- 
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@lists.fedoraproject.org
http://lists.fedoraproject.org/admin/lists/package-review@lists.fedoraproject.org

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

[bugzilla]

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



--- Comment #3 from Jun Aruga  ---
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() = ".

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@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

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

[bugzilla]

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



--- Comment #2 from Vít Ondruch  ---
* 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

-- 
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@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

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

[bugzilla]

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

Vít Ondruch  changed:

   What|Removed |Added

 Status|NEW |ASSIGNED
 CC||vondr...@redhat.com
   Assignee|nob...@fedoraproject.org|vondr...@redhat.com
  Flags||fedora-review?



--- Comment #1 from Vít Ondruch  ---
I'll take this for a review!

-- 
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@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review