cmake: Set top-level target output locations #1553

pull hebasto wants to merge 2 commits into bitcoin-core:master from hebasto:240625-cmake-output changing 3 files +13 −11
  1. hebasto commented at 0:06 am on June 26, 2024: member

    While testing #1551, I noticed that when cross-compiling a shared library with examples for Windows, the ctest fails to run examples with Wine. Adjusting the PATH variable in https://github.com/bitcoin-core/secp256k1/blob/4af241b32099067464e015fa66daac5096206dea/examples/CMakeLists.txt#L16-L18 does not help because WINEPATH is expected.

    Another issue with the current implementation is that the examples cannot run individually on Windows.

    This PR resolves both issues by reverting the implementation from #1290 in favour of the reworked and improved implementation from #1233.

  2. real-or-random added the label assurance on Jun 26, 2024
  3. real-or-random added the label build on Jun 26, 2024
  4. real-or-random removed the label assurance on Jun 26, 2024
  5. real-or-random commented at 9:26 am on June 26, 2024: contributor

    ACK 3779472074d29125c06b715787005f3ac4f95cdf

    It’s probably a good idea that the binaries just work without the need to set a PATH. And it’s okay to collect all binary artifacts in a single directory, we don’t have that many.

  6. real-or-random commented at 9:26 am on June 26, 2024: contributor
    What are your commands for building on wine?
  7. hebasto commented at 10:19 am on June 26, 2024: member

    What are your commands for building on wine?

    On Ubuntu 24.04 with the wine-binfmt package installed:

     0$ cmake -B build --toolchain cmake/x86_64-w64-mingw32.toolchain.cmake -DSECP256K1_BUILD_EXAMPLES=ON
 1$ cmake --build build
 2$ ctest --test-dir build
 3Internal ctest changing into directory: /home/hebasto/git/secp256k1/secp256k1/build
 4Test project /home/hebasto/git/secp256k1/secp256k1/build
 5    Start 1: noverify_tests
 61/6 Test [#1](/bitcoin-core-secp256k1/1/): noverify_tests ...................   Passed   52.72 sec
 7    Start 2: tests
 82/6 Test [#2](/bitcoin-core-secp256k1/2/): tests ............................   Passed  111.58 sec
 9    Start 3: exhaustive_tests
103/6 Test [#3](/bitcoin-core-secp256k1/3/): exhaustive_tests .................   Passed    8.01 sec
11    Start 4: ecdsa_example
124/6 Test [#4](/bitcoin-core-secp256k1/4/): ecdsa_example ....................   Passed    1.35 sec
13    Start 5: ecdh_example
145/6 Test [#5](/bitcoin-core-secp256k1/5/): ecdh_example .....................   Passed    1.34 sec
15    Start 6: schnorr_example
166/6 Test [#6](/bitcoin-core-secp256k1/6/): schnorr_example ..................   Passed    1.32 sec
17
18100% tests passed, 0 tests failed out of 6
19
20Total Test time (real) = 176.31 sec
21$ ./build/bin/schnorr_example.exe 
22Is the signature valid? true
23Secret Key: 0x964362f6515fbb691dee49b936c8ba46dadd505eb4aa27f5db9b1ccf510f04a8
24Public Key: 0x286123fee26ecfc75ec8bd6d4e175a9b4fa729e7d2bcd4034661a33dee600d80
25Signature: 0xef0aa23ae13fd06710e31204a7a8ab52f0b726159b3cb2344c090220ed32d6dda958c9f7b3383c3fa7524a7f7ed54bf381c8a3bab1f8561072887dae6bf7bfe1
  8. fanquake commented at 2:50 pm on June 26, 2024: member

    implementation from #1233.

    cc @theuni

  9. real-or-random commented at 2:52 pm on August 19, 2024: contributor

    implementation from #1233.

    cc @theuni

    Friendly ping @theuni :)

  10. cmake: Set top-level target output locations 
    This change:
1. Collects build artifacts in dedicated locations.
2. Allows to run individual examples with a shared library on Windows.
3. Is compatible with Wine when testing cross-compiled Windows binaries
   on Linux.
4. Is compatible with integration the project into a larger project
   hierarchy.
    26e4a7c214
  11. Revert "cmake: Set `ENVIRONMENT` property for examples on Windows" 
    This reverts commit 116d2ab3df630455f23a7b21f50237689879ecc0.
    c232486d84
  12. hebasto force-pushed on Sep 18, 2024
  13. hebasto commented at 5:15 pm on September 18, 2024: member
    Rebased.
  14. theuni commented at 8:27 pm on September 18, 2024: contributor
    I’m not sure what I’m missing here. Why does moving from src/ to bin/ fix anything? And mirroring the source tree is kinda the convention for CMake, no? I’d expect to find them in src/ anyway.
  15. hebasto commented at 7:43 am on September 19, 2024: member

    Why does moving from src/ to bin/ fix anything?

    Moving built binaries from both src/ and example/ to bin/ does fix the issue because it places the executable and its dependency DLL in the same folder.

    FWIW, we have a similar issue with the libbitcoinkernel shared library in Bitcoin Core.

    And mirroring the source tree is kinda the convention for CMake, no?

    It’s the default behaviour adopted by many projects. However, here is a code snippet from the LLVM project:

    0set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)
1set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX})
2set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX})

    Here is a quote from Professional CMake: A Practical Guide 19th Edition, section 43.5.2, about the *_OUTPUT_DIRECTORY properties:

    A common use of these target properties is to collect libraries and executables together in a similar directory structure as they would have when installed. This is helpful if applications expect various resources to be located at a particular location relative to the executable’s binary. On Windows, it can also simplify debugging, since executables and DLLs can be collected into the same directory, allowing the executables to find their DLL dependencies automatically (this isn’t needed on other platforms, since RPATH support embeds the necessary locations in the binaries themselves).

  16. theuni commented at 5:51 pm on September 19, 2024: contributor

    Ok, I see. I think (from googling) the part that I was missing is that dll’s are put in CMAKE_RUNTIME_OUTPUT_DIRECTORY rather than CMAKE_LIBRARY_OUTPUT_DIRECTORY as I would’ve expected. Is that correct?

    For Linux at least, .so’s go to CMAKE_LIBRARY_OUTPUT_DIRECTORY.

  17. real-or-random commented at 8:31 am on September 20, 2024: contributor

    Ok, I see. I think (from googling) the part that I was missing is that dll’s are put in CMAKE_RUNTIME_OUTPUT_DIRECTORY rather than CMAKE_LIBRARY_OUTPUT_DIRECTORY as I would’ve expected. Is that correct?

    This matches the CMake docs: The file is a “runtime” output when add_library() is called with the SHARED option (which may be implied), and it’s a “library” output when add_library() is called with MODULE (which is intended for modules/plugins loaded dynamically at runtime.

  18. real-or-random commented at 12:08 pm on October 8, 2024: contributor
    ping @theuni
  19. theuni commented at 11:11 am on October 15, 2024: contributor

    Concept ACK and utACK c232486d84e21ece78b6cc956cd233cdf1bf8eee.

    Discussed with @hebasto in person. We’ve decided to switch all of Core’s CMake projects to use this layout (barring any unforeseen issues).

  20. real-or-random approved
  21. real-or-random commented at 11:36 am on October 15, 2024: contributor
    utACK c232486d84e21ece78b6cc956cd233cdf1bf8eee
  22. real-or-random merged this on Oct 15, 2024
  23. real-or-random closed this on Oct 15, 2024
  24. hebasto referenced this in commit 5c425b5703 on Oct 16, 2024
  25. hebasto commented at 8:49 pm on October 16, 2024: member
    This change might need a note in the CHANGELOG.md, no?
  26. real-or-random added the label needs-changelog on Oct 17, 2024
  27. achow101 referenced this in commit c24b343141 on Oct 24, 2024
  28. hebasto deleted the branch on Oct 26, 2024


hebasto real-or-random fanquake theuni

Labels
build needs-changelog

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-12-21 17:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me