Bug 836368

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=PackageMaintainers/UsingKoji

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

(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

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.

(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

Taking.

I would appreciate it if you would review my review request bug 872910.

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)

(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

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

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

As bug 874249 is already approved (by me), please update this one.

(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

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

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

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

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

(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_gems
>     )

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

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.

I've decided not to proceed with packaging this Gem at this time.