[Bug 993482] Review Request: rubygem-geoip - Search a GeoIP database for an IP address

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

 



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

Ken Dreyer <ktdreyer@xxxxxxxxxxxx> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|package-review@lists.fedora |
                   |project.org                 |
              Flags|needinfo?(ktdreyer@ktdreyer |
                   |.com)                       |



--- Comment #4 from Ken Dreyer <ktdreyer@xxxxxxxxxxxx> ---
Thank you very much for the review.

(In reply to Jan Včelák from comment #3)
> [!]: Package must own all directories that it creates.
> 
>      Directory '/usr/share/gems/doc' is not owned.

I think this may be a bug in fedora-review? I have seen this pop up on every
gem I have created and reviewed. /usr/share/gems/doc is properly owned by a
package ("yum whatprovides /usr/share/gems/doc" shows "rubygems").


>      In addition, i think
>      the documentation should be placed somewhere else.

Can you help me understand what you have in mind? I'm pretty sure this is where
all gems place their documentation.


> [!]: Requires correct, justified where necessary.
> 
>      There should be just (others are extra):
> 
>      Requires: ruby(release)
>      Requires: rubygems

Thanks, I've filtered out /usr/bin/ruby in 1.3.0-2.


> [!]: All build dependencies are listed in BuildRequires, except for any that
>      are listed in the exceptions section of Packaging Guidelines.
> 
>      There should be just (others are extra):
> 
>      BuildRequires: rubygems-devel

This probably needs to be fixed in gem2rpm. I've removed the extra
BuildRequires in 1.3.0-2.


> [!]: Permissions on files are set properly.
> 
>      License file is installed as executable.

Great catch. Fixed.


>      I think the installation of geoip binary is cryptic. I do not like
>      the combination of cp, find, xargs and chmod. I would prefer
>      'install -m 0755 ...'. But the result is fine, feel free to keep this
>      as it is now.

I agree that it's cryptic. I'll keep it as-is for the sake of consistency with
other packages. But this probably needs to be fixed in gem2rpm.


> [!]: Final provides and requires are sane (see attachments).
> 
>      Final provides are sane, but rubygem(name) should not be specified
>      in Provides explicitly, it is generated by rpm.

I experimented with this, and I cannot make RPM generate the virtual provide.
When I remove the explicit Provides: rubygem(name) in the .spec, and I check
the resulting binary RPM with rpm -qp --provides, I just see rubygem-geoip, not
rubygem(geoip). I think this is why gem2rpm puts this into the package.



Here's the updated package:

Diff:
http://fedorapeople.org/cgit/ktdreyer/public_git/rubygem-geoip.git/commit/?id=5865ec8fb6094a6483e436a6d0c571ea56d7b682

Spec URL: http://ktdreyer.fedorapeople.org/reviews/rubygem-geoip.spec
SRPM URL:
http://ktdreyer.fedorapeople.org/reviews/rubygem-geoip-1.3.0-2.fc21.src.rpm

Rawhide scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=5930798

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=5WCDodLmTV&a=cc_unsubscribe
_______________________________________________
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]