Page MenuHomePhabricator

Added CMake function to detect BOOST_TEST_DYN_LINK
ClosedPublic

Authored by nakihito on Nov 12 2019, 01:05.

Details

Summary

Avoid code duplication by making BOOST_TEST_DYN_LINK detection into a CMake
module.

Test Plan
ninja check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
BoostDynFunction
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 8078
Build 14146: Bitcoin ABC Buildbot (legacy)
Build 14145: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Nov 12 2019, 01:05
Fabien requested changes to this revision.Nov 12 2019, 07:30

This seems to me that it is doing half of the work:

  • You set a boost library as a requirement, but you don't search for it
  • You add a boost unit test specific definition, but you don't link the executable to the boost library
  • Intended that this is test specific, why not put it to the TestSuite.cmake module ? The proposed module, by its name and content will likely never contain anything more than this very specific function.
cmake/modules/DetectBoostDynLink.cmake
3 ↗(On Diff #14063)

You should make it a function so that CMAKE_REQUIRED_LIBRARIES remains scoped.

14 ↗(On Diff #14063)

This is missing include(CheckCXXSourceCompiles)

This revision now requires changes to proceed.Nov 12 2019, 07:30

Moved function to TestSuite.cmake. Function now also handles searching for the boost package and linking the library to the executable.

Function now also adds the unit test files to the test suite by calling add_test_to_suite.

Fabien requested changes to this revision.Nov 12 2019, 20:49
Fabien added inline comments.
cmake/modules/TestSuite.cmake
20 ↗(On Diff #14081)

Use the same prototype than add_test_to_suite, the second argument is not yet an executable at call time.

27 ↗(On Diff #14081)

Newlines are free, you can add some to make it easier to read.

36 ↗(On Diff #14081)

This doesn't match the function name.

This revision now requires changes to proceed.Nov 12 2019, 20:49
This revision is now accepted and ready to land.Nov 12 2019, 21:17