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 #15 from Darryl L. Pierce <dpierce@xxxxxxxxxx> 2012-04-26 08:59:40 EDT --- (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 -- 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