Paths

Table of Contentst

Differential D5333

[CMAKE] Fix build with make as a generator
ClosedPublic
Actions

Authored by Fabien on Feb 26 2020, 13:26.

Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Commits

rSTAGINGe92c4d4db9a5: [CMAKE] Fix build with make as a generator
rABCe92c4d4db9a5: [CMAKE] Fix build with make as a generator

Summary

There are a few issues preventing this build to succeed:

There is a dependency issue in the NativeExecutable.cmake module: a

custom_command() cannot have a file dependency which is the output of
another custom_command() if this command is from another list file.
The solution here is to use a target dependency instead.

Target naming with relative path is unsupported.
The make generator differs in the way the build directories are

created, and require some manual creation.

Since the generator name is Unix Makefiles, it should be quoted to

retain the space.

Test Plan

# No regression
cmake -GNinja ..
ninja all check-all

cmake ..
make all check-all

Diff Detail

Repository

rABC Bitcoin ABC

Branch

cmake_fix_make

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 9585
Build 17073: Default Diff Build & Tests
Build 17072: arc lint + arc unit

Event Timeline

Fabien created this revision.Feb 26 2020, 13:26

Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 26 2020, 13:26

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9579: Diff 16518.Feb 26 2020, 13:36

Fabien added a child revision: D5334: [CI] Run the cmake build with make as a generator.Feb 26 2020, 13:52

deadalnix requested changes to this revision.Feb 26 2020, 16:51

deadalnix added a subscriber: deadalnix.

deadalnix added inline comments.

cmake/modules/NativeExecutable.cmake
35 ↗	(On Diff #16518)	Won't this rerun the native cmake every time?
cmake/templates/NativeCmakeRunner.cmake.in
3 ↗	(On Diff #16518)	No space after -G
src/qt/CMakeLists.txt
50 ↗	(On Diff #16518)	Why is this needed?

This revision now requires changes to proceed.Feb 26 2020, 16:51

You need to fix you test plan, it's not displaying right.

Fabien edited the test plan for this revision. (Show Details)Feb 26 2020, 18:58

Fabien added inline comments.Feb 26 2020, 19:08

cmake/modules/NativeExecutable.cmake
35 ↗	(On Diff #16518)	It will re-run only if the dependencies from the native/CMakeCache.txt file requires it, which is the same behavior as before (because this file is the only dependency of the custom target). I tested this behavior with both `ninja` and `make` and it was OK.
src/qt/CMakeLists.txt
50 ↗	(On Diff #16518)	There is a difference in behavior between the `make` and `ninja` generators. The `make` one does not create the build subdirs that result from a custom command (which I suppose the `qt5_wrap_dir` macro does). I failed to find this documented anywhere, I deduced it from observation. The same happen with the native build directory. If you look at the template script I had to remove the `cd` command since the directory was not created with `make` (it was not necessary anyway because the `cmake` command uses absolute paths).

Remove space.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9585: Diff 16528.Feb 26 2020, 19:20

Fabien added a child revision: D5338: [CMAKE] Fix the build with Xcode as a generator.Feb 27 2020, 11:57

deadalnix added inline comments.Feb 27 2020, 23:30

cmake/modules/NativeExecutable.cmake
33	I get that this is useful for make, but this is far from ideal because this can be ambiguous. Is that possible to specialize on the generator so that the ambiguous case is only used with crappy ones (like make)?
src/qt/CMakeLists.txt
50	Add a comment to explain why this is useful.

deadalnix requested changes to this revision.Feb 27 2020, 23:30

This revision now requires changes to proceed.Feb 27 2020, 23:30

Fabien planned changes to this revision.Feb 28 2020, 07:46

Fabien added inline comments.

cmake/modules/NativeExecutable.cmake
33	This is technically possible, but I don't think it's a good idea. There is no single `make` generator, but there are plenty. The list will likely increase with cmake versions, and this is not accounting for their variants ! AFAIK, `ninja` is more the exception than the rule here but I didn't test all the generators so it's hard to be sure. That would make it the exception for the "good" behavior, which is weird. I'm not even sure that you can manage to have collision, since the `--build` command is processing from the `${NATIVE_BUILD_DIR}`, so only the top `Makefile` should matter and the targets cannot be duplicated in it.
src/qt/CMakeLists.txt
50	Will do.