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