Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix Boost::unit_test_framework linkage -- don't mix header & static library usage variants #337

Merged
merged 3 commits into from
Jul 2, 2024

Conversation

spoonincode
Copy link
Member

Boost unit test framework can be used via header-only or by linking to a static library (or dynamic library). See https://www.boost.org/doc/libs/1_85_0/libs/test/doc/html/boost_test/usage_variants.html. Mixing these up can cause linker failures and/or ODR violations. If this sounds familiar, we previously had a mix ups fixed in AntelopeIO/leap#807. It appears we've mixed these up again in leap5 with the boost submodularization.

For example, in fc,

target_link_libraries( fc PUBLIC Boost::date_time Boost::chrono Boost::iostreams Boost::interprocess Boost::multi_index Boost::dll
Boost::multiprecision Boost::beast Boost::asio Boost::thread Boost::unit_test_framework Threads::Threads

The Boost::unit_test_framework target will link the Boost unit test static library. But then in test_fc the header-only usage variant is used,
#define BOOST_TEST_MODULE libfc
#include <boost/test/included/unit_test.hpp>

(of course, libfc linking to Boost::unit_test_framework is also undesirable on its own -- there is no reason nodeos should be linking to boost unit test framework)

This PR removes the Boost::unit_test_framework linkage from a number of base libraries -- appbase, fc, and chainbase -- and then adds the appropriate header-only Boost::included_unit_test_framework target where it is used. (curiously, chainbase_test appears to be the only remaining test leveraging the static library approach. I've left it as is, there is nothing wrong with that.)

Requires,

#define BOOST_TEST_MODULE benchmark dummy module
#define BOOST_TEST_NO_MAIN
#include <boost/test/included/unit_test.hpp>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ugly but with the way libtester is currently written, anything it is linked to needs to have various boost unit test symbols defined.. and this is the best way without further refactor afaict

@spoonincode spoonincode merged commit b48c934 into main Jul 2, 2024
36 checks passed
@spoonincode spoonincode deleted the butf_linkage branch July 2, 2024 15:24
@ericpassmore
Copy link
Contributor

Note:start
group: CLEANCODE
category: INTERNALS
summary: Consistently include Boost Test Framework, linking to library.
Note: end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants