build: fix typos in NAT-PMP detection block of configure.ac#447
Open
JSanchezFDZ wants to merge 1 commit into
Open
build: fix typos in NAT-PMP detection block of configure.ac#447JSanchezFDZ wants to merge 1 commit into
JSanchezFDZ wants to merge 1 commit into
Conversation
The `dnl Enable NAT-PMP support` block is a copy of the equivalent
UPnP block immediately above it, with two introduced typos that
compound:
* Line 1571: `if test "$have_natpmp" = "yes"` inverts the
disable/enable branches. The matching UPnP version reads
`if test "$have_miniupnpc" = "no"`. The block decides what to do
when the library is NOT found; with the typo it instead enters
that branch when the library WAS found, so on a successful
detection it disables NAT-PMP and on a failed detection it tries
to enable it.
* Line 1578: `if test "$use_natpmp" != "no"l` — stray trailing `l`.
The shell evaluates `"$use_natpmp" != "no"l` which is always
true (no value ever equals the literal string `"no"l`). The
UPnP equivalent correctly reads `!= "no"`.
The compounded effect: `USE_NATPMP` ends up `AC_DEFINE`'d even when
the user requested `--without-natpmp` (so `mapport.cpp` compiles in
the NAT-PMP code path, since it gates on `#ifdef USE_NATPMP` which
only checks for defined-ness, not value) while at the same time
`NATPMP_LIBS` stays empty (no `-lnatpmp` on the link line). The
result is an undefined-reference link error against
`readnatpmpresponseorretry`, `sendnewportmappingrequest`, and
`sendpublicaddressrequest` in `libraptoreum_server.a(mapport.o)`.
Also fix `determine` -> `determines` in the `AC_DEFINE_UNQUOTED`
description string, matching the UPnP equivalent.
Verified by building raptoreumd with `--disable-wallet
--disable-bench --without-gui` on Ubuntu 22.04 / GCC 11.4 against
develop HEAD: prior to this commit the daemon fails to link, after
it links cleanly (with `USE_NATPMP` correctly left undefined when
the depends/-shipped `config.site` opts out of NAT-PMP).
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix two typos in the NAT-PMP detection block of
configure.acthat cause--disable-walletbuilds (and likely--without-natpmprequests on systems with libnatpmp available) to either link-fail or silently misbehave.The bug
The
dnl Enable NAT-PMP supportblock (lines 1569-1595 on develop) is a copy of the equivalent UPnP block immediately above it, but with two introduced typos:Line 1571 —
if test "$have_natpmp" = "yes"; theninverts the disable/enable branches. The matching UPnP version (line 1543) readsif test "$have_miniupnpc" = "no"; then. The block decides what to do when the library was not found; with the typo it instead enters that branch when the library was found, so the auto-detect path runs the disable path on a successful detection and vice versa.Line 1578 —
if test "$use_natpmp" != "no"l; then. The trailinglis a stray character. The shell evaluates"$use_natpmp" != "no"lwhich is always true (no value of$use_natpmpever equals the literal string"no"l). The matching UPnP version (line 1550) correctly readsif test "$use_upnp" != "no"; then.The compounded effect of the two typos:
USE_NATPMPends upAC_DEFINE'd (somapport.cppcompiles in the NAT-PMP code path, since it uses#ifdef USE_NATPMPwhich only checks for defined-ness, not value) while at the same timeNATPMP_LIBSends up empty (so-lnatpmpis not on the link line). The result on--disable-walletbuilds of develop HEAD (989ec2e):(The reason
--enable-walletbuilds happen to escape the failure on this host is irrelevant link-order luck — the underlying bug is the same in both configurations.)Fix
Make the NAT-PMP block mirror the UPnP block character-for-character (modulo the rename
miniupnpc→natpmp/upnp→natpmp):"yes"→"no"on the outerif test.lafter"no"on the innerif test.determine→determinesin theAC_DEFINE_UNQUOTEDdescription string, matching the UPnP equivalent.Test plan
Verified on develop HEAD (989ec2e) with build prerequisites:
./autogen.sh && CONFIG_SITE=depends/x86_64-pc-linux-gnu/share/config.site.Before patch,
./configure --disable-bench --without-gui --disable-wallet:raptoreum-config.hcontains#define USE_NATPMP 0(a defined-but-zero value).MakefilehasNATPMP_LIBS =(empty).make raptoreumdfails with the link errors quoted above.After patch, same configure:
raptoreum-config.hcontains/* #undef USE_NATPMP */.make raptoreumdbuilds successfully. Daemon starts,--helpworks.After patch,
./configure ... --enable-wallet --with-incompatible-bdb: still builds and runs cleanly. The--with-natpmp/--without-natpmpflags behave as documented.Why this matters
--disable-wallet(headless nodes, masternode-only builds, future Guix-deterministic builds) hit the link failure today ondevelop.Related
amount_testsint64 overflow) and build: add ENABLE_WALLET guards to rpc/rpcassets.cpp #446 (ENABLE_WALLETguards inrpc/rpcassets.cpp) — two other regressions blocking the same--disable-walletverification path.CFeeRatehardening tracking ticket.