cmake: add a helper for linking into static libs #1678
Conversation
|
For reference, here's a branch demonstrating Core's usage of it for the kernel: https://github.com/theuni/bitcoin/commits/static_kernel/ |
|
Obvious Concept ACK if Bitcoin Core needs this But what's the status on the Core side of things? Has it been decided yet that kernel will use this? If not, we may want to wait. If Core doesn't need it, I'm not sure if we should include this feature. It's rather niche, and a bit of a layer violation. |
|
A few notes:
|
This came up as a real-world frustration during last week's working group call. Ping @TheCharlatan @stickies-v @fanquake @hebasto for their thoughts. |
Btw, I agree that this is a layer violation of sorts, but it's intended to avoid a worse one in Core. The alternative is for us to specify secp's internals at that layer: theuni/bitcoin@4f43838 |
Looking for possible alternatives. |
|
Concept ACK. I would not consider this approach a layer violation. An However, some explanatory documentation or comments would be useful. |
They do. But this is effective only at the linking stage, which is not the case for a "combined" static library. |
I believe this explanation may not be accurate. To my knowledge, CMake does not allow |
src/CMakeLists.txt
Outdated
| @@ -10,13 +10,19 @@ add_library(secp256k1_precomputed OBJECT EXCLUDE_FROM_ALL | |||
| # from being exported. | |||
| add_library(secp256k1 secp256k1.c $<TARGET_OBJECTS:secp256k1_precomputed>) | |||
|
|
|||
| # Create a helper lib that parent projects can use to link secp256k1 into a | |||
| # static lib. | |||
| add_library(secp256k1_objs INTERFACE EXCLUDE_FROM_ALL) | |||
There was a problem hiding this comment.
Because the SOURCES property of the secp256k1_objs target is not set, it is not an actual build target, so the EXCLUDE_FROM_ALL option may be omitted.
There was a problem hiding this comment.
Rebase on top of #1675?
just merged that one
|
Also some duplication might be avoided: --- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -37,16 +37,17 @@ endif()
get_target_property(use_pic secp256k1 POSITION_INDEPENDENT_CODE)
set_target_properties(secp256k1_precomputed PROPERTIES POSITION_INDEPENDENT_CODE ${use_pic})
-target_include_directories(secp256k1 INTERFACE
- # Add the include path for parent projects so that they don't have to manually add it.
+set(_include_directory_for_parent_projects
$<BUILD_INTERFACE:$<$<NOT:$<BOOL:${PROJECT_IS_TOP_LEVEL}>>:${PROJECT_SOURCE_DIR}/include>>
+)
+target_include_directories(secp256k1 INTERFACE
+ ${_include_directory_for_parent_projects}
$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>
)
-
target_include_directories(secp256k1_objs INTERFACE
- # Add the include path for parent projects so that they don't have to manually add it.
- $<BUILD_INTERFACE:$<$<NOT:$<BOOL:${PROJECT_IS_TOP_LEVEL}>>:${PROJECT_SOURCE_DIR}/include>>
+ ${_include_directory_for_parent_projects}
)
+unset(_include_directory_for_parent_projects)
# This emulates Libtool to make sure Libtool and CMake agree on the ABI version,
# see below "Calculate the version variables" in build-aux/ltmain.sh. |
Yes, in this case I meant that linking |
Nice. Also, after #1679 this could just be: set_target_properties(secp256k1_objs PROPERTIES $<TARGET_PROPERTY:secp256k1, INTERFACE_INCLUDE_DIRECTORIES>) |
Concept ACK, this would be very helpful for py-bitcoinkernel. I'm in the process of porting the library to use the kernel C++ header, whereby I have to link against a static bitcoinkernel. Not having to also link to kernel's dependencies makes that process a lot more elegant, as tested with a patch very similar to theuni/bitcoin@bc2aea3. |
|
Addressed @hebasto's feedback. |
|
Rebased. |
|
Maybe add this PR to the 0.7.0 milestone? |
Okay, makes sense given that Bitcoin Core will need it. Added. |
Parent projects (Bitcoin Core in this case) may wish to include secp256k1 in another static library (libbitcoinkernel) so that users are not forced to bring their own static libsecp256k1. Unfortunately, CMake lacks the machinery to link (combine) one static lib into another. To work around this, secp256k1_objs is exposed as an interface library which parent projects can "link" into static libs..
51d2ecd to
145ae3e
Compare
|
Rebased and updated. A (non-rebased) version of this can be seen/tested here: https://github.com/theuni/bitcoin/commits/static_kernel/ Building Core with |
There was a problem hiding this comment.
ACK 145ae3e, tested on Ubuntu 24.04 using cmake 3.22.6 and the default cmake 3.28.3.
Because the
SOURCESproperty of thesecp256k1_objstarget is not set, it is not an actual build target...
Perhaps it would be clearer if this target were named secp256k1_interface_objs or something similar?
| $<BUILD_INTERFACE:$<$<NOT:$<BOOL:${PROJECT_IS_TOP_LEVEL}>>:${PROJECT_SOURCE_DIR}/include>> | ||
| ) | ||
| set_target_properties(secp256k1_objs PROPERTIES | ||
| INTERFACE_INCLUDE_DIRECTORIES "$<TARGET_PROPERTY:secp256k1,INTERFACE_INCLUDE_DIRECTORIES>" |
There was a problem hiding this comment.
This generator expression has a behaviour change in version 3.26, but it doesn't affect us.
|
@hebasto Mmm, I'm not sure that's any clearer. I also considered I think I'd prefer to leave it as-is unless you feel strongly. |
That's technically correct. The Additionally, from a downstream project developer’s perspective, take Bitcoin Core as an example: add_library(bitcoinkernel
bitcoinkernel.cpp
...
$<TARGET_OBJECTS:bitcoin_clientversion>
$<TARGET_OBJECTS:bitcoin_crypto>
$<TARGET_OBJECTS:leveldb>
$<TARGET_OBJECTS:crc32c>
)
target_link_libraries(bitcoinkernel
PRIVATE
core_interface
secp256k1_objs
$<$<PLATFORM_ID:Windows>:bcrypt>
$<TARGET_NAME_IF_EXISTS:USDT::headers>
PUBLIC
Boost::headers
)You can build all The new library clearly belongs to that latter group:
I don’t consider my comment to be a blocker. |
There was a problem hiding this comment.
Light ACK 145ae3e
I tested (macOS 15.4, cmake 4.0.2) by subtree-ing this branch into https://github.com/TheCharlatan/bitcoin/tree/kernelApi_Cpp_Internal_Headers and cherry-picking theuni/bitcoin@23c5503 on top of it. Statically linking against this monolith works fine for me.
Light ACK because I only have superficial cmake knowledge and almost no knowledge of the secp256k1 repo. I just wanted to test if this works for my use case, and it does.
|
@hebasto I don't disagree with any of the above, I just don't think it fits with the |
|
Given that this is a feature, I wonder if this should get a changelog entry. I've added the label for now. But I don't feel competent enough in CMake to write one. @theuni Can you give it a try? |
As discussed here: #1674 (comment)
Parent projects (Bitcoin Core in this case) may wish to include secp256k1 in another static library (libbitcoinkernel) so that users are not forced to bring their own static libsecp256k1.
Unfortunately, CMake lacks the machinery to link (combine) one static lib into another.
To work around this, secp256k1_objs is exposed as an interface library which parent projects can "link" into static libs.