Skip to content

ci: replace stale libgeoip with libmaxminddb in CI workflow deps - #507

Merged
mrjimenez merged 2 commits into
amule-project:masterfrom
got3nks:pr1-ccpp-maxminddb
May 1, 2026
Merged

ci: replace stale libgeoip with libmaxminddb in CI workflow deps#507
mrjimenez merged 2 commits into
amule-project:masterfrom
got3nks:pr1-ccpp-maxminddb

Conversation

@got3nks

@got3nks got3nks commented May 1, 2026

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #502 which migrated aMule from libGeoIP to libmaxminddb (GeoIP-1 .dat → GeoIP-2 .mmdb).
The CI workflows' apt / brew / pacman deps lists kept the old libGeoIP packages, which left ENABLE_IP2COUNTRY silently off on every CI run since #502 merged:

  • Ubuntu: had libgeoip-dev but no libmaxminddb-dev. cmake/ip2country.cmake's find_path(maxminddb.h) fell through the soft-fail and disabled the feature.
  • Windows MSYS2: same gap (mingw-w64-x86_64-geoip instead of mingw-w64-x86_64-libmaxminddb).
  • macOS: hardcoded -DENABLE_IP2COUNTRY=NO with a comment claiming "Homebrew dropped legacy libgeoip; only libmaxminddb is available, which aMule doesn't use" — outdated since IP2Country: swap libGeoIP for libmaxminddb (.dat → .mmdb) #502, libmaxminddb is in Homebrew and aMule does use it.
  • CodeQL workflow (.github/workflows/codeql.yml): same libgeoip-dev entry, same silent IP2Country skip on every scheduled scan.

Changes

  • Ubuntu (ccpp.yml): libgeoip-devlibmaxminddb-dev
  • Windows (ccpp.yml): mingw-w64-x86_64-geoipmingw-w64-x86_64-libmaxminddb
  • macOS (ccpp.yml): add libmaxminddb to the Homebrew install list
  • macOS (ccpp.yml): flip -DENABLE_IP2COUNTRY=NO=YES and drop the stale comment
  • CodeQL (codeql.yml): libgeoip-devlibmaxminddb-dev so the manual c-cpp build covers the IP2Country code path

Net effect

PR builds and CodeQL scans now exercise the IP2Country code path on all three platforms instead of silently skipping it. No source-code changes; runtime behaviour for users is unchanged (the database file is still user-supplied, see docs/IP2Country.md).

got3nks added 2 commits May 1, 2026 09:53
Follow-up to amule-project#502 which migrated aMule from libGeoIP to libmaxminddb
(GeoIP-1 .dat → GeoIP-2 .mmdb). The CI workflow's apt / brew /
pacman deps lists kept the old libGeoIP packages, which left
ENABLE_IP2COUNTRY silently off on every CI run:

* Ubuntu had `libgeoip-dev` (unused since amule-project#502) but no
  `libmaxminddb-dev`, so cmake/ip2country.cmake's find_path on
  maxminddb.h fell through the soft-fail and disabled the feature.
* Windows MSYS2 had the same gap (`mingw-w64-x86_64-geoip` instead
  of `mingw-w64-x86_64-libmaxminddb`).
* macOS hardcoded `cmake_macos_config_flags: -DENABLE_IP2COUNTRY=NO`
  with a comment claiming "Homebrew dropped legacy libgeoip; only
  libmaxminddb is available, which aMule doesn't use" — outdated
  since amule-project#502, libmaxminddb is in Homebrew and aMule does use it.

This patch:

* Replaces `libgeoip-dev` -> `libmaxminddb-dev` on Ubuntu
* Replaces `mingw-w64-x86_64-geoip` -> `mingw-w64-x86_64-libmaxminddb`
  on Windows
* Adds `libmaxminddb` to the macOS Homebrew install list
* Flips macOS `-DENABLE_IP2COUNTRY=NO` -> `=YES` and drops the stale
  comment

Net effect: PR builds now exercise the IP2Country code path on all
three platforms instead of silently skipping it. No source-code
changes; runtime behaviour for users is unchanged (the database file
is still user-supplied, see docs/IP2Country.md).
@got3nks got3nks changed the title ci: replace stale libgeoip with libmaxminddb in ccpp.yml deps ci: replace stale libgeoip with libmaxminddb in CI workflow deps May 1, 2026
@mrjimenez
mrjimenez merged commit 5d65d20 into amule-project:master May 1, 2026
12 checks passed
@Vollstrecker

Copy link
Copy Markdown
Collaborator

For that one: correct (as almost always). For future chnages: I want to get away from that "silently disabled". If a user set a feature ti enable and the deps for it aren't found, we don't disable it and keep going, we error-out and the feature will be disabled by the user, or the deps is provided. Same the other way: We don't check check for deps and enable features the user didn't ask for.

Again, no critic at all, I just wanted to state the vision as clear as possible, and thanks for your work.

@mrjimenez

Copy link
Copy Markdown
Contributor

Totally agree with @Vollstrecker here. If we don't fail the attempt and instead let the script find another way to succeed, we damage the reproducibility of the build. At best, we can send messages for the user to enable the features that will make the build succeed.

@got3nks

got3nks commented May 1, 2026

Copy link
Copy Markdown
Contributor Author

Agreed - addresed in #509

@got3nks

got3nks commented May 1, 2026

Copy link
Copy Markdown
Contributor Author

Good call, agree completely. I should've spotted that #507 was just patching the symptom (CI deps) while the real issue — the soft-fail in cmake/ip2country.cmake — sat untouched. My bad.

Opened #509 to fix it properly: -DENABLE_IP2COUNTRY=YES with missing deps now errors out with platform install hints, instead of silently flipping itself to =NO.

Same anti-pattern is in boost.cmake, upnp.cmake, and nls.cmake — happy to send small follow-up PRs for each once #509 looks good.

Thanks for spelling out the policy.

mrjimenez pushed a commit that referenced this pull request May 1, 2026
When the user explicitly passes -DENABLE_IP2COUNTRY=YES, the
soft-disable on missing libmaxminddb produced a green build with the
feature silently dropped — there is no way for the user to discover
the failure short of reading every STATUS line, which is exactly the
kind of mismatch this project's policy now wants gone.

Replace the silent downgrade with FATAL_ERROR messages that name the
exact dev/runtime package on Debian/Ubuntu, Fedora, Homebrew and
MSYS2, plus the escape hatch (-DENABLE_IP2COUNTRY=NO) for users who
genuinely don't want the feature. Honours the rule stated by
@Vollstrecker on PR #507: explicit user intent beats implicit
dep probing — set the feature, get the feature or a clear error,
never a silent miss.

The companion soft-fail patterns in cmake/boost.cmake,
cmake/upnp.cmake and cmake/nls.cmake have the same anti-pattern but
are out of scope here; will follow in subsequent PRs once this
approach lands.
mrjimenez pushed a commit that referenced this pull request May 2, 2026
Mirror of #509 applied to cmake/upnp.cmake. When the user passes
-DENABLE_UPNP=YES (or leaves the option at its default-on) but libupnp
isn't present, the previous behaviour was to silently flip the flag to
FALSE and emit a STATUS line — same anti-pattern PR #509 fixed for
IP2Country: a green build with the feature mysteriously absent at
runtime, with no error in the developer's terminal output.

Replace the silent downgrade with a FATAL_ERROR carrying platform-
specific install hints. The DOWNLOAD_AND_BUILD_DEPS fallback (where
CMake builds libupnp from source) stays as a legitimate alternate
fulfilment path — only the no-deps-and-no-fallback branch now errors
out.

Per @Vollstrecker on #507 (comment):

  > If a user sets a feature to enable and the deps for it aren't found,
  > we don't disable it and keep going, we error-out and the feature
  > will be disabled by the user, or the deps is provided.

The same anti-pattern is also in cmake/boost.cmake and cmake/nls.cmake;
those land as separate one-file PRs to keep each diff easily reviewable.

Test plan:

* configure with deps present (-DENABLE_UPNP=YES) — succeeds, UPnP
  enabled (verified locally on macOS Homebrew).
* configure with deps missing (-DENABLE_UPNP=YES) — fails with the new
  FATAL_ERROR message naming libupnp-dev / libupnp / mingw-w64-x86_64-pupnp
  and pointing at the -DENABLE_UPNP=NO and -DDOWNLOAD_AND_BUILD_DEPS=YES
  escape hatches.
* configure with -DENABLE_UPNP=NO — succeeds, no probing happens
  (top-level CMakeLists.txt:83 gates the include behind the option).
mrjimenez pushed a commit that referenced this pull request May 2, 2026
Mirror of #509 / #511 applied to cmake/nls.cmake. Two soft-fail spots
both flipped to FATAL_ERROR with platform-specific install hints:

1. msgfmt / msgmerge missing — fires on any platform without GNU
   gettext-tools installed. Hint covers Debian/Ubuntu (gettext),
   Fedora (gettext), macOS Homebrew (gettext), MSYS2
   (mingw-w64-x86_64-gettext).

2. libintl missing — fires only on platforms with a separate libintl
   runtime (macOS, MinGW/MSYS2, *BSD, musl). On glibc, libintl is part
   of libc itself and find_package(Intl) reports FOUND with empty
   Intl_LIBRARIES; the branch never fires there. Hint includes a
   note that hitting it on a glibc system means glibc-devel /
   libc6-dev is incomplete.

Both branches previously did 'set (ENABLE_NLS FALSE)' + STATUS
message, masking the missing dep behind a green build with no
localization at runtime — same anti-pattern PR #509 fixed for
IP2Country and #511 fixed for UPnP.

Per @Vollstrecker on #507 (comment):

  > If a user sets a feature to enable and the deps for it aren't found,
  > we don't disable it and keep going, we error-out and the feature
  > will be disabled by the user, or the deps is provided.

The same anti-pattern in cmake/boost.cmake will land as a separate
small PR (more nuanced — DOWNLOAD_AND_BUILD_DEPS interaction).

Test plan:

* configure with deps present (-DENABLE_NLS=YES) — succeeds, NLS
  enabled (verified locally on macOS Homebrew gettext).
* configure with msgfmt missing (-DENABLE_NLS=YES) — fails with the
  new FATAL_ERROR naming gettext per platform.
* configure with libintl missing on macOS/MinGW (-DENABLE_NLS=YES)
  — fails with the new FATAL_ERROR naming the runtime package.
* configure with -DENABLE_NLS=NO — succeeds, no probing happens
  (top-level CMakeLists.txt:79 gates the include behind the option).
mrjimenez pushed a commit that referenced this pull request May 2, 2026
Mirror of #509 / #511 / #512 applied to cmake/boost.cmake. Two
soft-fail spots both flipped to FATAL_ERROR:

1. asio probe failed (boost found but check_include_files for
   boost/system/error_code.hpp + boost/asio.hpp couldn't compile),
   no DOWNLOAD_AND_BUILD_DEPS opt-in. This is the realistic case —
   a user has libboost-dev installed but missing some transitive
   header / version mismatch / link-step issue tripped the probe.
   Previously: silently set ENABLE_BOOST=FALSE, falling back to
   wxWidgets sockets. Now: FATAL_ERROR with hint to install full
   boost-dev, or pass -DENABLE_BOOST=NO, or pass
   -DDOWNLOAD_AND_BUILD_DEPS=YES.

2. Boost_FOUND=FALSE — defensive branch, practically unreachable
   because find_package(Boost CONFIG REQUIRED) at the top of the
   file already errors out when boost is absent. Replacing the
   silent disable with FATAL_ERROR makes the assumption explicit:
   if a future edit ever drops the REQUIRED keyword, this branch
   surfaces the regression instead of producing a green build
   with no boost.

The DOWNLOAD_AND_BUILD_DEPS escape hatch (where CMake is expected
to build Boost itself elsewhere in the configure pass) is preserved
for branch 1 — when DAaBD is set, no error fires.

Per @Vollstrecker on #507 (comment):

  > If a user sets a feature to enable and the deps for it aren't found,
  > we don't disable it and keep going, we error-out and the feature
  > will be disabled by the user, or the deps is provided.

This completes the four-file cmake/<feature>.cmake hard-fail series
(ip2country in #509, upnp in #511, nls in #512, boost here).

Test plan:

* configure with deps present (-DENABLE_BOOST=YES) — succeeds, boost
  found, ASIO_SOCKETS=TRUE (verified locally on macOS Homebrew
  boost 1.90.0).
* configure with -DENABLE_BOOST=NO — succeeds, no probing happens
  (top-level CMakeLists.txt:71 gates the include behind the option).
* configure with deps missing (-DENABLE_BOOST=YES, no boost) — the
  REQUIRED keyword on find_package(Boost CONFIG) at line 1 errors
  out before our changes are reached; the new FATAL_ERROR for the
  asio-probe-failed case isn't triggerable without artificially
  installing a broken boost. Mechanically identical FATAL_ERROR
  shape to #509 / #511 / #512.
got3nks added a commit to got3nks/amule that referenced this pull request May 2, 2026
…3nks

mrjimenez flagged on PR amule-project#514 review that these three contributors
were missing from the roster:

* Werner Mahr / Vollstrecker — has merge access on amule-project/amule
  and merges incoming PRs (e.g. amule-project#430, amule-project#385, amule-project#319, amule-project#330, amule-project#286). Goes
  under Maintainers based on that role.
* Pablo Barciela / Sc0w — recent contributor, e.g. wxWidgets 3.2.6
  build fixes, GTK version-check cleanup, dropping gtk1/gtk2 support,
  Chinese translation work. Goes under Developers.
* got3nks — Wayland app_id binding + SNI tray (amule-project#508), macOS UX
  (amule-project#508), libmaxminddb CI fix (amule-project#507), ip2country/upnp/nls/boost
  hard-fail series (amule-project#509/amule-project#511/amule-project#512/amule-project#513), packaging (amule-project#510),
  docs modernize (this PR). Goes under Developers.

Other recent folks may be missing too — those can be added in
follow-up commits as they're identified. mrjimenez explicitly noted
this is the priority subset to not block the rename PR.
mrjimenez pushed a commit that referenced this pull request May 2, 2026
…3nks

mrjimenez flagged on PR #514 review that these three contributors
were missing from the roster:

* Werner Mahr / Vollstrecker — has merge access on amule-project/amule
  and merges incoming PRs (e.g. #430, #385, #319, #330, #286). Goes
  under Maintainers based on that role.
* Pablo Barciela / Sc0w — recent contributor, e.g. wxWidgets 3.2.6
  build fixes, GTK version-check cleanup, dropping gtk1/gtk2 support,
  Chinese translation work. Goes under Developers.
* got3nks — Wayland app_id binding + SNI tray (#508), macOS UX
  (#508), libmaxminddb CI fix (#507), ip2country/upnp/nls/boost
  hard-fail series (#509/#511/#512/#513), packaging (#510),
  docs modernize (this PR). Goes under Developers.

Other recent folks may be missing too — those can be added in
follow-up commits as they're identified. mrjimenez explicitly noted
this is the priority subset to not block the rename PR.
@got3nks
got3nks deleted the pr1-ccpp-maxminddb branch May 2, 2026 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants