https://bugzilla.redhat.com/show_bug.cgi?id=836368
Bug ID: 836368 QA Contact: extras-qa@fedoraproject.org Severity: medium Version: rawhide Priority: medium CC: notting@redhat.com, package-review@lists.fedoraproject.org Assignee: nobody@fedoraproject.org Summary: Review Request: rubygem-bicho - Library to access Bugzilla Regression: --- Story Points: --- Classification: Fedora OS: Linux Reporter: dpierce@redhat.com Type: --- Documentation: --- Hardware: All Mount Type: --- Status: NEW Component: Package Review Product: Fedora
Spec URL: http://mcpierce.fedorapeople.org/rpms/rubygem-bicho.spec SRPM URL: http://mcpierce.fedorapeople.org/rpms/rubygem-bicho-0.0.6-1.fc17.src.rpm Description: Library to access Bugzilla. Fedora Account System Username: mcpierce
https://bugzilla.redhat.com/show_bug.cgi?id=836368
Josef Stribny jstribny@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |jstribny@redhat.com
--- Comment #1 from Josef Stribny jstribny@redhat.com --- Hi,
* rpmlint gave me no errors on your spec and SRPM. I also run a scratch koji build: `koji --scratch f19 rubygem-bicho-0.0.6-1.fc17.src.rpm` and it built just fine [1].
For the next time I suggest you to include a link to koji with successfully built SRPM before you submit a review request. Additional information about koji is available on fedoraproject.org wiki [2].
* The spec file seems alright.
(This is an informal review.)
[1] http://koji.fedoraproject.org/koji/taskinfo?taskID=4603039 [2] https://fedoraproject.org/wiki/Using_the_Koji_build_system?rd=PackageMaintai...
https://bugzilla.redhat.com/show_bug.cgi?id=836368
--- Comment #2 from Josef Stribny jstribny@redhat.com --- Also:
* The spec file doesn't run tests. You should always run the test suite, especially if it's provided by upstream. Include the tests that come with the gem and run them in %check section. If they are based on Test::Unit you can use minitest to run them (which is a recommended way).
In your spec file:
- require minitest BuildRequires: rubygem(minitest)
- add check section that goes after %build %check pushd .%{gem_instdir} testrb -Ilib test
When I added this %check section I got following errors:
``` Finished tests in 2.049980s, 2.4390 tests/s, 1.9512 assertions/s.
1) Error: test_oscrc_parsing(NovellPlugin_test): NameError: undefined local variable or method `user' for Bicho::Plugins::Novell:Class
/home/strzibny/rpmbuild/BUILD/rubygem-bicho-0.0.6/usr/share/gems/gems/bicho-0.0.6/lib/bicho/plugins/novell.rb:70:in `oscrc_credentials'
/home/strzibny/rpmbuild/BUILD/rubygem-bicho-0.0.6/usr/share/gems/gems/bicho-0.0.6/test/test_novell_plugin.rb:21:in `test_oscrc_parsing'
2) Error: test_urls_are_correct(NovellPlugin_test): NameError: undefined local variable or method `user' for Bicho::Plugins::Novell:Class
/home/strzibny/rpmbuild/BUILD/rubygem-bicho-0.0.6/usr/share/gems/gems/bicho-0.0.6/lib/bicho/plugins/novell.rb:70:in `oscrc_credentials'
/home/strzibny/rpmbuild/BUILD/rubygem-bicho-0.0.6/usr/share/gems/gems/bicho-0.0.6/lib/bicho/plugins/novell.rb:77:in `transform_api_url_hook'
/home/strzibny/rpmbuild/BUILD/rubygem-bicho-0.0.6/usr/share/gems/gems/bicho-0.0.6/lib/bicho/client.rb:115:in `block in initialize'
/home/strzibny/rpmbuild/BUILD/rubygem-bicho-0.0.6/usr/share/gems/gems/bicho-0.0.6/lib/bicho/client.rb:108:in `each'
/home/strzibny/rpmbuild/BUILD/rubygem-bicho-0.0.6/usr/share/gems/gems/bicho-0.0.6/lib/bicho/client.rb:108:in `initialize'
/home/strzibny/rpmbuild/BUILD/rubygem-bicho-0.0.6/usr/share/gems/gems/bicho-0.0.6/test/test_novell_plugin.rb:7:in `new'
/home/strzibny/rpmbuild/BUILD/rubygem-bicho-0.0.6/usr/share/gems/gems/bicho-0.0.6/test/test_novell_plugin.rb:7:in `test_urls_are_correct'
5 tests, 4 assertions, 0 failures, 2 errors, 0 skips error: Bad exit status from /var/tmp/rpm-tmp.FF2Q3X (%check) ``` Note: I get this also with the current master branch [1].
[1] https://github.com/dmacvicar/bicho
https://bugzilla.redhat.com/show_bug.cgi?id=836368
--- Comment #3 from Darryl L. Pierce dpierce@redhat.com --- (In reply to comment #2)
Also:
- The spec file doesn't run tests. You should always run the test suite,
especially if it's provided by upstream. Include the tests that come with the gem and run them in %check section. If they are based on Test::Unit you can use minitest to run them (which is a recommended way).
In your spec file:
- require minitest
BuildRequires: rubygem(minitest)
- add check section that goes after %build
%check pushd .%{gem_instdir} testrb -Ilib test
The test that fails has a dependency on Novell. So, for now, I'm going to skip adding any test checks to the specfile.
Scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=4662553
https://bugzilla.redhat.com/show_bug.cgi?id=836368
Bohuslav "Slavek" Kabrda bkabrda@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |bkabrda@redhat.com
--- Comment #4 from Bohuslav "Slavek" Kabrda bkabrda@redhat.com --- Hi Darryl, rubygem-bicho depends on rubygem-inifile in runtime. I don't see inifile in Fedora, nor it has an opened review. I think you should first package all the dependencies before actually finishing this review. Inifile seems to be the only thing you will need.
https://bugzilla.redhat.com/show_bug.cgi?id=836368
--- Comment #5 from Darryl L. Pierce dpierce@redhat.com --- (In reply to comment #4)
Hi Darryl, rubygem-bicho depends on rubygem-inifile in runtime. I don't see inifile in Fedora, nor it has an opened review. I think you should first package all the dependencies before actually finishing this review. Inifile seems to be the only thing you will need.
Thanks, I missed that in my initial packaging. I've packaged that and have it up for review as well:
https://bugzilla.redhat.com/show_bug.cgi?id=874249
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=836368
--- Comment #6 from Mamoru TASAKA mtasaka@fedoraproject.org --- Taking.
I would appreciate it if you would review my review request bug 872910.
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=836368
Mamoru TASAKA mtasaka@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|nobody@fedoraproject.org |mtasaka@fedoraproject.org
--- Comment #7 from Mamoru TASAKA mtasaka@fedoraproject.org --- First of all, please take a look at https://fedoraproject.org/wiki/Packaging:Ruby?rd=Packaging/Ruby#RubyGems
and change your spec file to match the current gem related packaging guidelines. (Especially, current guideline requests that gem is unpacked first using gem unpack)
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=836368
--- Comment #8 from Darryl L. Pierce dpierce@redhat.com --- (In reply to comment #7)
First of all, please take a look at https://fedoraproject.org/wiki/Packaging:Ruby?rd=Packaging/Ruby#RubyGems
and change your spec file to match the current gem related packaging guidelines. (Especially, current guideline requests that gem is unpacked first using gem unpack)
Done.
Updated SPEC: http://mcpierce.fedorapeople.org/rpms/rubygem-bicho.spec Updated SRPM: http://mcpierce.fedorapeople.org/rpms/rubygem-bicho-0.0.6-2.fc17.src.rpm
Scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=4773672
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=836368
Mamoru TASAKA mtasaka@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Depends On| |874249
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=836368
--- Comment #9 from Mamoru TASAKA mtasaka@fedoraproject.org --- For 0.0.6-2:
! bug dependency - As this bug depends on 836368, I cannot approve this bug until bug 836368 is updated.
* About C extension related things - This package is noarch, no C code is included, so C extension related things are not needed. (e.g. export CONFIGURE_ARGS=.... writing this on noarch srpm is confusing)
* Unneeded files - https://fedoraproject.org/wiki/Packaging:Ruby?rd=Packaging/Ruby#Running_test... - Current packaging guidelines requests NOT to ship files under test/
- Also, "Rakefile" is something like "Makefile", which we do not ship in binary rpms.
- %{gem_instdir}/bicho.gemspec is usually also not needed.
* Executing test
https://fedoraproject.org/wiki/Packaging:Ruby?rd=Packaging/Ruby#Running_test... - As this gem contains test/ directory, please execute some test suite on %check if possible. If not possible, please write some comments on the spec file (e.g. test suite needs network access or so)
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=836368
--- Comment #10 from Mamoru TASAKA mtasaka@fedoraproject.org --- As bug 874249 is already approved (by me), please update this one.
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=836368
--- Comment #11 from Darryl L. Pierce dpierce@redhat.com --- (In reply to comment #9)
For 0.0.6-2:
! bug dependency
- As this bug depends on 836368, I cannot approve this bug until bug 836368 is updated.
- About C extension related things
- This package is noarch, no C code is included, so C extension related things are not needed. (e.g. export CONFIGURE_ARGS=.... writing this on noarch srpm is confusing)
Removed.
- Unneeded files
https://fedoraproject.org/wiki/Packaging:Ruby?rd=Packaging/ Ruby#Running_test_suites - Current packaging guidelines requests NOT to ship files under test/
Also, "Rakefile" is something like "Makefile", which we do not ship in binary rpms.
%{gem_instdir}/bicho.gemspec is usually also not needed.
I've excluded these files from the package.
- Executing test
https://fedoraproject.org/wiki/Packaging:Ruby?rd=Packaging/ Ruby#Running_test_suites
- As this gem contains test/ directory, please execute some test suite on %check if possible. If not possible, please write some comments on the spec file (e.g. test suite needs network access or so)
To run the tests requires BRs for rubygem-nokogiri and rubygem-inifile. This works locally, but won't be buildable via Koji until the git repo is made for rubygem-inifile.
Updated SPEC: http://mcpierce.fedorapeople.org/rpms/rubygem-bicho.spec updated SRPM: http://mcpierce.fedorapeople.org/rpms/rubygem-bicho-0.0.6-2.1.fc17.src.rpm
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=836368
Vít Ondruch vondruch@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |vondruch@redhat.com
--- Comment #12 from Vít Ondruch vondruch@redhat.com --- A few minor nits from my side:
* Run test suite in %{gem_instdir} - We typically run test suite inside %{gem_instdir}, but it should not be of much importance here
* Keep the Gemfile*, Rakefile and tests - I would suggest yout to keep the abovementioned files in -doc subpackage. Although they make not much sense in the Gem, I would keep them in Fedoras packages, since upstream ships them. Or on the contrary, ask upstream to remove them from the package.
* Don't mark Rakefile by %doc macro - Rakefile is definitely not a document and should not be prepended by %doc macro
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=836368
--- Comment #13 from Vít Ondruch vondruch@redhat.com --- (In reply to comment #11)
(In reply to comment #9) Removed.
- Unneeded files
https://fedoraproject.org/wiki/Packaging:Ruby?rd=Packaging/ Ruby#Running_test_suites - Current packaging guidelines requests NOT to ship files under test/
Also, "Rakefile" is something like "Makefile", which we do not ship in binary rpms.
%{gem_instdir}/bicho.gemspec is usually also not needed.
I've excluded these files from the package.
Sorry, I noticed this comment a bit late. But anyway, there are these two lines:
%exclude %{gem_instdir}/Rakefile %doc %{gem_instdir}/Rakefile
which should be corrected somehow.
Not sure if we should not revisit the "Do not ship tests" in guidelines though.
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=836368
--- Comment #14 from Darryl L. Pierce dpierce@redhat.com --- (In reply to comment #13)
Sorry, I noticed this comment a bit late. But anyway, there are these two lines:
%exclude %{gem_instdir}/Rakefile %doc %{gem_instdir}/Rakefile
which should be corrected somehow.
Okay, I'll remove the %doc line when I push the first package for Fedora.
Not sure if we should not revisit the "Do not ship tests" in guidelines though.
Not to pull in a separate discussion on this bug, but in thinking about this, what is the benefit to shipping tests? It seems they're more of use for developers and for validation during the build process and don't really serve much purpose being installed in an RPM.
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=836368
--- Comment #15 from Mamoru TASAKA mtasaka@fedoraproject.org --- Umm... there are some issues I did not notice before. Not sure why I missed them, however anyway:
* Non-equality dependency - Well, I did not know that rpm accepts something like "Requires: rubygem(inifile) *=>* 0.4.1" (not >= but =>), however I don't think this is a normal usage on rpm.
Note that "rpm -qp --requires rubygem-bicho-0.0.6-2.1.fc17.src.rpm" shows "rubygem(inifile) >= 0.4.1", so "=>" is actually regarded as ">=" (by rpm) currently.
* Unsatisfied dependency - Well, bicho requiers inifile no less than 0.4.1 and less than 0.5 (according to metadata), however rubygem-inifile imported into Fedora is 2.0.2, so rubygem-bicho (built from this package) won't work. Please fix dependency for inifile.
! Note that you have to modify dependency not on rpm spec file, but also on %gem_spec file included in rubygem-bicho binary rpm.
? (Question) test result on %check - From build log, test result on %check looks like: -------------------------------------------------- + ruby -Ilib test/helper.rb test/test_novell_plugin.rb test/test_query.rb Run options:
# Running tests:
Finished tests in 0.000805s, 0.0000 tests/s, 0.0000 assertions/s.
0 tests, 0 assertions, 0 failures, 0 errors, 0 skips -------------------------------------------------- Note "0 tests". I don't regard this as a blocker for this review, however please recheck this.
* Build failure - This srpm does not build on koji (for F-19). at least "BR: rubygem(minitest)" or so is needed. ref: http://koji.fedoraproject.org/koji/taskinfo?taskID=4836144
* File list - The following entry -------------------------------------------------- %{gem_instdir}/test %exclude %{gem_instdir}/test -------------------------------------------------- on the _same_ binary rpm is confusing. The latter entry (i.e. only %exclude ... line) is enough.
* Installing original gem, not regenerated gem - In %build: -------------------------------------------------- %build gem build %{gem_name}.gemspec
gem install --local \ --install-dir .%{gem_dir} \ --bindir .%{_bindir} \ --force \ %{SOURCE0} -------------------------------------------------- Well, using %{SOURCE0} (i.e. original gem file) here is not what ruby packaging guideline requests. Use regenerated gem file here (see: example written on:
https://fedoraproject.org/wiki/Packaging:Ruby?rd=Packaging/Ruby#Building_gem... )
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=836368
Bug 836368 depends on bug 874249, which changed state.
Bug 874249 Summary: Review Request: rubygem-inifile - INI file reader and writer https://bugzilla.redhat.com/show_bug.cgi?id=874249
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution|--- |NEXTRELEASE
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=836368
--- Comment #16 from Darryl L. Pierce dpierce@redhat.com --- (In reply to comment #15)
Umm... there are some issues I did not notice before. Not sure why I missed them, however anyway:
Non-equality dependency
Well, I did not know that rpm accepts something like "Requires: rubygem(inifile) *=>* 0.4.1" (not >= but =>), however I don't think this is a normal usage on rpm.
Note that "rpm -qp --requires rubygem-bicho-0.0.6-2.1.fc17.src.rpm" shows "rubygem(inifile) >= 0.4.1", so "=>" is actually regarded as ">=" (by rpm) currently.
Done.
Unsatisfied dependency
- Well, bicho requiers inifile no less than 0.4.1 and less than 0.5 (according to metadata), however rubygem-inifile imported into Fedora is 2.0.2, so rubygem-bicho (built from this package) won't work. Please fix dependency for inifile.
! Note that you have to modify dependency not on rpm spec file, but also on %gem_spec file included in rubygem-bicho binary rpm.
I'm going to work with the upstream to fix this. There doesn't seem to be a dependency on that specific version of inifile since the code works well with inifile 2.0.2. Actually, it appears that all of the requirements in the gemspec seem to be unnecessarily specific or even, really, completely necessary. I don't have any but inifile installed and yet the code works correctly for me.
? (Question) test result on %check
- From build log, test result on %check looks like:
- ruby -Ilib test/helper.rb test/test_novell_plugin.rb test/test_query.rb
Run options:
# Running tests:
Finished tests in 0.000805s, 0.0000 tests/s, 0.0000 assertions/s.
0 tests, 0 assertions, 0 failures, 0 errors, 0 skips
Note "0 tests". I don't regard this as a blocker for this review, however please recheck this.
Build failure
- This srpm does not build on koji (for F-19). at least "BR: rubygem(minitest)" or so is needed. ref: http://koji.fedoraproject.org/koji/taskinfo?taskID=4836144
File list
- The following entry
%{gem_instdir}/test %exclude %{gem_instdir}/test
on the _same_ binary rpm is confusing. The latter entry (i.e. only %exclude ... line) is enough.
Dropped the former line.
- Installing original gem, not regenerated gem
- In %build:
%build gem build %{gem_name}.gemspec
gem install --local \ --install-dir .%{gem_dir} \ --bindir .%{_bindir} \ --force \ %{SOURCE0}
Well, using %{SOURCE0} (i.e. original gem file) here is not what ruby packaging guideline requests. Use regenerated gem file here (see: example written on:
https://fedoraproject.org/wiki/Packaging:Ruby?rd=Packaging/Ruby#Building_gem... )
Fixed.
Updated SPEC: http://mcpierce.fedorapeople.org/rpms/rubygem-bicho.spec Updated SRPM: http://mcpierce.fedorapeople.org/rpms/rubygem-bicho-0.0.6-2.2.fc17.src.rpm Scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=4845739
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=836368
--- Comment #17 from Mamoru TASAKA mtasaka@fedoraproject.org --- For -2.2:
* Non-equality dependency - Please fix the line below, too. ------------------------------------------------------- Requires: rubygem(highline) "=>" 1.6.2 -------------------------------------------------------
* Unsatisfied dependency - As I said in the comment 15 (but "not on" should be read as "not only on", sorry), you have to modify gemspec file, too. Currently: ------------------------------------------------------- [mtasaka@localhost ~]$ rpm -q rubygem-bicho rubygem-bicho-0.0.6-2.2.fc.noarch [mtasaka@localhost ~]$ ruby -e "require 'bicho'" /usr/share/rubygems/rubygems/dependency.rb:247:in `to_specs': Could not find inifile (~> 0.4.1) amongst [RubyInline-3.11.0, ZenTest-4.6.2, abstract-1.0.0, actionmailer-3.2.8, actionpack-3.2.8, .... , zoom-0.4.1] (Gem::LoadError) from /usr/share/rubygems/rubygems/specification.rb:777:in `block in activate_dependencies' from /usr/share/rubygems/rubygems/specification.rb:766:in `each' from /usr/share/rubygems/rubygems/specification.rb:766:in `activate_dependencies' from /usr/share/rubygems/rubygems/specification.rb:750:in `activate' from /usr/share/rubygems/rubygems.rb:212:in `rescue in try_activate' from /usr/share/rubygems/rubygems.rb:209:in `try_activate' from /usr/share/rubygems/rubygems/custom_require.rb:59:in `rescue in require' from /usr/share/rubygems/rubygems/custom_require.rb:35:in `require' from -e:1:in `<main>' -------------------------------------------------------
* File list - Rakefile still seems to be installed in -doc.
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=836368
Darryl L. Pierce dpierce@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution|--- |WONTFIX Last Closed| |2013-01-12 11:11:32
--- Comment #18 from Darryl L. Pierce dpierce@redhat.com --- I've decided not to proceed with packaging this Gem at this time.
package-review@lists.fedoraproject.org