Closed hebasto closed 2 weeks ago
@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
ormake test
instead ofmake 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 istest
), 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
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 thecheck
alias target is exposed to the downstream project build system, which in turn fails:This PR fixes this issue by deleting the
CTEST_TEST_TARGET_ALIAS
usage.