Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 181974
Summary: | Review Request: mod_geoip - Apache module for GeoIP lookups | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Michael Fleming <mfleming+rpm> |
Component: | Package Review | Assignee: | Jason Tibbitts <j> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | ||
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-02-20 22:39:57 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: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 |
Description
Michael Fleming
2006-02-18 06:18:07 UTC
Looks clean and builds properly in mock (i386, development). rpmlint is silent. The package meets the naming and packaging guidelines. License is appropriate and matches source. Source file matches upstream. The specfile is properly named and is legible and understandable. BuildRequires: are proper. I find the Requires: a bit odd. What's the point of having httpd-mmn = missing /usr/include/httpd/.mmn doesn't exist? In any case, not a blocker. Approved. The tarball seems to be called "mod_geoip2". I know too little about this package, but why isn't the rpm called the same rsp. why doesn't the rpm provide a virtual property of this name? I'll answer both questions in one reply - Both are quite valid questions. Jason: The httpd-mmn Requires: is to ensure that the module version matches that of the Apache version you're going to install against. (Something one of the more experienced packagers (Ville?) pointed out while I was working on mod_security) Ralf: I called it mod_geoip to avoid potential confusion ("Where's/What is mod_geoip1?) and ensure some level of clarity. Upstream has different tarballs for Apache 1.3.x and 2.x, the latter is mod_geoip2 but, given that FC doesn't package the older branch anyway, I went for plain "mod_geoip" so even the most inebriated of users could guess what it was for :-D. As for a virtual property, I've never seen anyone package it elsewhere - and if they've called it mod_geoip they'll get cleanly upgraded anyway provided other deps are met. I'll import and build unless anyone has strong "blocker" objections to the name. I implicitly understood your reasoning for using mod_geoip and not mod_geoip2 and didn't see it as a problem. The name of the generated module is mod_geoip.so and the upstream name of the package is mod_geoip: http://www.maxmind.com/app/mod_geoip FYI, Mandriva seems to ship this package as mod_geoip as well: http://rpmfind.net/linux/RPM/mandriva/devel/cooker/cooker/media/contrib/Mandriva.html About the Requires:, I understand why it's there but I didn't quite get what use a Requires: of "httpd-mmn = missing" (in the case that /usr/include/httpd/.mmn doesn't exist) is going to be. I suppose it's just defensive programming, which is no problem with me. (In reply to comment #4) > I implicitly understood your reasoning for using mod_geoip and not mod_geoip2 > and didn't see it as a problem. The name of the generated module is > mod_geoip.so and the upstream name of the package is mod_geoip: > http://www.maxmind.com/app/mod_geoip I asked because of the mod_perl vs. mod_perl2 idocy. "httpd-mmn = missing" comes from some Core packages, so you'll have to ask there to get a definite answer. But at least it allows the specfile syntax to be ok even when httpd-devel is not installed, enabling for example rpm -q --specfile queries in those scenarios. If the "|| echo missing" part would not be there, it wouldn't work. Imported source RPM into CVS, will request a build when I get home from work. |