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