Page MenuHomePhabricator

Added CMake function to detect BOOST_TEST_DYN_LINK
ClosedPublic

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

Details

Reviewers
deadalnix
Fabien
jasonbcox
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABCe06c742b1b12: Added CMake function to detect BOOST_TEST_DYN_LINK
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 OK
Unit
No Unit Test Coverage
Build Status
Buildable 8068
Build 14126: Bitcoin ABC Buildbot (legacy)
Build 14125: arc lint + arc unit

Event Timeline

nakihito created this revision.Nov 12 2019, 01:05
Owners added a reviewer: Restricted Owners Package.Nov 12 2019, 01:05
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 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

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

14

This is missing include(CheckCXXSourceCompiles)

This revision now requires changes to proceed.Nov 12 2019, 07:30
nakihito updated this revision to Diff 14066.Nov 12 2019, 08:26

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

nakihito planned changes to this revision.Nov 12 2019, 08:26
nakihito updated this revision to Diff 14080.Nov 12 2019, 19:49

Renamed function.

nakihito updated this revision to Diff 14081.Nov 12 2019, 20:31

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
nakihito updated this revision to Diff 14082.Nov 12 2019, 21:02

Addressed nits.

Fabien accepted this revision.Nov 12 2019, 21:17
This revision is now accepted and ready to land.Nov 12 2019, 21:17