cmake: Avoid overlinking #34

hebasto · 2023-10-19T15:23:25Z

Additionally, this PR fixes "ld: warning: ignoring duplicate libraries" on macOS:

[....] Linking CXX executable test_bitcoin
ld: warning: ignoring duplicate libraries: '../../libleveldb.a', '../libbitcoin_common.a', '../univalue/libunivalue.a', '../util/libbitcoin_util.a'

[....] Linking CXX executable bench_bitcoin
ld: warning: ignoring duplicate libraries: '../../libleveldb.a', '../univalue/libunivalue.a', '../wallet/libbitcoin_wallet.a'

The third commit makes the bitcoin_wallet library a direct dependency of bitcoind rather bitcoin_node, that is outlined in https://github.com/bitcoin/bitcoin/blob/master/doc/design/libraries.md.

fanquake · 2023-10-19T15:28:17Z

If these exist in master, why do they need to be fixed here (at least, at this point)? Doesn't that just make the difference in behaviour between master, and the cmake branches larger, and potentially harder to review?

I would expect changes to land in master first, and then incorporate them here?

hebasto · 2023-10-19T15:33:40Z

If these exist in master...

~~It does.~~ UPD. See #34 (comment)

... why do they need to be fixed here (at least, at this point)?

It can be done at any point. It can be postponed, of course.

Doesn't that just make the difference in behaviour between master, and the cmake branches larger, and potentially harder to review?

The duplicated libraries, which are passed to a linker, do not affect the resulted binaries, don't they?

I would expect changes to land in master first, and then incorporate them here?

It could be not so easy as in here.

hebasto · 2023-10-26T15:27:18Z

Rebased.

theuni · 2023-10-26T18:04:37Z

If I understand correctly, the duplicated libs don't happen in master because we don't use transitive linking for deps. So in master, libbitcoin_zmq (for example) has no dependencies, where in CMake it does.

From what I can tell, this is simply cleaning up where those dependencies were over-specified for CMake?

hebasto · 2023-10-26T18:11:21Z

If I understand correctly, the duplicated libs don't happen in master...

It happens in master.

(at least on macOS with the recent Xcode)

hebasto · 2023-10-26T18:12:47Z

From what I can tell, this is simply cleaning up where those dependencies were over-specified for CMake?

~~That's correct.~~ UPD. See #34 (comment)

theuni · 2023-10-26T20:32:57Z

If I understand correctly, the duplicated libs don't happen in master...

It happens in master.

(at least on macOS with the recent Xcode)

Could you please give an an example of exactly what is duplicated in master? I agree with @fanquake that if there's some broken behavior we don't want to carry forward, we should fix it upstream first.

So it seems, then, that this is a mix of upsteram fixes and CMake fixups? Please help us to understand what's happening here :)

hebasto · 2023-10-27T13:26:11Z

I have to admit that my previous comments were not as precise as they should be. I'm sorry about that.

I double checked the master branch on macOS Sonoma. There indeed are ld: warning: ignoring duplicate libraries warnings. But they differs from ones reported in the PR description. And they have different causes.

For example, the following code in the master branch:

bitcoin/configure.ac

Lines 1430 to 1434 in 9477ad9

    
           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])], 
        
             [have_miniupnpc=no] 
        
           )

expands the AC_CHECK_LIB macro three times:

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

which ends with:

$ make print-MINIUPNPC_LIBS
MINIUPNPC_LIBS=-lminiupnpc -lminiupnpc -lminiupnpc

Another duplicated linked library is -levent that is caused by:

$ make print-EVENT_PTHREADS_LIBS
EVENT_PTHREADS_LIBS=-levent_pthreads -levent
$ make print-EVENT_LIBS
EVENT_LIBS=-levent

From what I can tell, this is simply cleaning up where those dependencies were over-specified for CMake?

This PR is CMake-specific and fixes my own mistakes such as over-specified libraries to link and link order.

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.

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

Opened to bitcoin#28755 to remove some of the unrelated warnings.

hebasto · 2023-10-31T10:50:41Z

Rebased.

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.

hebasto · 2023-10-31T20:31:09Z

Rebased.

…nking b74e449 build: remove potential for duplciate natpmp linking (fanquake) 4e95096 build: remove duplicate -lminiupnpc linking (fanquake) Pull request description: 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 macOS linker released with Xcode 15, and also came up in hebasto/bitcoin#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. ACKs for top commit: jonatack: ACK b74e449 hebasto: ACK b74e449, it fixes one issue mentioned in hebasto/bitcoin#34 (comment). TheCharlatan: ACK b74e449 theuni: ACK b74e449 Tree-SHA512: 987a56ef17cbaf273cb672c41016f3f615b16889317325a9e88135d0c41f01af3840ad44a6f811a7df97f5873c9cd957e60aaa1b99bd408b17b4b1ffe2c68f36

theuni

Thanks, makes more sense after dropping the unrelated upstream stuff.

src/bench/CMakeLists.txt

theuni · 2023-11-07T14:29:35Z

src/zmq/CMakeLists.txt

@@ -16,7 +16,6 @@ target_compile_definitions(bitcoin_zmq
 target_link_libraries(bitcoin_zmq
  PRIVATE
    core
-    leveldb


Totally unrelated to CMake/this PR, but univalue is an unfortunate dependency for zmq. Seems to me it'd make sense to move zmq/rpc.cpp to rpc/zmq.cpp :)

hebasto · 2023-11-07T16:23:56Z

Rebased.

theuni

Looks good other than the comment about the added deps. I'm confused why this works without them.

theuni · 2023-11-07T16:57:51Z

src/bench/CMakeLists.txt

-  leveldb
+  bitcoin_node
+  $<TARGET_NAME_IF_EXISTS:bitcoin_wallet>
+  bitcoin_common


Why the new deps (bitcoin_common, bitcoin_consensus, etc)? Were they necessary before?

Otherwise, the "duplicate libraries" warnings still being emitted. Besides, both test_bitcoin and bench_bitcoin targets do use symbols from the added libraries.

I still don't understand. libs are added here but none are removed. How does that eliminate any dupes?

theuni · 2023-11-07T17:03:17Z

src/test/CMakeLists.txt

@@ -16,7 +16,6 @@ generate_header_from_raw(data/asmap.raw)

 add_executable(test_bitcoin
  main.cpp
-  $<TARGET_OBJECTS:bitcoin_consensus>


What's this change about?

Before CMake 3.12, the only way to consume the object library--which is a mere unarchived collection of object files, not a real library--was to add $<TARGET_OBJECTS:bitcoin_consensus> to sources. Now we can use an idiomatic way: target_link_libraries(test_bitcoin bitcoin_consensus).

In short, it is a modernization after bumping the minimum required CMake version from 3.10 to 3.16.

hebasto · 2023-11-09T15:15:01Z

Reworked. PR title and description have been updated.

theuni · 2023-11-09T15:42:00Z

I'm having trouble understanding several of the changes here. Deps are hopping around from libs to binaries.

Could you please add some extra info in the commit messages about what's happening so we reviewers don't have to try to interpret your changes?

Drop unneeded dependency.

hebasto · 2023-11-09T15:48:42Z

I'm having trouble understanding several of the changes here. Deps are hopping around from libs to binaries.

Could you please add some extra info in the commit messages about what's happening so we reviewers don't have to try to interpret your changes?

Done. Sorry for the mess :)

theuni · 2023-11-09T16:21:38Z

I'm having trouble understanding several of the changes here. Deps are hopping around from libs to binaries.
Could you please add some extra info in the commit messages about what's happening so we reviewers don't have to try to interpret your changes?

Done.

Ahhh. So much better :)

Please consider this a request to do the same for future fixups.

theuni

I haven't tested nor compared to the upstream deps, but changes make sense to me now that they have context.

Left one more question.

theuni · 2023-11-09T15:54:19Z

src/CMakeLists.txt

@@ -220,7 +220,6 @@ add_library(bitcoin_node STATIC EXCLUDE_FROM_ALL
 )
 if(ENABLE_WALLET)
  target_sources(bitcoin_node PRIVATE wallet/init.cpp)
-  target_link_libraries(bitcoin_node PRIVATE bitcoin_wallet)


Any reason not to go ahead and use TARGET_IF_NAME_EXISTS for target_sources here as well? Seems odd to leave only one behind.

Thanks! Done.

theuni · 2023-11-09T16:04:34Z

src/test/util/CMakeLists.txt

    Boost::headers
+  PUBLIC
+    univalue


I was going to comment that I'd rather see this be PRIVATE and have univalue included by the binaries as needed, but test_util pretty clearly exports univalue as part of its interface, so PUBLIC univalue actually follows the code as-is quite well I think.

Placing univalue into usage requirements, and removing it from build specifications for the bench_bitcoin and test_bitcoin targets, fixes actually, "ld: warning: ignoring duplicate libraries" on macOS.

Makes the `bitcoin_wallet` library a direct dependency of `bitcoind` rather `bitcoin_node`, that is outlined in bitcoin/bitcoin@master/doc/design/libraries.md.

1) Drop unneeded dependencies in `test_util`. 2) Adjust dependencies for `bench_bitcoin` (removed uneeded ones and add `bitcoin_node`, required after step (1).

Drop unneeded dependency.

theuni

ACK 8342981

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/bitcoin#34.

hebasto mentioned this pull request Oct 19, 2023

Rebased cmake-staging branch (post PRs 17, 18, 19) #31

Closed

hebasto force-pushed the cmake-staging branch from 618eec0 to f4162d0 Compare October 26, 2023 10:44

hebasto force-pushed the 231019-cmake-M branch from 8b53050 to 79a3b38 Compare October 26, 2023 15:26

hebasto force-pushed the 231019-cmake-M branch from 79a3b38 to 9477ad9 Compare October 26, 2023 15:48

fanquake mentioned this pull request Oct 30, 2023

build: remove duplicate -lminiupnpc linking bitcoin/bitcoin#28755

Merged

hebasto force-pushed the 231019-cmake-M branch from 9477ad9 to 8ecc77d Compare October 31, 2023 10:50

hebasto force-pushed the cmake-staging branch from 6e20a0f to d86340b Compare October 31, 2023 20:19

hebasto force-pushed the 231019-cmake-M branch from 8ecc77d to 16487e9 Compare October 31, 2023 20:30

theuni reviewed Nov 7, 2023

View reviewed changes

hebasto force-pushed the cmake-staging branch from 1a00d05 to 7dc3874 Compare November 7, 2023 16:20

hebasto force-pushed the 231019-cmake-M branch from 16487e9 to 80c585c Compare November 7, 2023 16:23

theuni reviewed Nov 7, 2023

View reviewed changes

hebasto force-pushed the 231019-cmake-M branch from 80c585c to 7d5af55 Compare November 9, 2023 14:55

hebasto changed the title ~~cmake: Fix "ld: warning: ignoring duplicate libraries" on macOS~~ cmake: Avoid overlinking Nov 9, 2023

hebasto added 2 commits November 9, 2023 15:43

fixup! cmake: Build bitcoin_util static library

0f0dcca

Drop unneeded dependency.

fixup! cmake: Add libzmq optional package support

e4b4fdd

Drop unneeded dependency.

hebasto force-pushed the 231019-cmake-M branch from 7d5af55 to b2610a4 Compare November 9, 2023 15:47

theuni reviewed Nov 9, 2023

View reviewed changes

hebasto added 3 commits November 9, 2023 17:07

fixup! cmake: Add wallet functionality

9a0a914

Makes the `bitcoin_wallet` library a direct dependency of `bitcoind` rather `bitcoin_node`, that is outlined in bitcoin/bitcoin@master/doc/design/libraries.md.

fixup! cmake: Build bench_bitcoin executable

ab9853a

1) Drop unneeded dependencies in `test_util`. 2) Adjust dependencies for `bench_bitcoin` (removed uneeded ones and add `bitcoin_node`, required after step (1).

fixup! cmake: Build test_bitcoin executable

8342981

Drop unneeded dependency.

hebasto force-pushed the 231019-cmake-M branch from b2610a4 to 8342981 Compare November 9, 2023 17:07

theuni approved these changes Nov 9, 2023

View reviewed changes

hebasto merged commit d3f3967 into cmake-staging Nov 9, 2023
6 checks passed

l0rinc mentioned this pull request Aug 12, 2024

cmake, doc: Update test/README.md and other docs #324

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmake: Avoid overlinking #34

cmake: Avoid overlinking #34

hebasto commented Oct 19, 2023 •

edited

Loading

fanquake commented Oct 19, 2023

hebasto commented Oct 19, 2023 •

edited

Loading

hebasto commented Oct 26, 2023

theuni commented Oct 26, 2023

hebasto commented Oct 26, 2023

hebasto commented Oct 26, 2023 •

edited

Loading

theuni commented Oct 26, 2023

hebasto commented Oct 27, 2023 •

edited

Loading

fanquake commented Oct 30, 2023

hebasto commented Oct 31, 2023

hebasto commented Oct 31, 2023

theuni left a comment

theuni Nov 7, 2023

hebasto commented Nov 7, 2023

theuni left a comment

theuni Nov 7, 2023

hebasto Nov 7, 2023

theuni Nov 7, 2023

hebasto Nov 9, 2023

theuni Nov 7, 2023

hebasto Nov 7, 2023 •

edited

Loading

hebasto commented Nov 9, 2023

theuni commented Nov 9, 2023

hebasto commented Nov 9, 2023

theuni commented Nov 9, 2023

theuni left a comment

theuni Nov 9, 2023

hebasto Nov 9, 2023

theuni Nov 9, 2023

hebasto Nov 9, 2023

theuni left a comment

cmake: Avoid overlinking #34

cmake: Avoid overlinking #34

Conversation

hebasto commented Oct 19, 2023 • edited Loading

fanquake commented Oct 19, 2023

hebasto commented Oct 19, 2023 • edited Loading

hebasto commented Oct 26, 2023

theuni commented Oct 26, 2023

hebasto commented Oct 26, 2023

hebasto commented Oct 26, 2023 • edited Loading

theuni commented Oct 26, 2023

hebasto commented Oct 27, 2023 • edited Loading

fanquake commented Oct 30, 2023

hebasto commented Oct 31, 2023

hebasto commented Oct 31, 2023

theuni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hebasto commented Nov 7, 2023

theuni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hebasto Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

hebasto commented Nov 9, 2023

theuni commented Nov 9, 2023

hebasto commented Nov 9, 2023

theuni commented Nov 9, 2023

theuni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theuni left a comment

Choose a reason for hiding this comment

hebasto commented Oct 19, 2023 •

edited

Loading

hebasto commented Oct 19, 2023 •

edited

Loading

hebasto commented Oct 26, 2023 •

edited

Loading

hebasto commented Oct 27, 2023 •

edited

Loading

hebasto Nov 7, 2023 •

edited

Loading