Spec URL: https://fedorapeople.org/~domcleal/reviews/rubygem-gettext-setup/rubygem-gettext-setup.spec SRPM URL: https://fedorapeople.org/~domcleal/reviews/rubygem-gettext-setup/rubygem-gettext-setup-0.24-1.fc27.src.rpm Description: A gem to ease internationalization with fast_gettext, used by Puppet and related projects Fedora Account System Username: domcleal
Spec URL: https://fedorapeople.org/~domcleal/reviews/rubygem-gettext-setup/rubygem-gettext-setup.spec SRPM URL: https://fedorapeople.org/~domcleal/reviews/rubygem-gettext-setup/rubygem-gettext-setup-0.25-1.fc27.src.rpm Updated to 0.25 to drop the patch carried in the first version.
I'll take a look ...
* Group is not needed anymore - The Group tag should not be used for F26+ [1]. * Unnecessary build dependencies - If I am not mistaken, rubygem(rspec) will pull in all other rspec-* packages. Are these BR necessary? - I am not sure about webmock and rack-test. I removed the dependency, searched for some requires but found nothing. Are they needed? - I don't think you need simplecov for anything, since we don't really care about code coverage. I'd get rid of the dependency by st. like: ~~~ sed -i "/require 'simplecov'/ s/^/#/" spec/spec_helper.rb sed -i "/SimpleCov.start/,/^end$/ s/^/#/" spec/spec_helper.rb ~~~ * Use executable instead of package dependencies. - This is jut minor nit and I'' leave it up to you, since it is more or less about style, but if the package requires some executables, such as 'git' or 'msgcmp' in this case, I prefer to require directly these executables, instead of the packages. E.g.: ~~~ BuildRequires: %{_bindir}/msgcmp BuildRequires: %{_bindir}/git ~~~ Otherwise, the package looks good => APPROVE [1] https://fedoraproject.org/w/index.php?title=Packaging:Guidelines&diff=487247&oldid=487238
Thanks very much for the review Vít, I agree with all of your comments. I updated the spec file and will request the package now. Spec URL: https://fedorapeople.org/~domcleal/reviews/rubygem-gettext-setup/rubygem-gettext-setup.spec SRPM URL: https://fedorapeople.org/~domcleal/reviews/rubygem-gettext-setup/rubygem-gettext-setup-0.25-2.fc27.src.rpm
(In reply to Dominic Cleal from comment #4) > I updated the spec file LGTM. Thx.
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/rubygem-gettext-setup
rubygem-gettext-setup-0.25-2.fc27 built.