cmake: Do not set CTEST_TEST_TARGET_ALIAS #1545

pull hebasto wants to merge 1 commits into bitcoin-core:master from hebasto:240620-test-alias changing 1 files +0 −4
  1. hebasto commented at 8:25 pm on June 20, 2024: member

    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:

     0$ make -C build check
 1...
 2Unable to find executable: /home/hebasto/git/bitcoin/build/src/secp256k1/src/exhaustive_tests
 33/3 Test [#3](/bitcoin-core-secp256k1/3/): exhaustive_tests .................***Not Run   0.00 sec
 4
 50% tests passed, 3 tests failed out of 3
 6
 7Total Test time (real) =   0.03 sec
 8
 9The following tests FAILED:
10	  1 - noverify_tests (Not Run)
11	  2 - tests (Not Run)
12	  3 - exhaustive_tests (Not Run)
13Errors while running CTest
14...

    This PR fixes this issue by deleting the CTEST_TEST_TARGET_ALIAS usage.

  2. cmake: Do not set `CTEST_TEST_TARGET_ALIAS` 
    An alias for the "test" target can be confusing for the downstream
project. So remove it.
    f87a3589f4
  3. hebasto commented at 8:27 pm on June 20, 2024: member

    @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

  4. real-or-random added the label build on Jun 21, 2024
  5. fanquake commented at 8:29 am on June 21, 2024: member

    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?

  6. real-or-random commented at 8:52 am on June 21, 2024: contributor

    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

  7. fanquake commented at 9:04 am on June 21, 2024: member
    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.
  8. hebasto commented at 9:18 am on June 21, 2024: member

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

  9. fanquake commented at 9:19 am on June 21, 2024: member
    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).
  10. hebasto commented at 9:23 am on June 21, 2024: member

    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.

  11. real-or-random commented at 10:39 am on June 22, 2024: contributor
    @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)
  12. sipa commented at 5:17 pm on June 24, 2024: contributor
    Concept ACK
  13. real-or-random merged this on Jun 24, 2024
  14. real-or-random closed this on Jun 24, 2024
  15. hebasto deleted the branch on Jun 24, 2024


hebasto fanquake real-or-random sipa

Labels
build

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-21 08:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me