Code review comment for ~bryce/ubuntu/+source/nginx:fix-unsatisfiable-depends

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

Ok, replacing the libnginx-mod-stream-geoip dependency with libnginx-mod-stream-geoip2 is the correct move. Both are in groovy-proposed/main, gut libnginx-mod-stream-geoip being in main is incorrect, as it depends on libgeoip1 which is in universe. So besides this change, please ask an archive admin in #ubuntu-release to move libnginx-mod-stream-geoip to universe.

Now, the descriptions:

a) nginx-core
In debian, nginx-core depends on libnginx-mod-http-geoip. This is, as the name implies, an http module, and is referenced in the description under "OPTIONAL HTTP MODULES".

nginx-core also depends on "libnginx-mod-stream-geoip", which is under "OPTIONAL STREAM MODULES".

We are dropping libnginx-mod-http-geoip from here as part of our delta, so we should remove it from the "OPTIONAL HTTP MODULES" line.

And since we are replacing libnginx-mod-stream-geoip with libnginx-mod-stream-geoip2, replacing GeoIP with GeoIP2 in "OPTIONAL STREAM MODULES" is correct and you did that already.

So I think this is the change you should apply to d/control for (a), and I'll add an inline comment to it as well:
diff --git a/debian/control b/debian/control
index 7d2dcacc..61ac42b4 100644
--- a/debian/control
+++ b/debian/control
@@ -97,7 +97,7 @@ Description: nginx web/proxy server (standard version)
  GIF, FastCGI, Geo, Limit Connections, Limit Requests, Map, Memcached, Proxy,
  Referer, Rewrite, SCGI, Split Clients, UWSGI.
  .
- OPTIONAL HTTP MODULES: Addition, Auth Request, Charset, WebDAV, GeoIP2, Gunzip,
+ OPTIONAL HTTP MODULES: Addition, Auth Request, Charset, WebDAV, Gunzip,
  Gzip, Gzip Precompression, Headers, HTTP/2, Image Filter, Index, Log, Real IP,
  Slice, SSI, SSL, SSL Preread, Stub Status, Substitution, Thread Pool,
  Upstream, User ID, XSLT.

b) nginx-full
Debian has it incorrect here, when they say GeoIP is an optional http module, as nginx-full there is pulling in libnginx-mod-http-geoip2, so your change is correct.

So +1 with just that tiny fix for the description of nginx-core, and you will have to ask an AA to demote libnginx-mod-stream-geoip because of its dependency on a universe package.

review: Approve

« Back to merge proposal