Bug 847457
Summary: | Review Request: rubygem-transaction-simple - Simple object transaction support for Ruby | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Miroslav Suchý <msuchy> |
Component: | Package Review | Assignee: | Vít Ondruch <vondruch> |
Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | notting, package-review, vondruch |
Target Milestone: | --- | Flags: | vondruch:
fedora-review+
gwync: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2012-08-27 03:26:59 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: |
Description
Miroslav Suchý
2012-08-11 16:10:31 UTC
I'll take it for a review. * Version 1.4.0.2 available - Is there any particular reason why not package the most recent version? * Use %global in place of %define [1] * Do not depend on rubygem(hoe) - This is just development dependency. You don't need it for runtime and there is only tiny chance that you would need it for build time. * Drop s.cert_chain in .gemspec - I would say that better than changing the s.cert_chain would be to fallback to default, i.e. drop the line: sed -i '/s.cert_chain = nil/ d' %{gem_name}.gemspec * Leave the History.txt Install.txt Licence.txt Readme.txt Manifest.txt on original place - Please consider to keep the files on the original place. I am not sure what would be the benefit to move them into different place * Move documentation into -doc subapackage - Please consider move of History.txt, Install.txt, Readme.txt and Manifest.txt files into -doc subpackage, since they are not required by runtime. * Exclude %{gem_cache} - We discussed it before in different review, but I'd like to mention it here, although I don't expect you will do that ;) * export CONFIGURE_ARGS not needed - Since this is not binary gem, the export CONFIGURE_ARGS could be omitted. * Mark %{gem_docdir} as as %doc * Please fix the executable bit - rpmlint complains about following files: rubygem-transaction-simple-doc.noarch: E: non-executable-script /usr/share/gems/gems/transaction-simple-1.4.0/test/test_transaction_simple.rb 0644L /usr/bin/env rubygem-transaction-simple-doc.noarch: E: non-executable-script /usr/share/gems/gems/transaction-simple-1.4.0/test/test_all.rb 0644L /usr/bin/env rubygem-transaction-simple-doc.noarch: E: non-executable-script /usr/share/gems/gems/transaction-simple-1.4.0/test/test_transaction_simple_group.rb 0644L /usr/bin/env rubygem-transaction-simple-doc.noarch: E: non-executable-script /usr/share/gems/gems/transaction-simple-1.4.0/test/test_transaction_simple_threadsafe.rb 0644L /usr/bin/env rubygem-transaction-simple-doc.noarch: E: non-executable-script /usr/share/gems/gems/transaction-simple-1.4.0/test/test_broken_graph.rb 0644L /usr/bin/env rubygem-transaction-simple-doc.noarch: E: non-executable-script /usr/share/gems/gems/transaction-simple-1.4.0/Rakefile 0644L /usr/bin/env [1] https://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define > Is there any particular reason why not package the most recent version? Yes. 1.4.0.2 depends on hoe ~> 3.0 which is not in EPEL and F16. My plan is to later upgrade to 1.4.0.2 in rawhide. > Do not depend on rubygem(hoe) indeed it is used for creating tar.gz. And it builds without it. Removing. Hmm... that may solve previous item. > Use %global in place of %define fixed > Drop s.cert_chain in .gemspec dropped > * Leave the History.txt Install.txt Licence.txt Readme.txt Manifest.txt on > original place I do not agree with you. > Move documentation into -doc subapackage Moved > Exclude %{gem_cache} Surprise! As long as it is in guidlines, I decided to obey it. Untill I succed in Guidelines change. > export CONFIGURE_ARGS not needed fixed Spec URL: http://miroslav.suchy.cz/fedora/rubygem-transaction-simple/rubygem-transaction-simple.spec SRC.RPM: http://miroslav.suchy.cz/fedora/rubygem-transaction-simple/rubygem-transaction-simple-1.4.0.2-2.fc17.src.rpm (In reply to comment #3) > > Is there any particular reason why not package the most recent version? > Yes. 1.4.0.2 depends on hoe ~> 3.0 which is not in EPEL and F16. My plan is > to later upgrade to 1.4.0.2 in rawhide. > > > Do not depend on rubygem(hoe) > indeed it is used for creating tar.gz. And it builds without it. > Removing. Hmm... that may solve previous item. :D > > * Leave the History.txt Install.txt Licence.txt Readme.txt Manifest.txt on > > original place > I do not agree with you. Roger * dot files - You remove one file in %build section while the other in %install, is it intentional? - I would put %exclude %{gem_instdir}.* into %files section instead - btw you should use the %{gem_instdir} macro in the rm in %build section. * URL - Seems that the homepage URL was changed to http://trans-simple.rubyforge.org/ * rpmlint: - You should probably remove the chmods a+x in %install section and use chmod a-x test/test_broken_graph.rb rubygem-transaction-simple-doc.noarch: E: script-without-shebang /usr/share/gems/gems/transaction-simple-1.4.0.2/test/test_transaction_simple_group.rb rubygem-transaction-simple-doc.noarch: E: script-without-shebang /usr/share/gems/gems/transaction-simple-1.4.0.2/test/test_transaction_simple.rb rubygem-transaction-simple-doc.noarch: E: script-without-shebang /usr/share/gems/gems/transaction-simple-1.4.0.2/Rakefile rubygem-transaction-simple-doc.noarch: E: script-without-shebang /usr/share/gems/gems/transaction-simple-1.4.0.2/test/test_broken_graph.rb Otherwise the package looks good, so I APPROVE it, but please consider to fix the first two nits and definitely fix the rpmlint errors prior importing into SCM. > - You remove one file in %build section while the other in %install, is it > intentional? No, it was completly random :) Moved to %install > I would put %exclude %{gem_instdir}.* into %files section instead I prefer that rm (for no obvious reason). > btw you should use the %{gem_instdir} macro in the rm in %build section. fixed > Seems that the homepage URL was changed to indeed, fixed > You should probably remove the chmods a+x in %install section and use > chmod a-x test/test_broken_graph.rb Ah, they change it from last version. Fixed. Thanks for review. New Package SCM Request ======================= Package Name: rubygem-transaction-simple Short Description: Simple object transaction support for Ruby Owners: msuchy Branches: F-18, F-17, F-16, EL-6 InitialCC: Git done (by process-git-requests). rubygem-transaction-simple-1.4.0.2-3.fc17 has been submitted as an update for Fedora 17. https://admin.fedoraproject.org/updates/rubygem-transaction-simple-1.4.0.2-3.fc17 rubygem-transaction-simple-1.4.0.2-3.fc16 has been submitted as an update for Fedora 16. https://admin.fedoraproject.org/updates/rubygem-transaction-simple-1.4.0.2-3.fc16 rubygem-transaction-simple-1.4.0.2-3.el6 has been submitted as an update for Fedora EPEL 6. https://admin.fedoraproject.org/updates/rubygem-transaction-simple-1.4.0.2-3.el6 rubygem-transaction-simple-1.4.0.2-3.fc18 has been submitted as an update for Fedora 18. https://admin.fedoraproject.org/updates/rubygem-transaction-simple-1.4.0.2-3.fc18 rubygem-transaction-simple-1.4.0.2-3.fc18 has been pushed to the Fedora 18 testing repository. rubygem-transaction-simple-1.4.0.2-3.fc16 has been pushed to the Fedora 16 stable repository. rubygem-transaction-simple-1.4.0.2-3.fc17 has been pushed to the Fedora 17 stable repository. rubygem-transaction-simple-1.4.0.2-3.el6 has been pushed to the Fedora EPEL 6 stable repository. rubygem-transaction-simple-1.4.0.2-3.fc18 has been pushed to the Fedora 18 stable repository. |