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

rodeos and eosio-tester #9018

Merged
merged 28 commits into from
May 5, 2020
Merged

rodeos and eosio-tester #9018

merged 28 commits into from
May 5, 2020

Conversation

tbfleming
Copy link
Contributor

Change Description

This builds on #8750

Consensus Changes

  • Consensus Changes

API Changes

  • API Changes

Documentation Additions

  • Documentation Additions

@spoonincode
Copy link
Contributor

I realize this is an opinion since we haven't come to a strategy as a team, but it'd be nice to not add new external deps unless needed. In this case, it seems like gflags has great documented support for being included as a git submodule and simply built as part of our eosio cmake "super project". Could we try that instead of bringing in a new external dep?

@tbfleming
Copy link
Contributor Author

Yep. That could solve some namespace differences I'm seeing between the different distros.

@tbfleming
Copy link
Contributor Author

The question is: how can I point rocksdb's cmake to it without modifying rocksdb's cmake

@spoonincode
Copy link
Contributor

actually, do we even need gflags support in rocksdb? Appears it can be built without it
CMAKE_DEPENDENT_OPTION(WITH_GFLAGS "build with GFlags" ON

@spoonincode
Copy link
Contributor

We may need to install rocksdb's license file. See toward end of root CMakeLists.txt.

Actually there may be two license files that we need, not sure about that leveldb license file.

@tbfleming
Copy link
Contributor Author

I suspect we need both

@tbfleming
Copy link
Contributor Author

@leordev is going to add rabbitmq support. We need to decide how to bring in that library. submodule?

@tbfleming tbfleming marked this pull request as ready for review April 29, 2020 20:30
@tbfleming tbfleming mentioned this pull request Apr 29, 2020
3 tasks
@leordev leordev mentioned this pull request Apr 30, 2020
3 tasks
Copy link
Contributor

@spoonincode spoonincode left a comment

Choose a reason for hiding this comment

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

I think we need to place the rapidjson in use by fc in to a separate namespace as it will be a different version of what abieos uses and may in theory cause an ODR violation? I will take care of that. Just blocking PR until I get that fix in..

@tbfleming
Copy link
Contributor Author

We also need to make sure that every piece of code which uses

#include <rapidjson/reader.h>

pulls in the right one.

@spoonincode spoonincode dismissed their stale review May 2, 2020 03:25

removing rapidjson block

@spoonincode
Copy link
Contributor

A couple build issues I've noticed:

abieos cmake lists cmake 3.11 as required. Is it actually required? using cmake 3.10 seemed to work for me. eosio current minimum is 3.8 so this change would move that requirement.
I also wonder if we really need the set(CMAKE_OSX_DEPLOYMENT_TARGET 10.14) in abieos any more that is kinda hacky.

gcc8 & 9 are failing due to line 53 in state_history_plugin.cpp -- another one of those member name same as type name issues.

gcc7 ICEs.

@spoonincode
Copy link
Contributor

For rocks, we're still building tests (WITH_TESTS) and benchmarks (WITH_BENCHMARKS_TOOLS) and tools (WITH_TOOLS). Do we need all of these?

@spoonincode
Copy link
Contributor

bind_front_handler, tcp_stream, and make_strand are boost 1.70 features and will fail with current eosio 1.67 minimum requirement.

@tbfleming
Copy link
Contributor Author

If I remember right, make_strand came with a change to the threading interface used by beast. If we switch it to the previous interface, it won't build with 1.70. Maybe I'm remembering wrong.

Copy link
Contributor

@arhag arhag left a comment

Choose a reason for hiding this comment

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

We will address the gcc7 ICE by raising minimum gcc requirements to 8. We will address the Boost issues by increasing minimum Boost requirements to 1.70. These will be handled in other PRs.

This approval does not mean that I fully reviewed all of the new rodeos and tester code. Rather this PR review was focused on build matters and ensuring that all the new code and changes do not impact the behavior of nodeos.

@arhag arhag merged commit 60691a5 into develop May 5, 2020
@arhag arhag deleted the rodeos_tester branch May 5, 2020 18:27
This was referenced May 12, 2020
@TaraTritt TaraTritt mentioned this pull request May 26, 2020
3 tasks
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.

4 participants