Page MenuHomePhabricator

Added CMake function to detect BOOST_TEST_DYN_LINK
ClosedPublic

Authored by nakihito on Tue, Nov 12, 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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

nakihito created this revision.Tue, Nov 12, 01:05
Owners added a reviewer: Restricted Owners Package.Tue, Nov 12, 01:05
Herald added a reviewer: Restricted Project. · View Herald TranscriptTue, Nov 12, 01:05
Fabien requested changes to this revision.Tue, Nov 12, 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.Tue, Nov 12, 07:30
nakihito updated this revision to Diff 14066.Tue, Nov 12, 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.Tue, Nov 12, 08:26
nakihito updated this revision to Diff 14080.Tue, Nov 12, 19:49

Renamed function.

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

Addressed nits.

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