Page MenuHomePhabricator

[CMAKE] Fix build with make as a generator
ClosedPublic

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

Details

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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.

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).

cmake/modules/NativeExecutable.cmake
33 ↗(On Diff #16528)

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 ↗(On Diff #16528)

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 ↗(On Diff #16528)

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 ↗(On Diff #16528)

Will do.

This revision is now accepted and ready to land.Feb 28 2020, 13:12
This revision was automatically updated to reflect the committed changes.