#### Pre-Submission Checklist - [x] Commit message has the format required by CONTRIBUTING guide - [x] Commits are split per component (core, individual modules, libs, utils, ...) - [ ] Each component has a single commit (if not, squash them into one commit) - [x] No commits to README files for modules (changes must be done to docbook files in `doc/` subfolder, the README file is autogenerated)
#### Type Of Change - [x] Small bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds new functionality) - [ ] Breaking change (fix or feature that would change existing functionality)
#### Checklist: - [x] PR should be backported to stable branches - [x] Tested changes locally - [ ] Related to issue #3592
#### Description
The `kamailio-geoip` RPM sub package currently builds both the `geoip` module - based on [the old and out of support MaxMind GeoIP library](https://github.com/maxmind/geoip-api-c) - and the newer `geoip2` module - based on [the currently supported `libmaxminddb`](https://github.com/maxmind/libmaxminddb).
The old GeoIP C library had its end of support date at May 2022 but for a while was still shipped for many distributions. At this point though, the EPEL repository that shipped the GeoIP library for RHEL-based operating systems, has removed it from their repositories for "Enterprise Linux 9": 
Therefor builds using the bundled spec file fail on EL9 installations.
The suggested change is to add an RPM build option `--with geoip2` that when specified will build just the `geoip2` module for the `kamailio-geoip` RPM subpackage. This build option is not enabled by default, unless the build is running on a RHEL 9 based OS - in which case it is set as the default, because otherwise trivial builds break. To revert to the older behavior in RHEL 9 based OSs (and assuming you have the required unsupported library installed correctly) one will need to set both `--without geoip2 --with geoip`. You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/3886
-- Commit Summary --
* packaging: RPM build option to build just the geoip2 module * packaging: make 'with geoip2' the default behavior for EL9
-- File Changes --
M pkg/kamailio/obs/kamailio.spec (36)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/3886.patch https://github.com/kamailio/kamailio/pull/3886.diff
I didn't squash the two commits to the same file as the first is introducing optional behavior and the second is just for changing the default behavior - so I thought it makes sense to see them separately, in case you want to drop the second one. If you prefer that I squash these - just let me know.
This PR is for the main branch. I'm interested in porting it back to at least 5.7 (that I still use) - should I do that after (if) this PR is merged?
Thanks for the PR, it will be reviewed. Regarding the backport, this is done as part of the release process if applicable, no dedicated PR for this is necessary.
@sergey-safarov: this one is about RPM specs, can you do a review and merge if it is ok?
@guss77 when used ``` ```
@sergey-safarov OK, I agree this is confusing - it took me a minute to re-read all I wrote. `--with geoip2` means "disable GeoIP 1" (unlike `--without geoip` which means "disable *all* GeoIP"). This is probably not a great API... 😑
So for RHEL up to and including 8, the defaults are changed to: `%bcond_without geoip` (build with GeoIP, both 1 and 2) and `%bcond_with geoip2` (do not modify the status of GeoIP build). If you look at the `$package geoip` declaration you can see that when `with geoip2` is **not set**, both `GeoIP` and `libmaxminddb` are used.
For RHEL 9 (and up), the default is then changed to: `%bcond_without geoip` (build with GeoIP, both 1 and 2) and `%bcond_without geoip2` (build only GeoIP 2, drop GeoIP 1).
The `%bcond_with/out` macros are confusing enough as it is an I added double the confusion (so, now it is triple strength), where `--with geoip2` actually means `--without geoip1`. OK, now that I think about it - that makes more sense? If you want I can rewrite this patch - instead of adding a "build just geoip2" flag, a "do not build geoip1" flag.
This PR is stale because it has been open 6 weeks with no activity. Remove stale label or comment or this will be closed in 2 weeks.
@guss77 pushed 2 commits.
f888d3f86f8d2526c23fcf072234e81d4b3ab36e packaging: RPM build option to build just the geoip2 module a94a08347e95809a6fa39ce490092d06acce71bb packaging: make 'with geoip2' the default behavior for EL9
@guss77 pushed 1 commit.
fdd7ea2a3ae7d498d395b46e6c26638bf43c158d packaging: disable geoip module when it isn't buildable
I changed the configuration option to be: * `--without geoip1` for distributions that we expect to have the Maxmind GeoIP library but the user wants to build only `geoip2` * `--with geoip1` for distributions we expect to not have the Maxmind GeoIP library, but the user wants to try to build both `geoip` and `geoip2`
@sergey-safarov - I hope that clears the confusion and we can get this (now smaller) change merged.
@sergey-safarov Any more comments on this PR after the latest improvements? If there are no more comments, it will be merged soon.
For me, all `geoip` functionality is present in the `geoip2`. I am fine to move the `geoip` module to the archive and for all dists use `geoip2`.
@linuxmaniac could you also comment?
Fine for me.
Thanks for the comment. @guss77 I might be confused by the spec file syntax, but the change I saw regarding the Red Hat version 9 still enable geoip1, contrary to your description of this PR.
``` %if 0%{?rhel} == 9 [...] %bcond_with geoip %bcond_with geoip1 [...]
```
Regarding the archiving of geoip module, for now I would not like to do it, some old distros still in use have only the option for it. It is fine to not package it as deb or rpm, but still to keep it in the main source code repository.
I have dropped the `geoip` module rpm packaging. The `geoip2` module is still packaged into rpm.
Just to follow up on this PR. So the geoip module was already moved to the archive, and the geoip module was removed from rpm packaging. @guss77 Its my reasoning correct that this PR is then not needed anymore?
geoip module was not moved to the archive, there was a PR for it, but I didn't agree as I am aware of being still used in many deployments. It can be skipped from packaging on new distros, but not removed from repo.
Right, the PRs were not yet merged to finally move it.
Closed #3886.
Just to follow up on this PR. [...] the geoip module was removed from rpm packaging. @guss77 Its my reasoning correct that this PR is then not needed anymore?
Hi, sorry for dropping out.
I've reviewed the referenced here PRs, and they only update deb packaging. I'm using the RPM packaging specs, which have not been modified (at least not by any PR referenced here).
I'll re-do the change in the same manner as the deb PRs to just remove support for `geoip1` for OSs where `geoip2` can be built - and resubmit a new PR.
@henningw I can see that current `master` had the `geoip` conditional flag completely removed as well as building the `geoip` module completely removed in favor of unconditionally building the `geoip2` module.
I'm OK with that change, and I see no need for further work. Thank you very much!
Thanks for checking and the feedback.
-
Daniel-Constantin Mierla -
github-actions[bot]
-
Henning Westerholt
-
Oded Arbel
-
sergey-safarov
-
Victor Seva