cmake: Do not set `CTEST_TEST_TARGET_ALIAS` #1545

An alias for the "test" target can be confusing for the downstream project.

For instance, when integrating using add_subdirectory(secp256k1 EXCLUDE_FROM_ALL) (see https://github.com/hebasto/bitcoin/pull/192), test binaries are not being built by default. But the check alias target is exposed to the downstream project build system, which in turn fails:

$ make -C build check
...
Unable to find executable: /home/hebasto/git/bitcoin/build/src/secp256k1/src/exhaustive_tests
3/3 Test [#3](/bitcoin-core-secp256k1/3/): exhaustive_tests .................***Not Run   0.00 sec

0% tests passed, 3 tests failed out of 3

Total Test time (real) =   0.03 sec

The following tests FAILED:
	  1 - noverify_tests (Not Run)
	  2 - tests (Not Run)
	  3 - exhaustive_tests (Not Run)
Errors while running CTest
...

This PR fixes this issue by deleting the CTEST_TEST_TARGET_ALIAS usage.

@fanquake

This PR addresses your offline comment to https://github.com/hebasto/bitcoin/pull/192:

I can call the check target, but it fails because it doesn't find 2 of the bins. So it's kind of there, just broken

This PR fixes this issue by deleting the CTEST_TEST_TARGET_ALIAS usage.

Why is just deleting this the right fix? The check target works fine when run inside this project, and I assume was added here for a reason?

This PR fixes this issue by deleting the CTEST_TEST_TARGET_ALIAS usage.

Why is just deleting this the right fix? The check target works fine when run inside this project, and I assume was added here for a reason?

I don't think we have a strong preference to keep this alias. We added the alias to make sure that make check is the right command both for autotools and CMake, and to ease the transition for users familiar with autotools. But that's a somewhat questionable goal, given that many commands differ between the build systems anyway: if an autotools user needs to learn how to invoke cmake correctly, he can also learn to type ctest or make test instead of make check (and we document the native command in the README). On top of that, CTEST_TEST_TARGET_ALIAS is an undocumented feature, and this issue shows that it's not very mature.

My thinking is that it's a good idea to be consistent with Core. If Core does not want to support make check (or cannot support it due to this issue), it's okay to just get rid of it here.

utACK https://github.com/bitcoin-core/secp256k1/pull/1545/commits/f87a3589f4d3bfd7a398232971bdb1ff125b8e9d

Cool. If there's no real need to have it here, and it was implemented using an undocumented CMake feature, then deleting it seems fine. Wanted to be sure, as the solution of just deleting things that may be inconvinient for subprojects is probably not always going to be as convenient as this case.

... he can also learn to type ctest or make test instead of make check (and we document the native command in the README).

I would say that both make test and make check are inferior to ctest, as only the latter supports running tests in parallel using the -j option.

UPD. And ctest is agnostic to the underlying build system (make, ninja etc).

Hmm. I guess even after this we'll still have some targets exposed that aren't useful / are potentially confusing. i.e tests will be exposed (as opposed to the --target to run our tests, which is test), which will compile some secp256k1 tests, but not run them. Along with the other secp256k1 targets (ctime_tests, exhaustive_tests, noverify_tests).

Hmm. I guess even after this we'll still have some targets exposed that aren't useful / are potentially confusing. i.e tests will be exposed (as opposed to the --target to run our tests, which is test), which will compile some secp256k1 tests, but not run them. Along with the other secp256k1 targets (ctime_tests, exhaustive_tests, noverify_tests).

That's true.

See related @theuni's https://github.com/hebasto/bitcoin/pull/192#discussion_r1594428809.

@sipa Can you Concept (N)ACK this? I think you had asked for make check to be available in CMake (in addition to the "native" command ctest)

Concept ACK