Page MenuHomePhabricator

[CMAKE] Build bitcoinconsensus library both static and shared
ClosedPublic

Authored by Fabien on Oct 9 2019, 11:46.

Details

Summary

The bitcoinconsensus library exports the functions from
src/script/bitcoinconsensus.h:

  • bitcoinconsensus_verify_script
  • bitcoinconsensus_verify_script_with_amount
  • bitcoinconsensus_version

This diff adds a facility to build (if required) and install a shared
library, independently of whether the library already exists as static and/or
shared.

Note that building with BUILD_SHARED_LIBS=ON set will fail and needs a
fix which is outside the scope of this diff (the failure is not related
to this diff).

This is a replacement for D4197 and D4198.

Test Plan
mkdir -p buildcmake/install && cd buildcmake
cmake -GNinja .. -DCMAKE_INSTALL_PREFIX=install
ninja install

ls install/lib

Should return 1 file and 2 symbolic links:

  • libbitcoinconsensus.so.0.0.0
  • libbitcoinconsensus.so.0 (link to libbitcoinconsensus.so.0.0.0)
  • libbitcoinconsensus.so (link to libbitcoinconsensus.so.0)

    ls install/include

Should return bitcoinconsensus.h.

nm -D --defined-only install/lib/libbitcoinconsensus.so

Check the exported symbols contain the 3 above mentioned functions.

ninja check # There are additional test cases in `script_tests.cpp`

rm -rf *
mkdir install
cmake -GNinja .. -DCMAKE_INSTALL_PREFIX=install \
  -DCMAKE_TOOLCHAIN_FILE=../cmake/platforms/OSX.cmake
ninja install

ls install/lib

Should return 1 file and 1 symbolic link:

  • libbitcoinconsensus.0.dylib
  • libbitcoinconsensus.dylib (link to libbitcoinconsensus.0.dylib)

    ls install/include

Should return bitcoinconsensus.h.

rm -rf *
mkdir install
cmake -GNinja .. -DCMAKE_INSTALL_PREFIX=install \
  -DBUILD_BITCOIN_SEEDER=OFF \
  -DCMAKE_TOOLCHAIN_FILE=../cmake/platforms/Win64.cmake
ninja install

ls install/lib

Should return 1 file :

  • libbitcoinconsensus-0.dll

    ls install/include

Should return bitcoinconsensus.h.

rm -rf *
cmake -GNinja .. -DCMAKE_INSTALL_PREFIX=install \
  -DBUILD_LIBBITCOINCONSENSUS=OFF
ninja install

ls install

Should have no lib nor include directory.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
cmake_libbitcoinconsensus
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 7721
Build 13481: Bitcoin ABC Buildbot (legacy)
Build 13480: arc lint + arc unit

Event Timeline

deadalnix requested changes to this revision.Oct 9 2019, 12:40
deadalnix added inline comments.
cmake/modules/AddBitcoinABC.cmake
4 ↗(On Diff #13413)

That is not a meaningful name.

cmake/modules/InstallationHelper.cmake
14 ↗(On Diff #13413)

This is a macro, so it"ll leave a bunch a definition in the caller's scope.

This revision now requires changes to proceed.Oct 9 2019, 12:40

Pick better names for the wrapper function and its cmake file.
Make the install_target() a function to separate its scope from the caller's.

Fabien planned changes to this revision.Oct 11 2019, 13:14

Attempt to make the interface more simple and clear.
The add_library_with_type() function manages adding a library with one type or
more (STATIC, SHARED or both) but let the caller deal with properties.
This makes it easier to apply some property to only the static or shared variant
of a library if both are added.

cmake/modules/AddLibraryHelper.cmake
26 ↗(On Diff #13529)

Why is EXCLUDE_FROM_ALL necessary?

39 ↗(On Diff #13529)

If you have SHARED and STATIC, you build the shared version twice.

src/CMakeLists.txt
451 ↗(On Diff #13529)

This seems like an aweful lot of boilerplate.

deadalnix requested changes to this revision.Oct 15 2019, 13:42
This revision now requires changes to proceed.Oct 15 2019, 13:42

Revert dependency, now depends on D4257.

Fabien planned changes to this revision.Oct 16 2019, 06:58
Fabien edited the summary of this revision. (Show Details)
cmake/modules/AddLibraryHelper.cmake
39 ↗(On Diff #13529)

If you have both you end up with 2 calls:

  • The first one builds the object and static libs
  • The second one builds the shared lib

The unset(ARG_SHARED) prevents creating the shared lib twice.

Remove useless EXCLUDE_FROM_ALL.
Remove some boilerplates:

  • Let the function define a couple variables so the target names are directly testable and accessible
  • Move the versioning and output naming back into the function as they are not library specific.

Ok, so I don't think this API is very good. add_library_with_type is kind of a tell, and the fact it has to duplicate a bunch of facility such as specifying linking and all. So here is the API that you want, IMO:

cmake
# libbitcoinconsensus
add_library(bitcoinconsensus
	arith_uint256.cpp
	hash.cpp
	primitives/transaction.cpp
	pubkey.cpp
	uint256.cpp
	util/strencodings.cpp
)

target_link_libraries(bitcoinconsensus script)

if(BUILD_LIBBITCOINCONSENSUS)
  install_shared_library(bitcoinconsensus script/bitcoinconsensus.cpp PUBLIC_HEADER script/bitcoinconsensus.h)
endif()

I typed this without checking correctness, it's to be considered pseudocode to convey the idea.

If the lib is already shared, then you can edit its properties to add the new source. If it is static, then you can create a new shared lib, either making the static one a dependency, or by gutting the static one into an object one, by reading/editing its properties. Then proceed to install the shared lib where it is appropriate. You could even do so unconditionay rather than make it a config, and simply not build the shared lib by default, but only when it is asked for.

src/CMakeLists.txt
418 ↗(On Diff #13607)

That is probably not needed. You'd only want this to be able to run the tests, but you might as well link the shared lib directly to the test executable.

src/test/CMakeLists.txt
183 ↗(On Diff #13607)

It's probably better placed in the generated config.h like for autotools. Or it can be set as a transitive property on the lib itself (it's probably the best option).

deadalnix requested changes to this revision.Oct 25 2019, 01:38
This revision now requires changes to proceed.Oct 25 2019, 01:38
Fabien planned changes to this revision.Oct 28 2019, 15:04
Fabien edited the summary of this revision. (Show Details)
Fabien marked an inline comment as done.

Update API according to feedback.
Make the static lib a dependency for the shared one instead of building an object lib.

src/CMakeLists.txt
402 ↗(On Diff #13723)

I kept the option, because excluding the target from the default build will prevent using any install directive on this target.

src/test/CMakeLists.txt
183 ↗(On Diff #13607)

I set it as a transitive dependency, which I also think is a better option.

This revision is now accepted and ready to land.Oct 29 2019, 20:07