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

CMake cleanup & MinGW support #1706

Merged
merged 13 commits into from
Sep 23, 2019

Conversation

crypto-ape
Copy link
Contributor

@crypto-ape crypto-ape commented Apr 8, 2019

Hey Monkeys!

I have cleaned up the CMake scripts for the main project also, and made some improvements:

  • removed unused and irrelevant code
    • old and not supported Boost library handling routines, TCL and BDB references
  • MinGW support ( supports building Windows binaries on Linux )
  • enforcing more strict compiler warnings and errors

Python helpers:

  • rewritten cat-parts and embed_genesis helpers into Python
    • simplifies cross build
    • can be manually turned off with GENERATING_HELPERS CMake parameter
  • if Python is not found, the build continues in the usual way
  • works on Windows / Linux / Mac

I have also enabled code fortification mechanisms

  • FORTIFY_SOURCE, safe-stack, stack-protector, ...

Related: bitshares/bitshares-fc#121

Ape out!

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@abitmore abitmore added this to the 3.2.0 - Feature Release milestone Apr 8, 2019
@pmconrad
Copy link
Contributor

pmconrad commented Apr 9, 2019

What I really dislike about this PR is the huge load of new CMake modules. Is that really necessary?

I'm all for simplifying the build process, but adding 3kLoC only for replacing cat-parts and embed_genesis with python equivalents is not really it.

@crypto-ape
Copy link
Contributor Author

Well, I kind of agree with you. I was not sure about adding those files, neither.

In the meantime, I have come to an idea that would support older CMake 3 versions, and would not require so many supporting files.

This PR was not just about replacing cat-parts, and embed_genesis, but also about cleaning up the unneeded and confusing parts of CMake scripts.

@abitmore
Copy link
Member

Being able to "fallback" to old C++ cat-parts means we have to keep both the C++ code and the new python script in the code base? What will we gain by doing so?

@crypto-ape
Copy link
Contributor Author

Being able to "fallback" to old C++ cat-parts means we have to keep both the C++ code and the new python script in the code base? What will we gain by doing so?

Yes, you are right. Both versions would have to be present.

Mostly, lowering requirements for the most common use case, making the build easier for the curious user.

The technical trad-off is of course the need to keep the both versions in sync.

On the other hand, it seems to me that this would be of no serious concern, because the functionality of embed_genesis and cat-parts is stable and would not require much modifications in the future.

@crypto-ape
Copy link
Contributor Author

Hey Monkeys, It's me again.

I was working the fix for the fc subroject, and now it is finally working.

Here is what is updated since the initial commit.

  • Updated FindPython and FindBoost, both are more future-proof
    • rewritten FindPython to be a lightweight wrapper for FindPythonInterp, if FindPython is not available ( CMake < 3.12 )
  • FindBoost
    • uses config from fc subproject
      • the configuration is in one place
    • the fc does some FindBoost magic
  • parallel (multiprocessed) embed_genesis Python script - speed improvement while processing large genesis files

This project requires updated version of fc to compile (namely the BoostConfig.cmake file)

@abitmore
Copy link
Member

abitmore commented May 17, 2019

Travis complained:

CMake Error at CMakeLists.txt:116 (add_subdirectory):
  add_subdirectory given source "libraries/fc/CMakeModules/Legacy" which is
  not an existing directory.
CMake Error at CMakeLists.txt:147 (FIND_PACKAGE):
  Could not find a package configuration file provided by "Boost" with any of
  the following names:
    BoostConfig.cmake
    boost-config.cmake
  Add the installation prefix of "Boost" to CMAKE_PREFIX_PATH or set
  "Boost_DIR" to a directory containing one of the above files.  If "Boost"
  provides a separate development package or SDK, be sure it has been
  installed.

Need to merge bitshares/bitshares-fc#121 first, then bump FC?

@pmconrad
Copy link
Contributor

See comment above:

This project requires updated version of fc to compile (namely the BoostConfig.cmake file)

@pmconrad
Copy link
Contributor

bitshares/bitshares-fc#121 has been merged, please rebase and bump fc.

@crypto-ape
Copy link
Contributor Author

I have failed to compile the most up-to-date, develop with fc:master, branch, because of issues unrelated to this PR.

There is some other refactoring in the fc submodule which probably causes this failure.

Excerpt from compilation log:

libraries/protocol/include/graphene/protocol/object_id.hpp:171:17: error: ‘true_type’ in namespace ‘fc’ does not name a type
     typedef fc::true_type  is_defined;
                 ^~~~~~~~~
/libraries/protocol/include/graphene/protocol/object_id.hpp:172:17: error: ‘false_type’ in namespace ‘fc’ does not name a type
     typedef fc::false_type is_enum;

After fixing this, another issues with uint128 arose.

It seems to me, that the develop branch in its current form is not usable with the fc:master branch, and consequently neither is this PR branch.

@pmconrad
Copy link
Contributor

Right, sorry. fc/master requires changes from #1789 . There are still some issues in there wrt windows + mac builds, that's why it hasn't been merged into develop yet.

@pmconrad
Copy link
Contributor

@crypto-ape please rebase on latest develop. #1789 has been merged in so there shouldn't be conflicts anymore.

@crypto-ape
Copy link
Contributor Author

crypto-ape commented Aug 15, 2019

@pmconrad Working on it. Will push tomorrow if nothing unexpected happens.

@crypto-ape
Copy link
Contributor Author

crypto-ape commented Sep 20, 2019

@pmconrad please review, updated according to your suggestions.

I have tested, seems to work on all platforms, except for the problem with cli_wallet where the wallet.cpp fails for MinGW.

@pmconrad
Copy link
Contributor

Thanks!

@abitmore abitmore merged commit d527abb into bitshares:develop Sep 23, 2019
@crypto-ape crypto-ape deleted the windws_build_update branch September 24, 2019 13:09
@ryanRfox ryanRfox mentioned this pull request Nov 18, 2019
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.

5 participants