https://bugzilla.redhat.com/show_bug.cgi?id=1116021
Bug ID: 1116021 Summary: Review Request: rubygem-ruby-prof - a fast ruby profiler Product: Fedora Version: rawhide Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: steve.traylen@cern.ch QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org
Spec URL: http://cern.ch/straylen/rpms/rubygem-ruby-prof/rubygem-ruby-prof.spec SRPM URL: http://cern.ch/straylen/rpms/rubygem-ruby-prof/rubygem-ruby-prof-0.15.1-1.fc... Description: ruby-prof is a fast code profiling tool for Ruby. It is a C extension and therefore is many times faster than the standard Ruby profiling tool. It supports both flat and graph profiles. For each method, graph profiles show how long the method ran, which methods called it and which methods it called. RubyProf can generate both text and HTML and can output it to standard out or to a file.
Fedora Account System Username: stevetraylen
https://bugzilla.redhat.com/show_bug.cgi?id=1116021
Steve Traylen steve.traylen@cern.ch changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |1116024
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=1116024 [Bug 1116024] Review Request: rubygem-elasticsearch-extensions - Extensions for the Elasticsearch Rubygem.
https://bugzilla.redhat.com/show_bug.cgi?id=1116021
František Dvořák valtri@civ.zcu.cz changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |valtri@civ.zcu.cz
--- Comment #1 from František Dvořák valtri@civ.zcu.cz --- Some comments from me (although I'm not experienced ruby packager):
* license: it is rather "BSD" than "MIT"
rubygems.org refers to MIT, but the LICENSE file and .gemspec file contains BSD (2-clause). Were is source of the license for rubygems.org? (maybe upstream could be notified?)
* rake is not recommended for launching the tests
Rake is not typically used in Fedora to execute test suite, since it almost always needs some additional unnecessary dependencies. I were lucky to launch tests this way:
mkdir tmp ruby -Ilib:%{buildroot}%{gem_extdir_mri}/lib:test -e 'Dir.glob "./test/**/*_test.rb", &method(:require)'
==> rake-compiler would not be needed then, although I think there is required rubygem(test-unit) BR?
* %{buildroot}%{gem_instdir}/lib/ruby_prof.so doesn't exists (package can't be built in rawhide)
I think the move command should look like this?:
mv %{buildroot}%{gem_instdir}/ext/ruby_prof/ruby_prof.so %{buildroot}%{gem_extdir_mri}/lib/
* E: non-standard-executable-perm /usr/lib64/gems/ruby/ruby-prof-0.15.1/lib/ruby_prof.so 0775L
Permissions of %{gem_extdir_mri}/lib/ruby_prof.so should be rather 0755 then 0775.
* W: no-manual-page-for-binary ruby-prof, ruby-prof-check-trace
It is recommended to work with upstream to create man-pages, or to check Debian: https://fedoraproject.org/wiki/Packaging:Guidelines?rd=PackagingGuidelines#M...
* other issues seems OK or false positives: - dir ownership in debuginfo - doc subpackage dependency OK (it's noarch) - bundled font files OK (rdoc generated) - here is described porting to minitest 5: https://lists.fedoraproject.org/pipermail/ruby-sig/2014-June/001594.html, I've tried some porting and were lucky with that, but the patch seems quite aggressive and '%check' will be needed disabled anyway on some branches...
https://bugzilla.redhat.com/show_bug.cgi?id=1116021
--- Comment #2 from František Dvořák valtri@civ.zcu.cz --- (In reply to František Dvořák from comment #1)
Some comments from me (although I'm not experienced ruby packager):
license: it is rather "BSD" than "MIT"
rubygems.org refers to MIT, but the LICENSE file and .gemspec file
contains BSD (2-clause). Were is source of the license for rubygems.org? (maybe upstream could be notified?)
Just noticed this commit: https://github.com/ruby-prof/ruby-prof/commit/84f086b5ecb02cf8f077acc537cadc...
It is not release yet, I guess rubygems.org will be OK with next release of ruby-prof.
https://bugzilla.redhat.com/show_bug.cgi?id=1116021
--- Comment #3 from Steve Traylen steve.traylen@cern.ch --- Thanks for taking a look.
Spec URL: http://cern.ch/straylen/rpms/rubygem-ruby-prof/rubygem-ruby-prof.spec SRPM URL: http://cern.ch/straylen/rpms/rubygem-ruby-prof/rubygem-ruby-prof-0.15.1-2.fc...
addresses all your comments above with the exception of adding man pages. The commands have --help support which is enough I would say.
As for the licensing it's clearly meant to be BSD indeed , I've added a comment highlighting the discrepancy with rubygems.org which should hopefully fix at some time.
Only other change was I deleted a duplicate copy of arch depedendent .so file incorrectly in /usr/share.
https://bugzilla.redhat.com/show_bug.cgi?id=1116021
--- Comment #4 from Steve Traylen steve.traylen@cern.ch --- ping.
https://bugzilla.redhat.com/show_bug.cgi?id=1116021
Troy Dawson tdawson@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |tdawson@redhat.com Assignee|nobody@fedoraproject.org |tdawson@redhat.com
--- Comment #5 from Troy Dawson tdawson@redhat.com --- Hi Steve, I'll take this, but due to a bug in the spec file, it's failing on my fedora-review, so I can't do a full review yet. Can you do three things for me.
1 - Change "rm %{buildroot}%{gem_instdir}/lib/ruby_prof.so" to "rm -f %{buildroot}%{gem_instdir}/lib/ruby_prof.so" That will fix my bug. On rawhide that file isn't there, but a force allows it go just move along if it isn't there.
2 - Update to the latest version (0.15.8) This will also fix the license problem, because the license then matches the gemspec upstream. You can then remove the comments about it in the spec file.
3 - Ensure all the tests still work with the updated version.
https://bugzilla.redhat.com/show_bug.cgi?id=1116021
--- Comment #6 from Upstream Release Monitoring upstream-release-monitoring@fedoraproject.org --- jgrulich's scratch build of kdevelop?#c8e2b9bc57f11e41f3dc6612cdbcc591078d9062 for f22-candidate and git://pkgs.fedoraproject.org/kdevelop?#c8e2b9bc57f11e41f3dc6612cdbcc591078d9062 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11212117
https://bugzilla.redhat.com/show_bug.cgi?id=1116021
--- Comment #7 from Troy Dawson tdawson@redhat.com --- I'm dropping being a reviewer of this package, and I recommend it be closed incase someone else wants to create this package. There has been too long without any response from the original packager.
https://bugzilla.redhat.com/show_bug.cgi?id=1116021
Troy Dawson tdawson@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |NEW CC|tdawson@redhat.com | Assignee|tdawson@redhat.com |nobody@fedoraproject.org
https://bugzilla.redhat.com/show_bug.cgi?id=1116021
Steve Traylen steve.traylen@cern.ch changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |CLOSED Resolution|--- |WONTFIX Last Closed| |2017-05-31 05:19:37
--- Comment #8 from Steve Traylen steve.traylen@cern.ch --- So old, can't remember by why I wanted it. Apologies for the lack of response.
package-review@lists.fedoraproject.org