[CMAKE] Fix the check-bitcoin-* targets when running with Xcode
ClosedPublic
Actions

Authored by Fabien on Feb 27 2020, 21:53.

Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Commits

rSTAGING92a5a729ff76: [CMAKE] Fix the check-bitcoin-* targets when running with Xcode
rABC92a5a729ff76: [CMAKE] Fix the check-bitcoin-* targets when running with Xcode

Summary

This diff makes the unit test runner script use the full path to run
test_bitcoin, making it independent of the generator configuration for
Xcode and Visual Studio (see D5338). This applies as well to
test_bitcoin-qt and test_bitcoin-seeder.

Note: the check target still fails and will require more changes to
make it work. This is because the python scripts bitcoin-util-test.py
use an hardcoded path to the bitcoin-tx binary (relative to the build
directory).

Test Plan

cmake -GNinja ..
ninja check

cmake -G"Unix Makefiles" ..
make check

On OSX:

cmake -GXcode ..
cmake --build . --config RelWithDebInfo --target check-bitcoin
cmake --build . --config RelWithDebInfo --target check-bitcoin-qt
cmake --build . --config RelWithDebInfo --target check-bitcoin-seeder

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

Fabien created this revision.Feb 27 2020, 21:53

Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 27 2020, 21:53

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9599: Diff 16553.Feb 27 2020, 22:06

Overall LGTM, but I don't really understand why the file is copied. Should the buid directory be different for different configuration anyways? I don't get it.

cmake/modules/TestSuite.cmake
55 ↗	(On Diff #16553)	This say what this does, but we kind of know that already by reading the code. A more interesting thing is WHY?
59 ↗	(On Diff #16553)	Why copy rather than link?
cmake/templates/TestRunner.cmake.in
3 ↗	(On Diff #16553)	Using absolute path is a good idea regardless of anything else IMO

This revision now requires changes to proceed.Feb 29 2020, 00:08

Improve the comment, explain why file(GENERATE) is needed.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Fabien added inline comments.Mar 2 2020, 11:06

cmake/modules/TestSuite.cmake
55 ↗	(On Diff #16553)	I updated the comment, hopefully it's clearer now. This is not trivial, but both `configure_file()` AND `file(GENERATE)` are required: `configure_file()` will replace the standard variables but not the generator expressions `file(GENERATE)` will replace the generator expressions, but not the standard variables