Page MenuHomePhabricator

[cmake] Refactor native build cmake generation
ClosedPublic

Authored by deadalnix on Sun, Jan 19, 22:08.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC25dd8a720622: [cmake] Refactor native build cmake generation
Summary

This adds a native_add_cmake_flags function for tools to add flags they want to pass to the native build. The flags are collected during the cmake execution process and _gen_native_cmake_target is run at the very end so that all the flags are used by watching when CMAKE_CURRENT_LIST_DIR is set to empty.

Test Plan

Run cmake build from scratch.
Delete the native folder. Check that it is regenrated when rebuilding.
Run the build a second time to ensure that dependecies for native target are tracked properly.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

deadalnix created this revision.Sun, Jan 19, 22:08
Herald added a reviewer: Restricted Project. · View Herald TranscriptSun, Jan 19, 22:08
Fabien requested changes to this revision.Mon, Jan 20, 11:39
Fabien added a subscriber: Fabien.
Fabien added inline comments.
cmake/modules/NativeExecutable.cmake
14 ↗(On Diff #15677)

The if is not necessary. Only INTERNAL cache variables are overridden unless FORCE is specified.

26 ↗(On Diff #15677)

Nit: symlink to because => symlink because

27 ↗(On Diff #15677)

Nit: ecutble => executable

34 ↗(On Diff #15677)

You need to add ${ARGN} to the dependencies, otherwise the native target won't rebuild if there source files are changed.

76 ↗(On Diff #15677)

Can you point to some documentation regarding this behavior ? I couldn't find anything.

cmake/templates/NativeCmakeRunner.cmake.in
3 ↗(On Diff #15677)

The bin dir is not used.

This revision now requires changes to proceed.Mon, Jan 20, 11:39
deadalnix added inline comments.Mon, Jan 20, 14:35
cmake/modules/NativeExecutable.cmake
14 ↗(On Diff #15677)

Obviously...

34 ↗(On Diff #15677)

Good catch. This isn't exact, but probably good enough.

76 ↗(On Diff #15677)

From https://cmake.org/cmake/help/v3.10/variable/CMAKE_CURRENT_LIST_DIR.html

When CMake finishes processing commands from the file it restores the previous value.

Which ends up being nothing for the last one. Let's be clear, this is an horrible hack, but it doesn't looks like there is a better way to do this.

deadalnix updated this revision to Diff 15686.Mon, Jan 20, 14:36

Update as per comments

Fabien accepted this revision.Mon, Jan 20, 15:07
Fabien added inline comments.
cmake/modules/NativeExecutable.cmake
14 ↗(On Diff #15677)

Yes it's weird...

76 ↗(On Diff #15677)

Indeed, that's hacky !

But I can't this at any good solution: PRE-BUILD custom command attached to target will only work with VS, and calling the generation at the end of the top level file will break secp256k1 build...

cmake/templates/NativeCmakeRunner.cmake.in
3 ↗(On Diff #15677)

The bin dir is still unused.

This revision is now accepted and ready to land.Mon, Jan 20, 15:07
This revision was automatically updated to reflect the committed changes.