Skip to content

main/nginx: add new package for nginx-mod-http-geoip2#6762

Closed
aptalca wants to merge 1 commit intoalpinelinux:masterfrom
aptalca:nginx-mod-http-geoip2
Closed

main/nginx: add new package for nginx-mod-http-geoip2#6762
aptalca wants to merge 1 commit intoalpinelinux:masterfrom
aptalca:nginx-mod-http-geoip2

Conversation

@aptalca
Copy link
Contributor

@aptalca aptalca commented Mar 19, 2019

This PR creates a new package for nginx-mod-http-geoip2 that has a dependency on package libmaxminddb.
Nginx is also compiled with support for the dynamic module.

The package source is https://github.com/leev/ngx_http_geoip2_module

@aptalca aptalca changed the title add new package for nginx-mod-http-geoip2 main/nginx add new package for nginx-mod-http-geoip2 Mar 20, 2019
@aptalca aptalca changed the title main/nginx add new package for nginx-mod-http-geoip2 main/nginx: add new package for nginx-mod-http-geoip2 Mar 20, 2019
@Ikke Ikke added the R-main Main repository label Apr 14, 2019
Copy link
Contributor

@maxwell-k maxwell-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes overall look good.

My only request is to please squash the two commits into one. The first line of the combined commit message should mention the repository and package, perhaps for example:

main/nginx: add http-geoip2 module

For more details about squashing commits please see https://github.com/alpinelinux/aports/blob/master/.github/CONTRIBUTING.md#clean-up-a-pull-request-pr

@aptalca
Copy link
Contributor Author

aptalca commented Apr 17, 2019

@maxwell-k thanks, commits squashed as requested

@maxwell-k
Copy link
Contributor

@aptalca: that's great thank you, at the moment our Drone CI is failing. If you please rebase that commit on the current master the CI should pass.

The Drone config has changed recently and it now applies a PR differently, the current approach is at:
https://github.com/alpinelinux/alpine-drone-ci/blob/master/files/usr/local/bin/build.sh#L81

Thanks for your help.

@aptalca
Copy link
Contributor Author

aptalca commented Apr 21, 2019

@maxwell-k thanks for the heads up and the pointers. All tests are successful now.

_add_module "http-vod" "1.24" "https://github.com/kaltura/nginx-vod-module"

_add_module "http-geoip2" "3.2" "https://github.com/leev/ngx_http_geoip2_module"
_http_geoip2_depends="libmaxminddb"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you depending on libmaxmindb?
abuild will automatically scan elf files for dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was not aware of that. I noticed the other module packages listing their dependencies, so added the dependency here. If it's not needed, I can remove and squash/rebase. Please confirm.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYR, you can try to build it with abuild -rK (dont forget to cleanup build deps afterwards) and check the contents of

pkg/.control.$subpkgname/.PKGINFO
pkg/.control.nginx-mod-http-geoip2/.PKGINFO

This should already include the depends to libmaxinddb.
Explicit depends on libs is not needed in normal cases.

Copy link
Contributor Author

@aptalca aptalca Apr 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clandmeter
Yup, you're right. I locally built the package without the dependency listed and .PKGINFO lists the libmaxminddb.so and installing the package also installs libmaxminddb.

However, digging through the PKGINFO files, I realized that libmaxminddb is now also "auto detected" as a dependency for the main nginx package. No idea how that happened. libmaxminddb-dev is installed as a build dependency and the module is supposed to be built as a dynamic module, same as the geoip-dev package that is installed as a build dependency for the geoip1 module

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checkpkg says that the main package gets:

+usr/lib/nginx/modules/ngx_stream_geoip2_module.so

So the automatic package split is not moving all modules to the subpackage. The above file that is left in the main package is introducing the dependency to libmaxminddb.so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabled thanks so much for pointing me in the right direction. I'll get that figured out shortly

@algitbot
Copy link

algitbot commented May 6, 2019

Merged in ff3a47c by @jirutka. Thanks for your contribution!

(This pull request has been closed automatically by GitHub PR Closer. If you think that it’s not resolved yet, please add a comment.)

@algitbot algitbot closed this May 6, 2019
@aptalca aptalca deleted the nginx-mod-http-geoip2 branch May 8, 2019 20:36
@PeterDaveHello
Copy link
Contributor

Hello, thanks for adding this package, just wondering if there is anything wrong that I can't simply use it? Got no such problem with nginx-mod-http-geoip, thank you.

$ docker run --rm -it alpine:3.10 sh
/ # apk add nginx nginx-mod-http-geoip2
fetch http://dl-cdn.alpinelinux.org/alpine/v3.10/main/x86_64/APKINDEX.tar.gz
fetch http://dl-cdn.alpinelinux.org/alpine/v3.10/community/x86_64/APKINDEX.tar.gz
(1/8) Installing pcre (8.43-r0)
(2/8) Installing nginx (1.16.1-r0)
Executing nginx-1.16.1-r0.pre-install
(3/8) Installing ca-certificates (20190108-r0)
(4/8) Installing nghttp2-libs (1.39.2-r0)
(5/8) Installing libcurl (7.66.0-r0)
(6/8) Installing curl (7.66.0-r0)
(7/8) Installing libmaxminddb (1.3.2-r0)
(8/8) Installing nginx-mod-http-geoip2 (1.16.1-r0)
Executing busybox-1.30.1-r2.trigger
Executing ca-certificates-20190108-r0.trigger
OK: 9 MiB in 22 packages
/ # nginx -t
nginx: [emerg] dlopen() "/var/lib/nginx/modules/ngx_stream_geoip2_module.so" failed (Error relocating /var/lib/nginx/modules/ngx_stream_geoip2_module.so: ngx_stream_add_variable: symbol not found) in /etc/nginx/modules/http_geoip2.conf:1
nginx: configuration file /etc/nginx/nginx.conf test failed

@PeterDaveHello
Copy link
Contributor

Okay I saw #9234 & #9351

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

R-main Main repository

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants