Details

Reviewers

Group Reviewers

Restricted Project

Commits

rSTAGINGeb1d98d21207: [CMAKE] Build bitcoinconsensus library both static and shared
rABCeb1d98d21207: [CMAKE] Build bitcoinconsensus library both static and shared

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 7759
Build 13557: Bitcoin ABC Buildbot (legacy)
Build 13556: arc lint + arc unit

Event Timeline

Fabien created this revision.Oct 9 2019, 11:46

Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 9 2019, 11:46

Herald added a reviewer: deadalnix. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Diff Testing started.

Missing newline.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7721: Diff 13412.Oct 9 2019, 11:56

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7722: Diff 13413.Oct 9 2019, 12:02

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.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Harbormaster completed remote builds in B7725: Diff 13416.Oct 9 2019, 13:12

Build Bitcoin-ABC / Diffs / Diff Testing passed.

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

Fabien edited the summary of this revision. (Show Details)Oct 14 2019, 14:34

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.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7759: Diff 13529.Oct 14 2019, 15:14

Fabien added a child revision: D4257: [CMAKE] Propagate requirements for cmake >= 3.12.Oct 15 2019, 12:22

deadalnix added inline comments.Oct 15 2019, 13:41

cmake/modules/AddLibraryHelper.cmake
26	Why is EXCLUDE_FROM_ALL necessary?
39	If you have SHARED and STATIC, you build the shared version twice.
src/CMakeLists.txt
451	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

Fabien mentioned this in D4257: [CMAKE] Propagate requirements for cmake >= 3.12.Oct 16 2019, 06:56

Fabien removed a child revision: D4257: [CMAKE] Propagate requirements for cmake >= 3.12.

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)

Fabien added a parent revision: D4257: [CMAKE] Propagate requirements for cmake >= 3.12.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Harbormaster completed remote builds in B7787: Diff 13574.Oct 16 2019, 07:16

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Fabien added inline comments.Oct 18 2019, 13:25

cmake/modules/AddLibraryHelper.cmake
39	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.

Fabien mentioned this in rABCd6f375f2c914: [CMAKE] Propagate requirements for cmake >= 3.12.Oct 18 2019, 13:28

Fabien edited the test plan for this revision. (Show Details)Oct 18 2019, 13:36

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.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7808: Diff 13607.Oct 18 2019, 14:43

Fabien mentioned this in D4267: [CMAKE] Use CPack to build packages.Oct 18 2019, 15:51

Fabien mentioned this in rABC5928cdaa5b2c: [CMAKE] Use CPack to build packages.Oct 23 2019, 09:13

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 removed a parent revision: D4257: [CMAKE] Propagate requirements for cmake >= 3.12.

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

Build Bitcoin-ABC / Diffs / Diff Testing started.

Fabien added inline comments.Oct 28 2019, 16:12

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.