https://bugzilla.redhat.com/show_bug.cgi?id=1117025 --- Comment #2 from Ken Dreyer <ktdreyer@xxxxxxxxxxxx> --- (In reply to František Dvořák from comment #1) > Issues or changes for consideration: > > 1) new version of joiner is available (0.3.3) Thanks, I've updated the gem to 0.3.3. * Fri Oct 10 2014 Ken Dreyer <ktdreyer@xxxxxxxxxxxx> - 0.3.3-1 - Update to joiner 0.3.3 (RHBZ #1117025) - Use %%license macro (RHBZ #1117025) Spec URL: http://ktdreyer.fedorapeople.org/reviews/rubygem-joiner.spec SRPM URL: http://ktdreyer.fedorapeople.org/reviews/rubygem-joiner-0.3.3-1.fc22.src.rpm Exact changes in Git: https://fedorapeople.org/cgit/ktdreyer/public_git/rubygem-joiner.git/commit/?id=8f874b22823e7912fd2ed550810be24213c48c69 F22 scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=7825913 > 2) (cosmetic) you can consider using unified way of excluding files > (replacing "rm .%{gem_instdir}/%{gem_name}.gemspec" by %exclude macro?) In the case of rm %{gem_name}.gemspec, I consider that to be a bug in the upstream project. I don't see a point to shipping the gemspec file in the gem itself. Plenty of gems avoid doing this (eg rugged). See https://lists.fedoraproject.org/pipermail/ruby-sig/2013-December/001471.html for a discussion on this (that unfortunately didn't reach resolution.) > 3) (cosmetic) you can consider using %license macro for license file Thanks for pointing this out. When this guideline first came out I didn't see a backwards-compatible way to introduce this, but after reading https://fedorahosted.org/fpc/ticket/411#comment:11 I see there is an easy solution for backwards compatibility. I've changed this in my package. > 4) (future work) I think combustion gem requires bundler. Removing bundler > from spec_helper.rb may break things, or there will be needed some way of > disabling bundler in combustion? > > But it is not used now anyway, so it is not needed neither for build nor > passing the review. :-) Yeah, I'm typically able to strip out Bundler where possible. I'll be sure to check if we need to do that with combustion. > 5) (question) there is not used "require 'active_record'" anywere in joiner > sources, but I guess it's OK (developers using joiner will require rails or > activerecord anyway?) Interesting, I guess upstream thinks it's fine to just require it in the gemspec and not in the actual library. In the RPM case, since we Require: rubygem(activerecord) I think it's ok, but your point is valid that upstream probably needs to think about the non-Bundler case. When you were testing in your chroot, did rubygem-activerecord get installed there? Or did you install the package with --nodeps? It's weird that it would break like that for you, since the dependency is in the RPM. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review