Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=810926 --- Comment #13 from Darryl L. Pierce <dpierce@xxxxxxxxxx> 2012-04-25 11:32:20 EDT --- (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 -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review