kernel: Create usable static kernel library

TheCharlatan commented at 3:30 pm on September 4, 2024: contributor

Since the move to cmake, the kernel static library that is installed after a cmake –install build is unusable. It lacks symbols for the internal libraries, besides those defined in the kernel library target.

Fix this by explicitly installing all the required internal static libraries. To make usage of these installed libraries easy, add a pkg-config file that can be used during linking.

This patch can be tested with:

0cmake -B build -DBUILD_SHARED_LIBS=OFF -DBUILD_KERNEL_LIB=ON
1cmake --build build
2cmake --install build
3g++ -std=c++20 -o test_chainstate src/bitcoin-chainstate.cpp -I/home/drgrid/bitcoin/src $(pkg-config --libs --static libbitcoinkernel)

Attempts to solve #30801

DrahtBot commented at 3:30 pm on September 4, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto, fanquake, ryanofsky

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30595 (kernel: Introduce initial C header API by TheCharlatan)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Validation on Sep 4, 2024

maflcko added the label DrahtBot Guix build requested on Sep 4, 2024

DrahtBot added the label CI failed on Sep 4, 2024

DrahtBot commented at 5:00 pm on September 4, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/29678813861

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

ryanofsky approved

ryanofsky commented at 5:03 pm on September 4, 2024: contributor

Code review ACK 192d71e312d0eab6cc1ce061edb34d9d338f7fec.

This seems like a reasonable approach, but I wonder if a more consistent way of doing this would just treat all the libraries the same as secp, installing them to the lib/ directory and listing them in libbitcoinkernel.pc. That way the secp library would not be an outlier, the TARGET_OBJECTS calls would not be needed, and build would be lighter, with each .o object file added to one library file .a instead of two.

TheCharlatan commented at 6:30 pm on September 4, 2024: contributor

Not sure yet why the CI fails, will investigate and leave this in draft for now. If it is something cmake version related, I’ll probably end up abandoning this approach and instead try to get the installation of all the required libraries working.

TheCharlatan marked this as a draft on Sep 4, 2024

TheCharlatan force-pushed on Sep 4, 2024

DrahtBot commented at 3:52 am on September 5, 2024: contributor

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

File	commit 93e48240bfdc25c2760d33da69e739ba1f92da9b(master)	commit 65bbfb70afb9a35dd86a7eb727225dcce58fffad(master and this pull)
SHA256SUMS.part	`faff1f20a1f0ea44...`	`d4745ad9db50eca4...`
*-aarch64-linux-gnu-debug.tar.gz	`55b62e6c0e59b2a8...`	`59e87f5a13a0146a...`
*-aarch64-linux-gnu.tar.gz	`a940e0b5a1eb5d1b...`	`4433e044dd1da917...`
*-arm-linux-gnueabihf-debug.tar.gz	`a1669ce2b0203443...`	`e15819962fec636c...`
*-arm-linux-gnueabihf.tar.gz	`c6bea27cfba05cfe...`	`765467a7b025264e...`
*-arm64-apple-darwin-unsigned.tar.gz	`159747a344e05db5...`	`7759e2de33a0db23...`
*-arm64-apple-darwin-unsigned.zip	`5d61a9edb7d9038f...`	`b48d2b04dabcc1c2...`
*-arm64-apple-darwin.tar.gz	`e0b67448eb3c9e2a...`	`be23064ea1c1fa14...`
*-powerpc64-linux-gnu-debug.tar.gz	`509a49458e5c73c4...`	`d06d8ad1554af032...`
*-powerpc64-linux-gnu.tar.gz	`4c81f08f8dff1bdc...`	`ebc0f7256c095082...`
*-riscv64-linux-gnu-debug.tar.gz	`df90cf8275a94c9d...`	`3605b6b835ed0397...`
*-riscv64-linux-gnu.tar.gz	`503f6b143accb855...`	`27f8626f91538b24...`
*-x86_64-apple-darwin-unsigned.tar.gz	`6eed05b3066e135d...`	`5bc1e494c4eed143...`
*-x86_64-apple-darwin-unsigned.zip	`99756eda2bdc3662...`	`5aeed650ad5d2826...`
*-x86_64-apple-darwin.tar.gz	`810fd3216e4a5150...`	`e9fa065b33e87226...`
*-x86_64-linux-gnu-debug.tar.gz	`b31717682ebbc725...`	`0d2b5192f1e8112d...`
*-x86_64-linux-gnu.tar.gz	`169ca01ffede0d0d...`	`418b323ab8ec090a...`
*.tar.gz	`6bacd64a750b6cb7...`	`884f03af8a4e9c6d...`
guix_build.log	`8fbdcdf57700e091...`	`7d4052dbbec8e372...`
guix_build.log.diff		`f9658d033e2a7905...`

DrahtBot removed the label DrahtBot Guix build requested on Sep 5, 2024

TheCharlatan commented at 5:50 am on September 5, 2024: contributor

Re-worked this, it now installs all the required libraries when the static kernel lib is being built. Couldn’t figure out why the previous method was working on my machines, but not in the CI container. Since it is not a standard thing to do (just about touching undefined behaviour territory), and some reviewers commented they would support installing all libraries as an alternative, this now seems like the better and future-proof solution to me.

TheCharlatan marked this as ready for review on Sep 5, 2024

DrahtBot removed the label CI failed on Sep 5, 2024

in src/kernel/CMakeLists.txt:125 in 7739fa6946 outdated

118+
119+  set(all_target_static_link_libs)
120+  get_target_static_link_libs(bitcoinkernel all_target_static_link_libs)
121+
122+  foreach(lib ${all_target_static_link_libs})
123+    install(TARGETS ${lib} ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR})

ryanofsky commented at 3:26 pm on September 5, 2024:

In commit “build: Produce a usable static kernel library” (7739fa69462f070c62345f2f14fcf817a512d59a)

Seems ilke in theory this could install the same libraries more than once because it is just recursively adding dependencies without deduplicating them. For example if kernel library depends on util and crypto, and util library depends on crypto, crypto library could be listed twice. May be worth iterating over a unique version of the list so the install step is cleaner if it does contain duplicates

TheCharlatan commented at 3:38 pm on September 5, 2024:

I was doing this, but removed it again from the patch. I feel like if there are duplicates, it is a bug in our library topology. But it could still make sense to guard against them.

ryanofsky commented at 3:41 pm on September 5, 2024:

Yeah I would leave alone for now to keep things simple. I don’t think it is a bug to have multiple libraries a target depends on depend on some of the same libraries themselves though.

ryanofsky approved

ryanofsky commented at 3:27 pm on September 5, 2024: contributor

Code review ACK 766881e33d13ca2887f7ed179cdcc6bc007a6325. I like this approach, seems more generic and lightweight.

hebasto commented at 3:39 pm on September 6, 2024: member

766881e33d13ca2887f7ed179cdcc6bc007a6325

While installing libbitcoinkernel.pc, why ignore CMake’s configs for downstream projects using CMake?

in src/kernel/CMakeLists.txt:103 in 766881e33d outdated

 97@@ -98,6 +98,39 @@ set_target_properties(bitcoinkernel PROPERTIES
 98   CXX_VISIBILITY_PRESET default
 99 )
100 
101+# When building the static library, install all static libraries the
102+# bitcoinkernel depends on.
103+if (NOT BUILD_SHARED_LIBS)

hebasto commented at 4:10 pm on September 6, 2024:

style nit: All other CMake code uses the if() command without an additional space.

hebasto commented at 4:10 pm on September 6, 2024: member

Approach ACK 766881e33d13ca2887f7ed179cdcc6bc007a6325.

in src/kernel/CMakeLists.txt:122 in 766881e33d outdated

117+  endfunction()
118+
119+  set(all_target_static_link_libs)
120+  get_target_static_link_libs(bitcoinkernel all_target_static_link_libs)
121+
122+  set(LIBS_PRIVATE)

hebasto commented at 4:20 pm on September 6, 2024:

0  set(LIBS_PRIVATE "")

if there is no intention to let the user set it as a cache variable.

(same for all_target_static_link_libs a few lines above)

Why is this local variable name capitalized?

TheCharlatan commented at 7:38 pm on September 6, 2024:

It is capitalized to match the substitution in the pkg-config file. I had the impression the substitution variables are usually capitalized as a convention.

in src/kernel/CMakeLists.txt:125 in 766881e33d outdated

120+  get_target_static_link_libs(bitcoinkernel all_target_static_link_libs)
121+
122+  set(LIBS_PRIVATE)
123+  foreach(lib ${all_target_static_link_libs})
124+    install(TARGETS ${lib} ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR})
125+    set(LIBS_PRIVATE "${LIBS_PRIVATE} -l${lib}")

hebasto commented at 4:23 pm on September 6, 2024:

style: A shorter alternative:

0    string(APPEND LIBS_PRIVATE " -l${lib}")

build: Produce a usable static kernel library

Since the move to cmake, the kernel static library that is installed
after a cmake --install build is unusable. It lacks symbols for the
internal libraries, besides those defined in the kernel library target.

This is because cmake, unlike the libtool archiver, does not combine
multiple static libraries into a single static library on installation.
This is likely an intentional design choice, since there were a bunch of
caveats to the way libtool calculated these libraries.

Fix this problem by installing all the required libraries. The user must
then link all of them along with the bitcoin kernel library.

45be32f838

TheCharlatan commented at 7:31 pm on September 6, 2024: contributor

Re #30814 (comment)Z

While installing libbitcoinkernel.pc, why ignore CMake’s configs for downstream projects using CMake?

Do you mean why I am not contributing a cmake configuration file as well in this pull request? Or is there something wrong with the current installation step that precludes also installing a configuration file? I just did not have the need for a cmake configuration file so far,. It would be good to contribute one eventually as well though. I could do it now as well, if you think it should be part of this pull request.

build: Add a pkg-config file for libbitcoinkernel 0dd16d7118

TheCharlatan force-pushed on Sep 6, 2024

TheCharlatan commented at 7:40 pm on September 6, 2024: contributor

Updated 766881e33d13ca2887f7ed179cdcc6bc007a6325 -> 0dd16d7118f10ac291a45644769121cbdfd2352f (staticKernel_0 -> staticKernel_1, compare)

Addressed @hebasto’s comment, fixed spacing around if statements.
Addressed @hebasto’s comment, set variables with "".
Addressed @hebasto’s comment, use string(APPEND... to append to the link string.

hebasto approved

hebasto commented at 7:46 pm on September 6, 2024: member

ACK 0dd16d7118f10ac291a45644769121cbdfd2352f.

DrahtBot requested review from ryanofsky on Sep 6, 2024

DrahtBot added the label CI failed on Sep 7, 2024

DrahtBot removed the label CI failed on Sep 9, 2024

fanquake referenced this in commit 11e2f9fff4 on Sep 12, 2024

fanquake approved

fanquake commented at 3:02 pm on September 12, 2024: member

ACK 0dd16d7118f10ac291a45644769121cbdfd2352f - this looks like a good place to start.

ryanofsky approved

ryanofsky commented at 3:25 pm on September 12, 2024: contributor

Code review ACK 0dd16d7118f10ac291a45644769121cbdfd2352f

Just cmake style improvements since last reveiw

fanquake merged this on Sep 12, 2024

fanquake closed this on Sep 12, 2024

in libbitcoinkernel.pc.in:7 in 0dd16d7118

0@@ -0,0 +1,11 @@
1+prefix=@CMAKE_INSTALL_PREFIX@
2+exec_prefix=${prefix}
3+libdir=${prefix}/@CMAKE_INSTALL_LIBDIR@
4+includedir=${prefix}/@CMAKE_INSTALL_INCLUDEDIR@
5+
6+Name: @PACKAGE_NAME@ kernel library
7+Description: Experimental library for the Bitcoin Core validation engine.

maflcko commented at 3:42 pm on September 12, 2024:

q: Is there a reason why PACKAGE_NAME is used above and below, but not here?

in src/kernel/CMakeLists.txt:133 in 0dd16d7118

128+
129+  string(STRIP "${LIBS_PRIVATE}" LIBS_PRIVATE)
130+endif()
131+
132+configure_file(${PROJECT_SOURCE_DIR}/libbitcoinkernel.pc.in ${PROJECT_BINARY_DIR}/libbitcoinkernel.pc @ONLY)
133+install(FILES ${PROJECT_BINARY_DIR}/libbitcoinkernel.pc DESTINATION "${CMAKE_INSTALL_LIBDIR}/pkgconfig")

hebasto commented at 9:29 am on September 15, 2024:

Should this be a part of the “Kernel” component?

TheCharlatan commented at 9:40 am on September 15, 2024:

Yeah, I missed this. Already patched in #30595 , but could also patch it independently if you want to.

hebasto commented at 9:53 am on September 15, 2024:

Already patched in #30595 , but could also patch it independently if you want to.

Great!

Feel free to consider another refactor commit to get rid off foreach() loop: https://github.com/hebasto/bitcoin/commits/240915-kernel/.

kernel: Create usable static kernel library #30814

Code Coverage

Reviews

Conflicts

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]