build: remove duplicate `-lminiupnpc` linking #28755

pull fanquake wants to merge 2 commits into bitcoin:master from fanquake:remove_duplicate_linking changing 1 files +11 −10
  1. fanquake commented at 3:00 PM on October 30, 2023: member

    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 https://github.com/hebasto/bitcoin/pull/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.

  2. DrahtBot commented at 3:00 PM on October 30, 2023: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage

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

    <!--021abf342d371248e50ceaed478a90ca-->

    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.

  3. DrahtBot added the label Build system on Oct 30, 2023
  4. fanquake requested review from TheCharlatan on Oct 30, 2023
  5. theuni commented at 5:45 PM on October 30, 2023: member

    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]
   )
  6. fanquake commented at 5:49 PM on October 30, 2023: member

    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.

  7. theuni commented at 6:00 PM on October 30, 2023: member

    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 :)

  8. theuni commented at 6:03 PM on October 30, 2023: member

    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 :)

  9. fanquake commented at 7:21 PM on October 30, 2023: member

    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.

  10. jonatack commented at 9:15 PM on October 30, 2023: member

    Lightly tested ACK 33fe16be67daaf46d6aafdf61a3c937b4a0d70ef

    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:

    <details><summary>from this (on master)</summary><p>

    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'.

    </p></details>

    <details><summary>to this (on this branch)</summary><p>

    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'.

    </p></details>

  11. TheCharlatan approved
  12. TheCharlatan commented at 9:22 AM on October 31, 2023: contributor

    ACK 33fe16be67daaf46d6aafdf61a3c937b4a0d70ef

  13. in configure.ac:1428 in 33fe16be67 outdated 
    1428 | -    [miniupnpc/miniupnpc.h miniupnpc/upnpcommands.h miniupnpc/upnperrors.h],
1429 | -    [AC_CHECK_LIB([miniupnpc], [upnpDiscover], [MINIUPNPC_LIBS="$MINIUPNPC_LIBS -lminiupnpc"], [have_miniupnpc=no], [$MINIUPNPC_LIBS])],
1430 | -    [have_miniupnpc=no]
1431 | -  )
1432 | +  AC_CHECK_HEADERS([miniupnpc/miniupnpc.h miniupnpc/upnpcommands.h miniupnpc/upnperrors.h])
1433 | +  AC_CHECK_LIB([miniupnpc], [upnpDiscover], [MINIUPNPC_LIBS="$MINIUPNPC_LIBS -lminiupnpc"], [have_miniupnpc=no], [$MINIUPNPC_LIBS])
    hebasto commented at 10:35 AM on October 31, 2023:

    Running AC_CHECK_LIB unconditionally does not make sense. Maybe gate it with if test "$have_miniupnpc" != "no", where have_miniupnpc is set to no in "not-found" branch of the AC_CHECK_HEADERS macro?

    fanquake commented at 10:42 AM on October 31, 2023:

    I will just change this, along with the natpmp logic at the same time.

  14. fanquake force-pushed on Oct 31, 2023
  15. in configure.ac:1461 in 9a823890be outdated 
    1459 | -                   [AC_CHECK_LIB([natpmp], [initnatpmp], [NATPMP_LIBS="$NATPMP_LIBS -lnatpmp"], [have_natpmp=no], [$NATPMP_LIBS])],
1460 | -                   [have_natpmp=no])
1461 | +  AC_CHECK_HEADERS([natpmp.h], [], [have_natpmp=no])
1462 | +
1463 | +  if test "$have_natpmp" != "no"; then
1464 | +    AC_CHECK_LIB([natpmp], [initnatpmp], [NATPMP_LIBS="$NATPMP_LIBS -lnatpmp"], [], [$NATPMP_LIBS])
    hebasto commented at 11:05 AM on October 31, 2023:

    The AC_CHECK_LIB macro still have to set have_natpmp=no if the library not found, as it is used later: https://github.com/bitcoin/bitcoin/blob/9a823890be9a6b4edfe6f32890f61aba9605dbbc/configure.ac#L1750

    Same for have_miniupnpc: https://github.com/bitcoin/bitcoin/blob/9a823890be9a6b4edfe6f32890f61aba9605dbbc/configure.ac#L1729

    fanquake commented at 11:11 AM on October 31, 2023:

    I think this is covering an edge case that is very unlikely to happen, but I'll add it in any case.

  16. build: remove duplicate -lminiupnpc linking
    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 https://github.com/hebasto/bitcoin/pull/34.
    4e95096952
  17. build: remove potential for duplciate natpmp linking
    Consolidate the lib checking logic to be the same as miniupnpc.
    b74e449ffa
  18. fanquake force-pushed on Oct 31, 2023
  19. hebasto approved
  20. hebasto commented at 11:30 AM on October 31, 2023: member

    ACK b74e449ffa9965f18037f0322ea178e2fe1dbc18, it fixes one issue mentioned in https://github.com/hebasto/bitcoin/pull/34#issuecomment-1782914787.

    ./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
...
  21. DrahtBot requested review from TheCharlatan on Oct 31, 2023
  22. DrahtBot requested review from jonatack on Oct 31, 2023
  23. TheCharlatan approved
  24. TheCharlatan commented at 1:31 PM on October 31, 2023: contributor

    ACK b74e449ffa9965f18037f0322ea178e2fe1dbc18

  25. in configure.ac:1460 in b74e449ffa 
    1454 | @@ -1455,9 +1455,12 @@ dnl Check for libnatpmp (optional).
1455 |  if test "$use_natpmp" != "no"; then
1456 |    TEMP_CPPFLAGS="$CPPFLAGS"
1457 |    CPPFLAGS="$CPPFLAGS $NATPMP_CPPFLAGS"
1458 | -  AC_CHECK_HEADERS([natpmp.h],
1459 | -                   [AC_CHECK_LIB([natpmp], [initnatpmp], [NATPMP_LIBS="$NATPMP_LIBS -lnatpmp"], [have_natpmp=no], [$NATPMP_LIBS])],
1460 | -                   [have_natpmp=no])
1461 | +  AC_CHECK_HEADERS([natpmp.h], [], [have_natpmp=no])
1462 | +
1463 | +  if test "$have_natpmp" != "no"; then
    theuni commented at 5:05 PM on October 31, 2023:

    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.

    fanquake commented at 5:06 PM on October 31, 2023:

    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.

    theuni commented at 5:08 PM on October 31, 2023:

    Lol, you trying to tell me something? :)

  26. in configure.ac:1430 in 4e95096952 outdated 
    1432 | +  AC_CHECK_HEADERS([miniupnpc/miniupnpc.h miniupnpc/upnpcommands.h miniupnpc/upnperrors.h], [], [have_miniupnpc=no])
1433 |  
1434 | -  dnl The minimum supported miniUPnPc API version is set to 17. This excludes
1435 | -  dnl versions with known vulnerabilities.
1436 |    if test "$have_miniupnpc" != "no"; then
1437 | +    AC_CHECK_LIB([miniupnpc], [upnpDiscover], [MINIUPNPC_LIBS="$MINIUPNPC_LIBS -lminiupnpc"], [have_miniupnpc=no], [$MINIUPNPC_LIBS])
    theuni commented at 5:07 PM on October 31, 2023:

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

  27. theuni approved
  28. theuni commented at 5:08 PM on October 31, 2023: member

    ACK b74e449ffa9965f18037f0322ea178e2fe1dbc18

  29. jonatack commented at 11:31 PM on October 31, 2023: member

    ACK b74e449ffa9965f18037f0322ea178e2fe1dbc18

    Build output diff is the same as my previous #28755#pullrequestreview-1705089533, and with the latest push also see a similar improvement as reported by @hebasto in #28755#pullrequestreview-1706070279:

    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
  30. DrahtBot removed review request from jonatack on Oct 31, 2023
  31. fanquake merged this on Nov 1, 2023
  32. fanquake closed this on Nov 1, 2023
  33. fanquake deleted the branch on Nov 1, 2023
  34. bitcoin locked this on Oct 31, 2024
Contributors
fanquakeDrahtBottheunijonatackTheCharlatanhebasto
Labels
Build system
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-26 06:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me