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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fabien created this revision.Oct 9 2019, 11:46
Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 9 2019, 11:46
Fabien updated this revision to Diff 13413.Oct 9 2019, 11:50

Missing newline.

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
Fabien updated this revision to Diff 13416.Oct 9 2019, 13:02

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
Fabien edited the summary of this revision. (Show Details)Mon, Oct 14, 14:34
Fabien updated this revision to Diff 13529.Mon, Oct 14, 14:59

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.

deadalnix added inline comments.Tue, Oct 15, 13:41
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.Tue, Oct 15, 13:42
This revision now requires changes to proceed.Tue, Oct 15, 13:42
Fabien updated this revision to Diff 13574.Wed, Oct 16, 06:58

Revert dependency, now depends on D4257.

Fabien planned changes to this revision.Wed, Oct 16, 06:58
Fabien edited the summary of this revision. (Show Details)
Fabien added inline comments.Fri, Oct 18, 13:25
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.

Fabien edited the test plan for this revision. (Show Details)Fri, Oct 18, 13:36
Fabien updated this revision to Diff 13607.Fri, Oct 18, 14:33

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.Fri, Oct 25, 01:38
This revision now requires changes to proceed.Fri, Oct 25, 01:38
Fabien planned changes to this revision.Mon, Oct 28, 15:04
Fabien edited the summary of this revision. (Show Details)
Fabien updated this revision to Diff 13723.Mon, Oct 28, 16:11
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.

Fabien added inline comments.Mon, Oct 28, 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.

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