Bug 810926

Hi Darryl,

Are you aware of ruby-qpid? What is the relation between your gem and this package? Shouldn't this gem obsolete the ruby-qpid?

BTW it seems that this gem is not available on RubyGems.org nor in the listed source URL. This is at least a bit unusual.

(In reply to comment #1)
> Hi Darryl,
> 
> Are you aware of ruby-qpid? What is the relation between your gem and this
> package? Shouldn't this gem obsolete the ruby-qpid?
> 
> BTW it seems that this gem is not available on RubyGems.org nor in the listed
> source URL. This is at least a bit unusual.

The old ruby-qpid package is based on the old pure Ruby Qpid and is obsolete. This new package is based on the current, Swig-based bindings and is how we're providing the language bindings in future releases.

The gem isn't available on rubygems.org ATM: I haven't explored how to make it available there yet. I will be making it available there at some point in future, actually will look into that right now.

So are you going to contact maintainers of ruby-qpid to retire the ruby-qpid and add "Obsolete: ruby(qpid)"? Than would be nice.

(In reply to comment #3)
> So are you going to contact maintainers of ruby-qpid to retire the ruby-qpid
> and add "Obsolete: ruby(qpid)"? Than would be nice.

The package maintainer (nsantos) will retire it once rubygem-qpid is approved and generally available as a replacement.

I will coordinate with Darryl to obsolete ruby-qpid.

I've updated the specfile to include that it provides the ruby-qpid functionality:

Updated SPEC: http://mcpierce.fedorapeople.org/rpms/rubygem-qpid.spec
Updated SRPM: http://mcpierce.fedorapeople.org/rpms/rubygem-qpid-0.16.0-2.fc16.src.rpm

I've also been in touch with the person who owns the unrelated gem on rubygems.org named QPID. He's going to release the name to us so that, in future, we can also distributed the gem via RubyGems.org as well.

Hi Darryl,

You should follow the rename guidelines [1] IMO. So the "Provides: ruby-qpid" should be accompanied by "Obsoletes" directive.

So for F16, it should look like:

Provides: ruby-qpid = %{version}-%{release}
Obsoletes: ruby-qpid < 0.8-2

Btw since the package needs to go into Rawhide first, it would be nice if you could prepare the .spec file for Rawhide.


[1] https://fedoraproject.org/wiki/Packaging:Guidelines#Renaming.2FReplacing_Existing_Packages

(In reply to comment #7)
> Hi Darryl,
> 
> You should follow the rename guidelines [1] IMO. So the "Provides: ruby-qpid"
> should be accompanied by "Obsoletes" directive.
> 
> So for F16, it should look like:
> 
> Provides: ruby-qpid = %{version}-%{release}
> Obsoletes: ruby-qpid < 0.8-2
> 
> Btw since the package needs to go into Rawhide first, it would be nice if you
> could prepare the .spec file for Rawhide.
> 
> 
> [1]
> https://fedoraproject.org/wiki/Packaging:Guidelines#Renaming.2FReplacing_Existing_Packages

Changes are made per your recommendation:

Updates SPEC: http://mcpierce.fedorapeople.org/rpms/rubygem-qpid.spec
Updated SRPM: http://mcpierce.fedorapeople.org/rpms/rubygem-qpid-0.16.0-3.fc18.src.rpm

Thank you.

Another note. You should be able to remove the %{gemdir} and %{geminstir} macro definitions if you follow the new packaging guidelines [1] and use rubygems-devel package, which provide these macros (better to say the "underscore" alternatives). There are also helpful macros you can use in the %file section, etc.


[1] https://fedoraproject.org/wiki/Packaging:Ruby

(In reply to comment #9)
> Thank you.
> 
> Another note. You should be able to remove the %{gemdir} and %{geminstir} macro
> definitions if you follow the new packaging guidelines [1] and use
> rubygems-devel package, which provide these macros (better to say the
> "underscore" alternatives). There are also helpful macros you can use in the
> %file section, etc.
> 
> 
> [1] https://fedoraproject.org/wiki/Packaging:Ruby

Thank you. I've incorporated those changes as well.

Updated SPEC: http://mcpierce.fedorapeople.org/rpms/rubygem-qpid.spec
Updated SRPM: http://mcpierce.fedorapeople.org/rpms/rubygem-qpid-0.16.0-4.fc18.src.rpm

It's been a week since the last update. Can we continue (and hopefully complete) the package review?

I do have one question: we're likely going to rename the gem from qpid to qpid-messaging due to the owner of the original qpid gem on RubyForge.org not following through with his promise to release that name to our project. How will this affect the review process?

Ok, going to finish what I was already started. Taking the review.

(In reply to comment #11)
> I do have one question: we're likely going to rename the gem from qpid to
> qpid-messaging due to the owner of the original qpid gem on RubyForge.org not
> following through with his promise to release that name to our project. How
> will this affect the review process?

Well, during review, it is easy process, you just need to udpate the .spec file ant the ticket subject and that is it. However, I would prefer to see the gem with the current name on rubygems.org (I know, I know, I'm silent ;))

* License
  - The LICENSE file contains Apache license and Boost license. I am wondering
    what is purpose of the Boost license? Shouldn't it mentioned in license
    field? Or removed from the LICENSE file?

* Test suite
  - I'd love to see test suite executed in %check section.

* Binary extension placement.
  - Please move the .so file into the correct placement according to the
    guidelines:

# If there are C extensions, mv them to the extdir.
# You should replace REQUIRE_PATHS with the first value of the require_paths field in
# the gemspec file.  It will typically be either "lib" or "ext".  For instance:
#  s.require_paths = ["lib"] 
mkdir -p %{buildroot}%{gem_extdir}/REQUIRE_PATHS
mv %{buildroot}%{gem_instdir}/REQUIRE_PATHS/shared_object.so %{buildroot}%{gem_extdir}/REQUIRE_PATHS/

* Use macros provided by rubygems-devel
  - There is more macros provided by rubygems-devel, such as %{gem_libdir},
    %{gem_cache}, %{gem_spec}, %{gem_docdir} and %{gem_extdir}. It would be nice
    to use them in %files section and also elsewhere where suitable.

* Move some unnecessary files into -doc subpackage
  - the following files/dirs are not mandatory for runtime and should be moved
    into the -doc subpackage:

%{gem_instdir}/examples
%{gem_instdir}/features
%{gem_instdir}/spec
%{gem_instdir}/Rakefile
%doc %{gem_instdir}/TODO

* The package does not work.
  - I tried the most trivial test case, which unfortunately failed:

# ruby -e 'require "qpid"'
/usr/share/rubygems/rubygems/custom_require.rb:36:in `require': /usr/share/gems/gems/qpid-0.16.0/lib/qpid/encoding.rb:48: syntax error, unexpected ':', expecting keyword_then or ',' or ';' or '\n' (SyntaxError)
        when "amqp/map":  Cqpid.decodeMap message.message_impl
                        ^
/usr/share/gems/gems/qpid-0.16.0/lib/qpid/encoding.rb:49: syntax error, unexpected keyword_when, expecting keyword_end
        when "amqp/list": Cqpid.decodeList message.message_impl
            ^
/usr/share/gems/gems/qpid-0.16.0/lib/qpid/encoding.rb:49: syntax error, unexpected ':', expecting keyword_end
        when "amqp/list": Cqpid.decodeList message.message_impl
                         ^
/usr/share/gems/gems/qpid-0.16.0/lib/qpid/encoding.rb:57: syntax error, unexpected keyword_end, expecting $end
	from /usr/share/rubygems/rubygems/custom_require.rb:36:in `require'
	from /usr/share/gems/gems/qpid-0.16.0/lib/qpid.rb:23:in `<top (required)>'
	from /usr/share/rubygems/rubygems/custom_require.rb:60:in `require'
	from /usr/share/rubygems/rubygems/custom_require.rb:60:in `rescue in require'
	from /usr/share/rubygems/rubygems/custom_require.rb:35:in `require'
	from -e:1:in `<main>'

(In reply to comment #12)
> Ok, going to finish what I was already started. Taking the review.
> 
> (In reply to comment #11)
> > I do have one question: we're likely going to rename the gem from qpid to
> > qpid-messaging due to the owner of the original qpid gem on RubyForge.org not
> > following through with his promise to release that name to our project. How
> > will this affect the review process?
> 
> Well, during review, it is easy process, you just need to udpate the .spec file
> ant the ticket subject and that is it. However, I would prefer to see the gem
> with the current name on rubygems.org (I know, I know, I'm silent ;))

Our lead said we'll hold off on renaming the package for now, giving the previous name owner time to make his choice.

> * License
>   - The LICENSE file contains Apache license and Boost license. I am wondering
>     what is purpose of the Boost license? Shouldn't it mentioned in license
>     field? Or removed from the LICENSE file?

The underlying C++ code uses (but doesn't expose) the Boost libraries. I've added Boost to the list of licenses in the specifile.
sy
> * Test suite
>   - I'd love to see test suite executed in %check section.

The feature tests (rake test:features) require a local Qpid messaging broker to be running, so they are not invoked. A requirement I wouldn't think we would push onto koji.

The spec does run the Rspec tests using test:spec.

> * Binary extension placement.
>   - Please move the .so file into the correct placement according to the
>     guidelines:
> 
> # If there are C extensions, mv them to the extdir.
> # You should replace REQUIRE_PATHS with the first value of the require_paths
> field in
> # the gemspec file.  It will typically be either "lib" or "ext".  For instance:
> #  s.require_paths = ["lib"] 
> mkdir -p %{buildroot}%{gem_extdir}/REQUIRE_PATHS
> mv %{buildroot}%{gem_instdir}/REQUIRE_PATHS/shared_object.so
> %{buildroot}%{gem_extdir}/REQUIRE_PATHS/
> 
> * Use macros provided by rubygems-devel
>   - There is more macros provided by rubygems-devel, such as %{gem_libdir},
>     %{gem_cache}, %{gem_spec}, %{gem_docdir} and %{gem_extdir}. It would be
> nice
>     to use them in %files section and also elsewhere where suitable.

I think this is done correctly. It's a little confusing to me since I can't find a decent example to work from, but I'm confortable with what I have so far.

> 
> * Move some unnecessary files into -doc subpackage
>   - the following files/dirs are not mandatory for runtime and should be moved
>     into the -doc subpackage:
> 
> %{gem_instdir}/examples
> %{gem_instdir}/features
> %{gem_instdir}/spec
> %{gem_instdir}/Rakefile
> %doc %{gem_instdir}/TODO

Done.

> * The package does not work.
>   - I tried the most trivial test case, which unfortunately failed:
> 
> # ruby -e 'require "qpid"'
> /usr/share/rubygems/rubygems/custom_require.rb:36:in `require':
> /usr/share/gems/gems/qpid-0.16.0/lib/qpid/encoding.rb:48: syntax error,
> unexpected ':', expecting keyword_then or ',' or ';' or '\n' (SyntaxError)
>         when "amqp/map":  Cqpid.decodeMap message.message_impl
>                         ^
> /usr/share/gems/gems/qpid-0.16.0/lib/qpid/encoding.rb:49: syntax error,
> unexpected keyword_when, expecting keyword_end
>         when "amqp/list": Cqpid.decodeList message.message_impl
>             ^
> /usr/share/gems/gems/qpid-0.16.0/lib/qpid/encoding.rb:49: syntax error,
> unexpected ':', expecting keyword_end
>         when "amqp/list": Cqpid.decodeList message.message_impl
>                          ^
> /usr/share/gems/gems/qpid-0.16.0/lib/qpid/encoding.rb:57: syntax error,
> unexpected keyword_end, expecting $end
>  from /usr/share/rubygems/rubygems/custom_require.rb:36:in `require'
>  from /usr/share/gems/gems/qpid-0.16.0/lib/qpid.rb:23:in `<top (required)>'
>  from /usr/share/rubygems/rubygems/custom_require.rb:60:in `require'
>  from /usr/share/rubygems/rubygems/custom_require.rb:60:in `rescue in require'
>  from /usr/share/rubygems/rubygems/custom_require.rb:35:in `require'
>  from -e:1:in `<main>'

Ah, that was a change that didn't make it into the initial gemfile. The gemfile now includes that change as well.

Updated SPEC: http://mcpierce.fedorapeople.org/rpms/rubygem-qpid.spec
Updated SRPM: http://mcpierce.fedorapeople.org/rpms/rubygem-qpid-0.16.0-5.fc18.src.rpm

(In reply to comment #13)
> (In reply to comment #12)
> > Ok, going to finish what I was already started. Taking the review.
> > 
> > (In reply to comment #11)
> > > I do have one question: we're likely going to rename the gem from qpid to
> > > qpid-messaging due to the owner of the original qpid gem on RubyForge.org not
> > > following through with his promise to release that name to our project. How
> > > will this affect the review process?
> > 
> > Well, during review, it is easy process, you just need to udpate the .spec file
> > ant the ticket subject and that is it. However, I would prefer to see the gem
> > with the current name on rubygems.org (I know, I know, I'm silent ;))
> 
> Our lead said we'll hold off on renaming the package for now, giving the
> previous name owner time to make his choice.

Yes, that is sensitive. I agree.
 
> > * License
> >   - The LICENSE file contains Apache license and Boost license. I am wondering
> >     what is purpose of the Boost license? Shouldn't it mentioned in license
> >     field? Or removed from the LICENSE file?
> 
> The underlying C++ code uses (but doesn't expose) the Boost libraries. I've
> added Boost to the list of licenses in the specifile.

I'm not sure I understand it. What does it mean "uses". Is there copied some Boost code? I cannot find any reference to boost in cqpid.cpp

> sy
> > * Test suite
> >   - I'd love to see test suite executed in %check section.
> 
> The feature tests (rake test:features) require a local Qpid messaging broker to
> be running, so they are not invoked. A requirement I wouldn't think we would
> push onto koji.
> 
> The spec does run the Rspec tests using test:spec.

Is it possible to avoid usage of Rake and run the test suite using plain "rspec spec"? Rake tends to bring in more dependencies then necessary, therefore it is usually good idea to avoid its usage.

> 
> > * Binary extension placement.
> >   - Please move the .so file into the correct placement according to the
> >     guidelines:
> > 
> > # If there are C extensions, mv them to the extdir.
> > # You should replace REQUIRE_PATHS with the first value of the require_paths
> > field in
> > # the gemspec file.  It will typically be either "lib" or "ext".  For instance:
> > #  s.require_paths = ["lib"] 
> > mkdir -p %{buildroot}%{gem_extdir}/REQUIRE_PATHS
> > mv %{buildroot}%{gem_instdir}/REQUIRE_PATHS/shared_object.so
> > %{buildroot}%{gem_extdir}/REQUIRE_PATHS/
> > 
> > * Use macros provided by rubygems-devel
> >   - There is more macros provided by rubygems-devel, such as %{gem_libdir},
> >     %{gem_cache}, %{gem_spec}, %{gem_docdir} and %{gem_extdir}. It would be
> > nice
> >     to use them in %files section and also elsewhere where suitable.
> 
> I think this is done correctly. It's a little confusing to me since I can't
> find a decent example to work from, but I'm confortable with what I have so
> far.

Not a big deal as long as it works, however you can still benefit from %{gem_cache} and  %{gem_doc} macros. Recent gem2rpm can generate basic %file section with the macros for you:

gem2rpm -t fedora-17-rawhide qpid-0.16.0.gem

If you are already on F17, you can omit the "-t fedora-17-rawhide" parameter.  Btw we typically %exclude the %{gem_cache}.


> 
> > 
> > * Move some unnecessary files into -doc subpackage
> >   - the following files/dirs are not mandatory for runtime and should be moved
> >     into the -doc subpackage:
> > 
> > %{gem_instdir}/examples
> > %{gem_instdir}/features
> > %{gem_instdir}/spec
> > %{gem_instdir}/Rakefile
> > %doc %{gem_instdir}/TODO
> 
> Done.
> 
> > * The package does not work.
> >   - I tried the most trivial test case, which unfortunately failed:
> > 
> > # ruby -e 'require "qpid"'
> > /usr/share/rubygems/rubygems/custom_require.rb:36:in `require':
> > /usr/share/gems/gems/qpid-0.16.0/lib/qpid/encoding.rb:48: syntax error,
> > unexpected ':', expecting keyword_then or ',' or ';' or '\n' (SyntaxError)
> >         when "amqp/map":  Cqpid.decodeMap message.message_impl
> >                         ^
> > /usr/share/gems/gems/qpid-0.16.0/lib/qpid/encoding.rb:49: syntax error,
> > unexpected keyword_when, expecting keyword_end
> >         when "amqp/list": Cqpid.decodeList message.message_impl
> >             ^
> > /usr/share/gems/gems/qpid-0.16.0/lib/qpid/encoding.rb:49: syntax error,
> > unexpected ':', expecting keyword_end
> >         when "amqp/list": Cqpid.decodeList message.message_impl
> >                          ^
> > /usr/share/gems/gems/qpid-0.16.0/lib/qpid/encoding.rb:57: syntax error,
> > unexpected keyword_end, expecting $end
> >  from /usr/share/rubygems/rubygems/custom_require.rb:36:in `require'
> >  from /usr/share/gems/gems/qpid-0.16.0/lib/qpid.rb:23:in `<top (required)>'
> >  from /usr/share/rubygems/rubygems/custom_require.rb:60:in `require'
> >  from /usr/share/rubygems/rubygems/custom_require.rb:60:in `rescue in require'
> >  from /usr/share/rubygems/rubygems/custom_require.rb:35:in `require'
> >  from -e:1:in `<main>'
> 
> Ah, that was a change that didn't make it into the initial gemfile. The gemfile
> now includes that change as well.

It seems it works, at least it is possible to require the qpid (btw it would be worth of bumping the gem version to avoiding confusion)
 
> Updated SPEC: http://mcpierce.fedorapeople.org/rpms/rubygem-qpid.spec
> Updated SRPM:
> http://mcpierce.fedorapeople.org/rpms/rubygem-qpid-0.16.0-5.fc18.src.rpm

* $REQUIRE_PATHS
  - The REQUIRE_PATHS from guidelines is actually just placeholder you should
    replace by correct directory names. Please remove all references to
    REQUIRE_PATHS in your .spec file. The guidelines seems to be confusing.
    I already asked clarification [1]

* %{gem_instdir}/ext
  - Do you like to keep the directory around? Since it is not required in
    runtime, it could be moved into -doc subpackage or even dropped.

* Requires: rubygem-cucumber
  - I am bit surprised to see this as runtime dependency. Is it really
    required? And since you don't execute the cucumber test suite, it should
    not be require even in build time.


[1] https://fedorahosted.org/fpc/ticket/168

(In reply to comment #14)
> > > * License
> > >   - The LICENSE file contains Apache license and Boost license. I am wondering
> > >     what is purpose of the Boost license? Shouldn't it mentioned in license
> > >     field? Or removed from the LICENSE file?
> > 
> > The underlying C++ code uses (but doesn't expose) the Boost libraries. I've
> > added Boost to the list of licenses in the specifile.
> 
> I'm not sure I understand it. What does it mean "uses". Is there copied some
> Boost code? I cannot find any reference to boost in cqpid.cpp

Yeah, you're right. The C++ code depends on, as opposed to includes, the Boost libraries and code. It makes more sense to remove the Boost license statement from the Gem's copy of the LICENSE file rather than include it. I'll put that on the list of things to be handled as a part of our next release (the gem artifact's locked at this point).

> > > * Test suite
> > >   - I'd love to see test suite executed in %check section.
> > 
> > The feature tests (rake test:features) require a local Qpid messaging broker to
> > be running, so they are not invoked. A requirement I wouldn't think we would
> > push onto koji.
> > 
> > The spec does run the Rspec tests using test:spec.
> 
> Is it possible to avoid usage of Rake and run the test suite using plain "rspec
> spec"? Rake tends to bring in more dependencies then necessary, therefore it is
> usually good idea to avoid its usage.

Very good point. I've made this change, though will need to be some changes for previous versions of Rspec since the command line changed. I've added a few globals for running Rspec that I'll set for F16.

> > > * Binary extension placement.
> > >   - Please move the .so file into the correct placement according to the
> > >     guidelines:
> > > 
> > > # If there are C extensions, mv them to the extdir.
> > > # You should replace REQUIRE_PATHS with the first value of the require_paths
> > > field in
> > > # the gemspec file.  It will typically be either "lib" or "ext".  For instance:
> > > #  s.require_paths = ["lib"] 
> > > mkdir -p %{buildroot}%{gem_extdir}/REQUIRE_PATHS
> > > mv %{buildroot}%{gem_instdir}/REQUIRE_PATHS/shared_object.so
> > > %{buildroot}%{gem_extdir}/REQUIRE_PATHS/
> > > 
> > > * Use macros provided by rubygems-devel
> > >   - There is more macros provided by rubygems-devel, such as %{gem_libdir},
> > >     %{gem_cache}, %{gem_spec}, %{gem_docdir} and %{gem_extdir}. It would be
> > > nice
> > >     to use them in %files section and also elsewhere where suitable.
> > 
> > I think this is done correctly. It's a little confusing to me since I can't
> > find a decent example to work from, but I'm confortable with what I have so
> > far.
> 
> Not a big deal as long as it works, however you can still benefit from
> %{gem_cache} and  %{gem_doc} macros. Recent gem2rpm can generate basic %file
> section with the macros for you:
> 
> gem2rpm -t fedora-17-rawhide qpid-0.16.0.gem
> 
> If you are already on F17, you can omit the "-t fedora-17-rawhide" parameter. 
> Btw we typically %exclude the %{gem_cache}.

Thanks for the tip. I generated a temporary spec and used it to update the files section.

> > > * The package does not work.
> > >   - I tried the most trivial test case, which unfortunately failed:
> > > 
> > > # ruby -e 'require "qpid"'
> > > /usr/share/rubygems/rubygems/custom_require.rb:36:in `require':
> > > /usr/share/gems/gems/qpid-0.16.0/lib/qpid/encoding.rb:48: syntax error,
> > > unexpected ':', expecting keyword_then or ',' or ';' or '\n' (SyntaxError)
> > >         when "amqp/map":  Cqpid.decodeMap message.message_impl
> > >                         ^
> > > /usr/share/gems/gems/qpid-0.16.0/lib/qpid/encoding.rb:49: syntax error,
> > > unexpected keyword_when, expecting keyword_end
> > >         when "amqp/list": Cqpid.decodeList message.message_impl
> > >             ^
> > > /usr/share/gems/gems/qpid-0.16.0/lib/qpid/encoding.rb:49: syntax error,
> > > unexpected ':', expecting keyword_end
> > >         when "amqp/list": Cqpid.decodeList message.message_impl
> > >                          ^
> > > /usr/share/gems/gems/qpid-0.16.0/lib/qpid/encoding.rb:57: syntax error,
> > > unexpected keyword_end, expecting $end
> > >  from /usr/share/rubygems/rubygems/custom_require.rb:36:in `require'
> > >  from /usr/share/gems/gems/qpid-0.16.0/lib/qpid.rb:23:in `<top (required)>'
> > >  from /usr/share/rubygems/rubygems/custom_require.rb:60:in `require'
> > >  from /usr/share/rubygems/rubygems/custom_require.rb:60:in `rescue in require'
> > >  from /usr/share/rubygems/rubygems/custom_require.rb:35:in `require'
> > >  from -e:1:in `<main>'
> > 
> > Ah, that was a change that didn't make it into the initial gemfile. The gemfile
> > now includes that change as well.
> 
> It seems it works, at least it is possible to require the qpid (btw it would be
> worth of bumping the gem version to avoiding confusion)
> 
> > Updated SPEC: http://mcpierce.fedorapeople.org/rpms/rubygem-qpid.spec
> > Updated SRPM:
> > http://mcpierce.fedorapeople.org/rpms/rubygem-qpid-0.16.0-5.fc18.src.rpm
> 
> * $REQUIRE_PATHS
>   - The REQUIRE_PATHS from guidelines is actually just placeholder you should
>     replace by correct directory names. Please remove all references to
>     REQUIRE_PATHS in your .spec file. The guidelines seems to be confusing.
>     I already asked clarification [1]

Thanks for that. I've removed those references.

> * %{gem_instdir}/ext
>   - Do you like to keep the directory around? Since it is not required in
>     runtime, it could be moved into -doc subpackage or even dropped.

No, it's not necessary. I've removed it in the updated specfile.

> * Requires: rubygem-cucumber
>   - I am bit surprised to see this as runtime dependency. Is it really
>     required? And since you don't execute the cucumber test suite, it should
>     not be require even in build time.

Removed that dependency, as well as rake and rake-compiler since those aren't being directly used. Users who want to run the tests can install them but I won't force them to with the package.

Updated SPEC: http://mcpierce.fedorapeople.org/rpms/rubygem-qpid.spec
Updated SRPM: http://mcpierce.fedorapeople.org/rpms/rubygem-qpid-0.16.0-6.fc18.src.rpm

It gets lengthy, so I will start over.

* Test suite
  - The current approach is OK and cannot be showstopper for a review. 
    However, from my experience, if I may suggest, I would go following way:

pushd .%{gem_instdir}
%{rspec}
popd

  - This is the easiest way IMO. I tried it, so you can just copy/paste.
  - The main difference is that the test suite is executed from %{builddir}
    instead of %{buildroot}. So the gem is still in its original state, 
    including the binaries on the original places, so you don't have to fiddle
    too much with include paths (-I). Of course there are also disadvantages,
    but the advantages wins in this case IMO.
  - Hint: May be it would be worth of commenting the purpose of %{rspec} and
    %{rspec_format} macros. I was already confused by them, since I did not read
    carefully enough your previous comment :)

* %{gem_libname} macro is not used anywhere.


Overall, the package looks good in current state. I will approve it as soon as:

1) The gem will be available on rubygems.org
2) The LICENSE file will be fixed, or if there will be some note in the .spec
   file (you may have some ticket for that for example)

All of these things are now addressed in the specfile:

New SPEC: http://mcpierce.fedorapeople.org/rpms/rubygem-qpid.spec
New SRPM: http://mcpierce.fedorapeople.org/rpms/rubygem-qpid-0.16.0-7.fc17.src.rpm

Updated with additional notes about the official location of the gem and the issue with the existing qpid gem on rubygems.org:

New SPEC: http://mcpierce.fedorapeople.org/rpms/rubygem-qpid.spec
New SRPM: http://mcpierce.fedorapeople.org/rpms/rubygem-qpid-0.16.0-8.fc17.src.rpm

Ok, thank you. I APPROVE the package for F17. You can request repository now.

But please check the F16 stuff, since it should be done according to older Ruby packaging guidelines [1].

[1] https://fedoraproject.org/wiki/Packaging:Old_Ruby#Ruby_Gem_with_extension_libraries_written_in_C

Thank you, Vit.

New Package SCM Request
=======================
Package Name: rubygem-qpid
Short Description: Ruby bindings for the Qpid messaging framework 
Owners: mcpierce
Branches: f16 f17
InitialCC:

Git done (by process-git-requests).

Forgot to close this now that the review is completed.