Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

fix boost unit test linkage; fixes build on Fedora 34 #10305

Merged
merged 7 commits into from
May 6, 2021

Conversation

spoonincode
Copy link
Contributor

Change Description

Even with #10270 building still fails on Fedora 34 due to duplicate symbol errors.

I'm not really sure why Fedora 34 is the first environment encountering this problem. Compiler version? Boost version? Some combination of those? Are they using a different linker? Hidden compiler flags? Regardless, after digging it seems fairly clear we've messed up with linking boost unit test.

First off, there are several ways of using boost test:
https://www.boost.org/doc/libs/1_76_0/libs/test/doc/html/boost_test/usage_variants.html
Most important to us is the header-only variant and the static library variant.

In the current develop code you'll find that eosio_testing will static link to boost unit test

target_link_libraries( eosio_testing eosio_chain fc chainbase Logging IR WAST WASM Runtime Boost::unit_test_framework )

But then in, say, in plugin_test we link to eosio_testing
target_link_libraries( plugin_test eosio_testing eosio_chain chainbase chain_plugin wallet_plugin fc ${PLATFORM_SPECIFIC_LIBS} )

and then use... the header-only variant of boost_test in plugin_test!
#include <boost/test/included/unit_test.hpp>

This hasn't caused us any problems in the past (AFAIK), but it's causing problems on Fedora 34 and seems flat out wrong to mix them up.

Fixing this is complicated by txn_test_gen_plugin which links to eosio_testing. (yes this means the testing library gets linked in to nodeos 😞) Just simply removing the static library linkage of boost test from eosio_testing causes problems when building nodeos because of missing symbols -- symbols that can't easily be added back because we can't use the header-only version of boost test in nodeos due to it taking over main() etc etc etc

txn_test_gen_plugin actually only needs a very tiny sliver of eosio_testing -- it needs the eosio::testing::contracts in contracts.hpp, generated from contracts.hpp.in. This is quite self-contained.. it just reads some files at some paths figured out during the build. What I've elected to do is move this contracts.hpp generation out to its own cmake target: eosio_testing_contracts. This now allows txn_test_gen_plugin (and thus nodeos) to only link to that small helper instead of the entire eosio_testing which needs some boost test pieces.

Now honestly, what is going on here is still a little icky since eosio_testing_contracts is creating a header file that references contract file paths from unittests/. This doesn't seem any more or less icky that what already existed though -- eosio_testing having a dependency on a header file that doesn't exist until unittests/CMakeLists.txt creates it way after eosio_testing target has been defined. It may be possible to clean some of this up further in the future... maybe all contracts of unittests/ should get pulled up to libraries/testing or something. Such a change would make this PR hard to follow though.

Anyways, now this means we can remove the static library linkage of boost test from eosio_testing allowing the header-only variant to be properly used from something like plugin_test. Best I could tell, nearly all of our existing tests already were either

  1. using eosio_tester but using the header-only variant include
  2. not using eosio_tester, and correctly using either the header-only variant or static library variant

So a small number of tests were using eosio_testing but expected the static library variant. These were simply changed from <boost/test/unit_test.hpp> to <boost/test/included/unit_test.hpp> to fix them up.

After these changes, build completes correctly on Fedora 34.

Change Type

Select ONE:

  • Documentation
  • Stability bug fix
  • Other
  • Other - special case

Testing Changes

Select ANY that apply:

  • New Tests
  • Existing Tests
  • Test Framework
  • CI System
  • Other

Consensus Changes

  • Consensus Changes

API Changes

  • API Changes

Documentation Additions

  • Documentation Additions

Copy link
Contributor

@heifner heifner left a comment

Choose a reason for hiding this comment

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

I would also be in favor of only linking txn_test_gen_plugin when some cmake variable (off by default) is specified.

@spoonincode spoonincode merged commit 7f708fe into develop May 6, 2021
@spoonincode spoonincode deleted the fix_boost_unittest_linkage branch May 6, 2021 17:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants