cmake: add a helper for linking into static libs

[theuni]

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.

[theuni]

For reference, here's a branch demonstrating Core's usage of it for the kernel: https://github.com/theuni/bitcoin/commits/static_kernel/

[real-or-random]

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.

[theuni]

A few notes:

• Object libraries are frustrating and don't propagate dependencies as one would hope.
• Linking against $<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.
• One/some/all of the binaries could be switched to use secp256k1_objs as a test. As an added benefit, that would avoid runtime path issues when building as a shared lib.

[theuni]

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.

[theuni]

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: theuni/bitcoin@4f43838

[hebasto]

Ping ... @hebasto for their thoughts.

Looking for possible alternatives.

[hebasto]

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.

[hebasto]

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.

[hebasto]

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

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.

[hebasto]

This command fails with CMake 3.16:

CMake Error at src/CMakeLists.txt:15 (add_library):
  add_library INTERFACE library may not be used with EXCLUDE_FROM_ALL.

This signature has been added in CMake 3.19.

Rebase on top of #1675?

[hebasto]

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.

[real-or-random]

Rebase on top of #1675?

just merged that one

[hebasto]

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.

[theuni]

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

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.

Yes, in this case I meant that linking secp256k1_asm into secp256k1_objs caused this problem. Otherwise that approach would've bee preferred.

[theuni]

Also some duplication might be avoided:

Nice. Also, after #1679 this could just be:

set_target_properties(secp256k1_objs PROPERTIES $<TARGET_PROPERTY:secp256k1, INTERFACE_INCLUDE_DIRECTORIES>)

[stickies-v]

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 theuni/bitcoin@bc2aea3.

[theuni]

Addressed @hebasto's feedback.

[theuni]

Rebased.

[hebasto]

ACK 51d2ecd. Tested using this branch by linking the executable against both the shared and static versions of the bitcoinkernel library.

[hebasto]

Also some duplication might be avoided:

Nice. Also, after #1679 this could just be:

set_target_properties(secp256k1_objs PROPERTIES $<TARGET_PROPERTY:secp256k1, INTERFACE_INCLUDE_DIRECTORIES>)

#1679 has just been merged.

[hebasto]

@real-or-random

Maybe add this PR to the 0.7.0 milestone?

[real-or-random]

Maybe add this PR to the 0.7.0 milestone?

Okay, makes sense given that Bitcoin Core will need it. Added.

[theuni]

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.

[hebasto]

ACK 145ae3e, tested on Ubuntu 24.04 using cmake 3.22.6 and the default cmake 3.28.3.

Because the SOURCES property of the secp256k1_objs target 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?

[hebasto]

This generator expression has a behaviour change in version 3.26, but it doesn't affect us.

[theuni]

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

I think I'd prefer to leave it as-is unless you feel strongly.

[hebasto]

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:

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 bitcoinkernel prerequisites except core_interface:

$ cmake --build build -t bitcoin_clientversion bitcoin_crypto leveldb crc32c
$ cmake --build build -t core_interface
ninja: error: unknown target 'core_interface'

The new library clearly belongs to that latter group:

$ cmake --build build -t secp256k1_objs
ninja: 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.

[hebasto]

cc @TheCharlatan @stickies-v

[stickies-v]

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.

[theuni]

@hebasto I don't disagree with any of the above, I just don't think it fits with the _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.

[real-or-random]

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?