[Bug 1052283] Review Request: rubygem-more_core_extensions - Set of core extensions beyond those provided by ActiveSupport

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=1052283



--- Comment #5 from Mo Morsi <mmorsi@xxxxxxxxxx> ---
Thanks.

Final updated package:

Spec: https://mmorsi.fedorapeople.org/staging/rubygem-more_core_extensions.spec
SRPM:
https://mmorsi.fedorapeople.org/staging/rubygem-more_core_extensions-1.2.0-2.fc20.src.rpm
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=8332003

New Package SCM Request
=======================
Package Name: rubygem-more_core_extensions
Short Description: Set of core extensions beyond those provided by
ActiveSupport
Upstream URL: https://github.com/ManageIQ/more_core_extensions
Owners: mmorsi
Branches: f21 f20
InitialCC: 

(In reply to Ken Dreyer from comment #3)
> Hi Mo,
> 
> Please accept my apologies at taking so long to get back on this. Here's my
> review. Package is APPROVED. 
> 
> The following comments are not blockers, but suggestions:
> 
> - You can strip out the boilerplate gem2rpm comment about
> more_core_extensions-1.2.0.gem at the top of the file, since this is going
> to go stale.

Done

> 
> - rpmlint doesn't like the length of the Summary field. For some reason I
> have a vague memory that the Summary field is not supposed to contain the
> name of the package... but I can't find the guideline for that at the
> moment. At any rate, you could shorten the Summary to simply "Set of core
> extensions beyond those provided by ActiveSupport". That should fix the
> "summary-to-long" rpmlint error. (This could be fixed in the gemspec
> upstream as well :)

Done

> 
> - No need to BuildRequires: ruby when you've already got BuildRequires:
> ruby(release). BR: ruby is only for gems that can only work on MRI.
> 

Done

> - The coveralls and rspec BRs are commented out, and I'm not sure why they
> are commented. It seems like you should be able to run the test suite,
> right? The %check section is blank (it's just the default boilerplate from
> gem2rpm).
> 

There are a few broken specs reported upstream and seems to be a few more when
run against the latest gem stack in rawhide. Leaving these as ommitted for the
time being until the situation can be resolved.


> - With the changes in the Fedora 21 Ruby Packaging guidelines, you don't
> have to provide explicit Requires or Provides any more for rubygem packages.
> For backwards compatibility with Fedora 20 and RHEL 7, you can wrap your
> Requires and Provides like so:
> 
>   %if 0%{?fc19} || 0%{?fc20} || 0%{?el7}
>   Requires: ruby(release)
>   Requires: ruby(rubygems)
>   Requires: rubygem(activesupport) > 3.2
>   %endif
> 
>   %if 0%{?fc19} || 0%{?fc20} || 0%{?el7}
>   Provides: rubygem(%{gem_name}) = %{version}
>   %endif
> 
> And then just strip out each dist conditional as Fedora 19 goes EOL, then
> Fedora 20, etc.

Done

> 
> - The %license macro has not yet been adopted as a requirement by the
> Packaging Committee, but you can start to use it now, with a
> backwards-compatible shim, like so:
> 
>   %files
>   %{!?_licensedir:%global license %%doc}
>   %dir %{gem_instdir}
>   %license %{gem_instdir}/LICENSE.txt

Done

> 
> - In the %changelog section, your name is listed as "mmorsi", and this
> should be "Mo Morsi". (You might want to check the output of the
> rpmdev-packager utility on your box to be sure this is doing the Right
> Thing.)

Seems to be correct.

> 
> - The %{gem_instdir}/spec directory should not be marked as "%doc" since the
> code there is not documentation.

Done. Again thx for review.

-- 
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





[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]