build: remove duplicate `-lminiupnpc` linking #28755

fanquake · 2023-10-30T15:00:15Z

Having the link check in the header check loop means we get -lminiupnpc -lminiupnpc -lminiupnpc on the link line.
This is unnecessary, and results in warnings, i.e:

ld: warning: ignoring duplicate libraries: '-levent', '-lminiupnpc'
ld: warning: ignoring duplicate libraries: '-levent', '-lminiupnpc'
ld: warning: ignoring duplicate libraries: '-levent', '-lminiupnpc'

These warnings have been occurring since the new macOS linker released with Xcode 15, and also came up in hebasto#34.

There are other duplicate lib issues, i.e with -levent + -levent_pthreads -levent, but those are less straight forward to solve, and won't be included here.

DrahtBot · 2023-10-30T15:00:18Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto, TheCharlatan, theuni, jonatack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

theuni · 2023-10-30T17:45:59Z

As suggested by the docs: "You can give it a value of ‘break’ to break out of the loop on the first match."

Seems to work as intended:

diff --git a/configure.ac b/configure.ac
index 9b4b9bd42b..fdefad609c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1423,7 +1423,7 @@ if test "$use_upnp" != "no"; then
   CPPFLAGS="$CPPFLAGS $MINIUPNPC_CPPFLAGS"
   AC_CHECK_HEADERS(
     [miniupnpc/miniupnpc.h miniupnpc/upnpcommands.h miniupnpc/upnperrors.h],
-    [AC_CHECK_LIB([miniupnpc], [upnpDiscover], [MINIUPNPC_LIBS="$MINIUPNPC_LIBS -lminiupnpc"], [have_miniupnpc=no], [$MINIUPNPC_LIBS])],
+    [AC_CHECK_LIB([miniupnpc], [upnpDiscover], [MINIUPNPC_LIBS="$MINIUPNPC_LIBS -lminiupnpc"; break], [have_miniupnpc=no], [$MINIUPNPC_LIBS])],
     [have_miniupnpc=no]
   )

fanquake · 2023-10-30T17:49:44Z

Right, but I'm not sure why we even want to continue performing a link check for each header (with the same lib). Seems simpler to check for each header we want, and do a single link check separately.

theuni · 2023-10-30T18:00:14Z

Right, but I'm not sure why we even want to continue performing a link check for each header (with the same lib).

This is intended to be a shorthand pattern for: "only do the link test if we've found the headers", otherwise attempting to link makes no sense.

Seems simpler to check for each header we want, and do a single link check separately.

Sure, but it only makes sense to try to link once the headers have been found successfully. As-is in this PR, afaics, the AC_CHECK_HEADERS here is moot.

I don't really care much either way, configure.ac won't be around much longer :)

theuni · 2023-10-30T18:03:21Z

As suggested by the docs: "You can give it a value of ‘break’ to break out of the loop on the first match."

Seems to work as intended:

Nevermind, this doesn't actually work. It stops at the first found header. Disregard :)

fanquake · 2023-10-30T19:21:23Z

I don't really care much either way, configure.ac won't be around much longer :)

I think my want to "fix" this is two fold. Simpler porting for cmake, when comparing link behaviour & similar.

If these link warnings ever turn into link errors down the line, we'll have broken release branches for some amount of time.

jonatack

Lightly tested ACK 33fe16b

It doesn't seem to diminish the overall volume of warnings much, but is perhaps worth improving if these warnings were to become errors.

Build output on ARM64 Ventura 13.6 with Homebrew clang version 17.0.3:

from this (on master)

ld: warning: -bind_at_load is deprecated on macOS
ld: warning: -bind_at_load is deprecated on macOS
ld: warning: ignoring duplicate libraries: '-levent', '-lminiupnpc'
ld: warning: -bind_at_load is deprecated on macOS
ld: warning: -bind_at_load is deprecated on macOS
ld: warning: -bind_at_load is deprecated on macOS
ld: warning: ignoring duplicate libraries: '-levent', '-lminiupnpc', 'libbitcoin_util.a'
ld: warning: -bind_at_load is deprecated on macOS
ld: warning: ignoring duplicate libraries: '-levent', '-lminiupnpc'
ld: warning: -bind_at_load is deprecated on macOS
ld: warning: -bind_at_load is deprecated on macOS
ld: warning: ignoring duplicate libraries: '-levent', '-lminiupnpc', 'libbitcoin_util.a'
ld: warning: -bind_at_load is deprecated on macOS
ld: warning: -bind_at_load is deprecated on macOS
ld: warning: -bind_at_load is deprecated on macOS
ld: warning: ignoring duplicate libraries: '-levent', '-lminiupnpc'
ld: warning: -bind_at_load is deprecated on macOS
ld: warning: ignoring duplicate libraries: '-levent', '-lminiupnpc'
Making all in doc/man
make[1]: Nothing to be done for `all'.
make[1]: Nothing to be done for `all-am'.

to this (on this branch)

ld: warning: -bind_at_load is deprecated on macOS
ld: warning: -bind_at_load is deprecated on macOS
ld: warning: ignoring duplicate libraries: '-levent'
ld: warning: -bind_at_load is deprecated on macOS
ld: warning: -bind_at_load is deprecated on macOS
ld: warning: ignoring duplicate libraries: '-levent'
ld: warning: -bind_at_load is deprecated on macOS
ld: warning: -bind_at_load is deprecated on macOS
ld: warning: ignoring duplicate libraries: '-levent'
ld: warning: -bind_at_load is deprecated on macOS
ld: warning: -bind_at_load is deprecated on macOS
ld: warning: ignoring duplicate libraries: '-levent'
ld: warning: -bind_at_load is deprecated on macOS
ld: warning: -bind_at_load is deprecated on macOS
ld: warning: -bind_at_load is deprecated on macOS
ld: warning: ignoring duplicate libraries: '-levent', 'libbitcoin_util.a'
ld: warning: -bind_at_load is deprecated on macOS
ld: warning: ignoring duplicate libraries: '-levent', 'libbitcoin_util.a'
ld: warning: -bind_at_load is deprecated on macOS
ld: warning: -bind_at_load is deprecated on macOS
Making all in doc/man
make[1]: Nothing to be done for `all'.
make[1]: Nothing to be done for `all-am'.

TheCharlatan

ACK 33fe16b

configure.ac

Having the link check in the header check loop means we get `-lminiupnpc -lminiupnpc -lminiupnpc` on the link line. This is unnecessary, and results in warnings, i.e: ```bash ld: warning: ignoring duplicate libraries: '-levent', '-lminiupnpc' ld: warning: ignoring duplicate libraries: '-levent', '-lminiupnpc' ld: warning: ignoring duplicate libraries: '-levent', '-lminiupnpc' ``` These warnings have been occurring since the new linker released with Xcode 15, and also came up in hebasto#34.

Consolidate the lib checking logic to be the same as miniupnpc.

hebasto

ACK b74e449, it fixes one issue mentioned in hebasto#34 (comment).

./configure output:

in the master branch:

...
checking for miniupnpc/miniupnpc.h... yes
checking for upnpDiscover in -lminiupnpc... yes
checking for miniupnpc/upnpcommands.h... yes
checking for upnpDiscover in -lminiupnpc... (cached) yes
checking for miniupnpc/upnperrors.h... yes
checking for upnpDiscover in -lminiupnpc... (cached) yes
checking whether miniUPnPc API version is supported... yes
checking for natpmp.h... yes
checking for initnatpmp in -lnatpmp... yes
...
checking whether to build with support for UPnP... yes
checking whether to build with support for NAT-PMP... yes
...

in this PR branch:

...
checking for miniupnpc/miniupnpc.h... yes
checking for miniupnpc/upnpcommands.h... yes
checking for miniupnpc/upnperrors.h... yes
checking for upnpDiscover in -lminiupnpc... yes
checking whether miniUPnPc API version is supported... yes
checking for natpmp.h... yes
checking for initnatpmp in -lnatpmp... yes
...
checking whether to build with support for UPnP... yes
checking whether to build with support for NAT-PMP... yes
...

TheCharlatan

ACK b74e449

theuni · 2023-10-31T17:05:01Z

configure.ac

-                   [have_natpmp=no])
+  AC_CHECK_HEADERS([natpmp.h], [], [have_natpmp=no])
+
+  if test "$have_natpmp" != "no"; then


I think this is one of those cases where you need the funky xsyntax.

$have_natpmp is either "no" or empty. IIRC empty can screw up test on some shells.

This is usually avoided by either initializing to something or doing: if test "x$have_natpmp" != "xno"; then.

IIRC empty can screw up test on some shells.

I think bash fixed that in 1996, and we have since removed all the x-prefix usage from our build system. It'd be good to not have to reintroduce it.

Lol, you trying to tell me something? :)

theuni · 2023-10-31T17:07:00Z

configure.ac

  if test "$have_miniupnpc" != "no"; then
+    AC_CHECK_LIB([miniupnpc], [upnpDiscover], [MINIUPNPC_LIBS="$MINIUPNPC_LIBS -lminiupnpc"], [have_miniupnpc=no], [$MINIUPNPC_LIBS])


Same xsyntax thing here, but clearly it's not been an issue. Maybe the quotes are enough.

theuni

ACK b74e449

jonatack · 2023-10-31T23:31:35Z

ACK b74e449

Build output diff is the same as my previous #28755 (review), and with the latest push also see a similar improvement as reported by @hebasto in #28755 (review):

master

checking for miniupnpc/miniupnpc.h... yes
checking for upnpDiscover in -lminiupnpc... yes
checking for miniupnpc/upnpcommands.h... yes
checking for upnpDiscover in -lminiupnpc... (cached) yes
checking for miniupnpc/upnperrors.h... yes
checking for upnpDiscover in -lminiupnpc... (cached) yes
checking whether miniUPnPc API version is supported... yes
checking for natpmp.h... no
...
checking whether to build with support for UPnP... yes
checking whether to build with support for NAT-PMP... no

this branch

checking for miniupnpc/miniupnpc.h... yes
checking for miniupnpc/upnpcommands.h... yes
checking for miniupnpc/upnperrors.h... yes
checking for upnpDiscover in -lminiupnpc... yes
checking whether miniUPnPc API version is supported... yes
checking for natpmp.h... no
...
checking whether to build with support for UPnP... yes
checking whether to build with support for NAT-PMP... no

DrahtBot added the Build system label Oct 30, 2023

fanquake mentioned this pull request Oct 30, 2023

cmake: Avoid overlinking hebasto/bitcoin#34

Merged

fanquake requested a review from TheCharlatan October 30, 2023 16:36

jonatack reviewed Oct 30, 2023

View reviewed changes

TheCharlatan approved these changes Oct 31, 2023

View reviewed changes

hebasto reviewed Oct 31, 2023

View reviewed changes

configure.ac Outdated Show resolved Hide resolved

fanquake force-pushed the remove_duplicate_linking branch from 33fe16b to 9a82389 Compare October 31, 2023 10:58

hebasto reviewed Oct 31, 2023

View reviewed changes

configure.ac Outdated Show resolved Hide resolved

fanquake added 2 commits October 31, 2023 11:12

build: remove potential for duplciate natpmp linking

b74e449

Consolidate the lib checking logic to be the same as miniupnpc.

fanquake force-pushed the remove_duplicate_linking branch from 9a82389 to b74e449 Compare October 31, 2023 11:12

hebasto approved these changes Oct 31, 2023

View reviewed changes

DrahtBot requested review from TheCharlatan and jonatack October 31, 2023 11:30

TheCharlatan approved these changes Oct 31, 2023

View reviewed changes

theuni reviewed Oct 31, 2023

View reviewed changes

theuni approved these changes Oct 31, 2023

View reviewed changes

DrahtBot removed the request for review from jonatack October 31, 2023 23:31

fanquake merged commit 9d594ed into bitcoin:master Nov 1, 2023
16 checks passed

fanquake deleted the remove_duplicate_linking branch November 1, 2023 10:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: remove duplicate `-lminiupnpc` linking #28755

build: remove duplicate `-lminiupnpc` linking #28755

fanquake commented Oct 30, 2023

DrahtBot commented Oct 30, 2023 •

edited

theuni commented Oct 30, 2023

fanquake commented Oct 30, 2023 •

edited

theuni commented Oct 30, 2023 •

edited

theuni commented Oct 30, 2023

fanquake commented Oct 30, 2023 •

edited

jonatack left a comment

TheCharlatan left a comment

hebasto left a comment •

edited

TheCharlatan left a comment

theuni Oct 31, 2023

fanquake Oct 31, 2023

theuni Oct 31, 2023

theuni Oct 31, 2023

theuni left a comment

jonatack commented Oct 31, 2023

		if test "$have_miniupnpc" != "no"; then
		AC_CHECK_LIB([miniupnpc], [upnpDiscover], [MINIUPNPC_LIBS="$MINIUPNPC_LIBS -lminiupnpc"], [have_miniupnpc=no], [$MINIUPNPC_LIBS])

build: remove duplicate -lminiupnpc linking #28755

build: remove duplicate -lminiupnpc linking #28755

Conversation

fanquake commented Oct 30, 2023

DrahtBot commented Oct 30, 2023 • edited

Code Coverage

Reviews

theuni commented Oct 30, 2023

fanquake commented Oct 30, 2023 • edited

theuni commented Oct 30, 2023 • edited

theuni commented Oct 30, 2023

fanquake commented Oct 30, 2023 • edited

jonatack left a comment

Choose a reason for hiding this comment

TheCharlatan left a comment

Choose a reason for hiding this comment

hebasto left a comment • edited

Choose a reason for hiding this comment

TheCharlatan left a comment

Choose a reason for hiding this comment

theuni Oct 31, 2023

Choose a reason for hiding this comment

fanquake Oct 31, 2023

Choose a reason for hiding this comment

theuni Oct 31, 2023

Choose a reason for hiding this comment

theuni Oct 31, 2023

Choose a reason for hiding this comment

theuni left a comment

Choose a reason for hiding this comment

jonatack commented Oct 31, 2023

build: remove duplicate `-lminiupnpc` linking #28755

build: remove duplicate `-lminiupnpc` linking #28755

DrahtBot commented Oct 30, 2023 •

edited

fanquake commented Oct 30, 2023 •

edited

theuni commented Oct 30, 2023 •

edited

fanquake commented Oct 30, 2023 •

edited

hebasto left a comment •

edited