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.
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:
$<TARGET_OBJECTS:foo> doesn’t actually pull the objects into static libs, hence their inclusion as interface sources.secp256k1_asm_arm is used directly rather than secp256k1_asm as otherwise the public asm/field_10x26_arm.s bubbles up to Core and requires us to enable the ASM language.secp256k1_objs as a test. As an added benefit, that would avoid runtime path issues when building as a shared lib.
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.
This came up as a real-world frustration during last week’s working group call.
Ping @TheCharlatan @stickies-v @fanquake @hebasto for their thoughts.
It’s rather niche, and a bit of a layer violation.
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: https://github.com/theuni/bitcoin/commit/4f43838247fa62b8495133b3924ebb64f08dd0ec
Concept ACK.
I would not consider this approach a layer violation. An INTERFACE library is used to provide the interface, including include directories, between a subproject and its parent project when the latter integrates the former within the same build system, as is the case with libsecp256k1 and Bitcoin Core.
However, some explanatory documentation or comments would be useful.
A few notes:
- Object libraries are frustrating and don’t propagate dependencies as one would hope.
They do. But this is effective only at the linking stage, which is not the case for a “combined” static library.
secp256k1_asm_armis used directly rather thansecp256k1_asmas otherwise the publicasm/field_10x26_arm.sbubbles up to Core and requires us to enable theASMlanguage.
I believe this explanation may not be accurate. To my knowledge, CMake does not allow INTERFACE libraries, such as secp256k1_asm, in $<TARGET_OBJECTS:...> generator expressions.
9@@ -10,13 +10,19 @@ add_library(secp256k1_precomputed OBJECT EXCLUDE_FROM_ALL
10 # from being exported.
11 add_library(secp256k1 secp256k1.c $<TARGET_OBJECTS:secp256k1_precomputed>)
12
13+# Create a helper lib that parent projects can use to link secp256k1 into a
14+# static lib.
15+add_library(secp256k1_objs INTERFACE EXCLUDE_FROM_ALL)
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.
Rebase on top of #1675?
just merged that one
Also some duplication might be avoided:
0--- a/src/CMakeLists.txt
1+++ b/src/CMakeLists.txt
2@@ -37,16 +37,17 @@ endif()
3 get_target_property(use_pic secp256k1 POSITION_INDEPENDENT_CODE)
4 set_target_properties(secp256k1_precomputed PROPERTIES POSITION_INDEPENDENT_CODE ${use_pic})
5
6-target_include_directories(secp256k1 INTERFACE
7- # Add the include path for parent projects so that they don't have to manually add it.
8+set(_include_directory_for_parent_projects
9 $<BUILD_INTERFACE:$<$<NOT:$<BOOL:${PROJECT_IS_TOP_LEVEL}>>:${PROJECT_SOURCE_DIR}/include>>
10+)
11+target_include_directories(secp256k1 INTERFACE
12+ ${_include_directory_for_parent_projects}
13 $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>
14 )
15-
16 target_include_directories(secp256k1_objs INTERFACE
17- # Add the include path for parent projects so that they don't have to manually add it.
18- $<BUILD_INTERFACE:$<$<NOT:$<BOOL:${PROJECT_IS_TOP_LEVEL}>>:${PROJECT_SOURCE_DIR}/include>>
19+ ${_include_directory_for_parent_projects}
20 )
21+unset(_include_directory_for_parent_projects)
22
23 # This emulates Libtool to make sure Libtool and CMake agree on the ABI version,
24 # see below "Calculate the version variables" in build-aux/ltmain.sh.
secp256k1_asm_armis used directly rather thansecp256k1_asmas otherwise the publicasm/field_10x26_arm.sbubbles up to Core and requires us to enable theASMlanguage.I believe this explanation may not be accurate. To my knowledge, CMake does not allow
INTERFACElibraries, such assecp256k1_asm, in$<TARGET_OBJECTS:...>generator expressions.
Yes, in this case I meant that linking secp256k1_asm into secp256k1_objs caused this problem. Otherwise that approach would’ve bee preferred.
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.
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 https://github.com/theuni/bitcoin/commit/bc2aea3251602eb8f79356a63109573e5c37ea90.
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..
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 BUILD_KERNEL_LIB=ON and BUILD_SHARED_LIBS=OFF shows the secp objs getting linked in correctly.
85 target_include_directories(secp256k1 INTERFACE
86- # Add the include path for parent projects so that they don't have to manually add it.
87 $<BUILD_INTERFACE:$<$<NOT:$<BOOL:${PROJECT_IS_TOP_LEVEL}>>:${PROJECT_SOURCE_DIR}/include>>
88 )
89+set_target_properties(secp256k1_objs PROPERTIES
90+ INTERFACE_INCLUDE_DIRECTORIES "$<TARGET_PROPERTY:secp256k1,INTERFACE_INCLUDE_DIRECTORIES>"
ACK 145ae3e28d587dd04866ec5893be9270bc99289a, 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?
Mmm, I’m not sure that’s any clearer. I also considered
secp256k1_objs_interface, but that would lead one to believe that it has no sources/objects.
That’s technically correct. The SOURCE property of the new target is empty.
Additionally, from a downstream project developer’s perspective, take Bitcoin Core as an example:
0add_library(bitcoinkernel
1 bitcoinkernel.cpp
2 ...
3 $<TARGET_OBJECTS:bitcoin_clientversion>
4 $<TARGET_OBJECTS:bitcoin_crypto>
5 $<TARGET_OBJECTS:leveldb>
6 $<TARGET_OBJECTS:crc32c>
7)
8target_link_libraries(bitcoinkernel
9 PRIVATE
10 core_interface
11 secp256k1_objs
12 $<$<PLATFORM_ID:Windows>:bcrypt>
13 $<TARGET_NAME_IF_EXISTS:USDT::headers>
14 PUBLIC
15 Boost::headers
16)
You can build all bitcoinkernel prerequisites except core_interface:
0$ cmake --build build -t bitcoin_clientversion bitcoin_crypto leveldb crc32c
1$ cmake --build build -t core_interface
2ninja: error: unknown target 'core_interface'
The new library clearly belongs to that latter group:
0$ cmake --build build -t secp256k1_objs
1ninja: error: unknown target 'secp256k1_objs'
I think I’d prefer to leave it as-is unless you feel strongly.
I don’t consider my comment to be a blocker.
Light ACK 145ae3e28d587dd04866ec5893be9270bc99289a
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 https://github.com/theuni/bitcoin/commit/23c5503640a7e2073b7573e725b4d3a296a3bb08 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.
_interface suffix as we use it (mostly flags and dependency resolution), since linking against this lib does bring in objects, even if they’re not listed in TARGET_OBJECTS.