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 #14 from Vít Ondruch <vondruch@xxxxxxxxxx> 2012-04-26 06:03:17 EDT --- (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 -- 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