Page MenuHomePhabricator

[CMAKE] Refactor the native builds
AbandonedPublic

Authored by Fabien on Sep 24 2020, 14:53.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Summary

This diff changes the way the native builds are handled during the cmake
build.

The current behavior re-runs the top level cmake list file in the
context of a native build. This causes the build to require all the node
software dependencies in order to build simple generation tools with
little to no dependency. It also causes the build to fail when trying to
build from the depends on platforms that don't satisfy the dependency
requirements.

The approach from this diff is to generate a new project list file for
each native executable, so the scope is restricted to the absolute
minimum. Most of the logic from the current version is maintained in
order to rebuild the native targets as needed thanks to the depfiles.

Some key advantages:
1/ During crosscompilation it is no longer necessary to satisfy the dependencies on the host platform;
2/ It is no longer necessary to forward cmake arguments from the main build to the native build;
3/ As a consequence to 2, there is no need to postpone the generation to the end of the cmake lists parsing;
4/ Faster first build (half the cmake configuration time)
5/ Less code to maintain.

Out of scope for this diff:

  • The Gitian builds can be cleaned from the host dependencies.
Test Plan
cmake -GNinja ..
ninja all check-all

Should report no work to do:

ninja

ninja generate_univalue_escapes_h

Should report no work to do:

ninja generate_univalue_escapes_h

touch src/secp256k1/src/libsecp256k1-config.h

Should rebuild the gen_context executable:

ninja

Check everything is removed:

ninja clean

cmake -GNinja .. -DCMAKE_TOOLCHAIN_FILE=../cmake/platforms/OSX.cmake
ninja

Should report no work to do:

ninja

Still works with the make generator:

cmake ..
make -j42

Diff Detail

Repository
rABC Bitcoin ABC
Branch
cmake_generate_native_list
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 12913
Build 25893: Build Difflint-circular-dependencies · build-diff · build-without-wallet · build-clang-tidy · build-clang-10
Build 25892: arc lint + arc unit

Event Timeline

Fabien requested review of this revision.Sep 24 2020, 14:53
deadalnix requested changes to this revision.Sep 24 2020, 15:06
deadalnix added a subscriber: deadalnix.

I don't think this approach make sense. It trade a low maintenance way of doing it for a high maintenance way of doing it. The only real problem that this solves is 1/. 2 to 4 are essentially made up problems to justify the solution and 5 is actually backward as there is no reason to believe that a solution that does everything automatically will be higher maintenance that a solution where one has to maintain separate build files for native executable.

This revision now requires changes to proceed.Sep 24 2020, 15:06
Fabien planned changes to this revision.Sep 25 2020, 09:46

2/ Is actually important wrt to maintenance. Having the native build de-correlated from the root CMakeLists.txt prevents from having to think to maybe adjust the native flags each time a new option is added.

I have a solution for keeping this approach but dropping the new CMakeLists files maintenance, will update.

Generate the CMakeLists.txt files. The scope is still reduced but there is no added maintenance.

deadalnix requested changes to this revision.Sep 25 2020, 13:38

2/ is not a major issue, at least in the same category of the problems created. If you get it wrong on 2/ either the build doesn't work, and you know it right away, or the build has an extra dependency, which is annoying but still okay.

Considering 1 and 2 are both about dependencies, you got to wonder what is it that you are actually trying to solve. WHat is rebuilding dependencies so much that it require breaking the way native build work? What can be done to solve the actual problem?

cmake/templates/NativeCMakeLists.txt.in
7

Now you can't have native executable use libraries.

This revision now requires changes to proceed.Sep 25 2020, 13:38