cmake: Fix cache issue when integrating by downstream project #1529

pull hebasto wants to merge 2 commits into bitcoin-core:master from hebasto:240511-cmake-cache changing 1 files +6 −7
  1. hebasto commented at 10:23 am on May 11, 2024: member
    As CMake’s cache is a global database, modifying it within a project integrated with the add_subdirectory() command, which may also include using the FetchContent module, could potentially affect downstream projects and sibling ones.
  2. cmake: Do not set emulated PROJECT_IS_TOP_LEVEL as cache variable
    Otherwise, downstream projects, which integrate the libsecp256k1 library
    using the `add_subdirectory()` command, will be affected.
    cae9a7ad14
  3. cmake: Simplify `PROJECT_IS_TOP_LEVEL` emulation
    Detecting whether it is the top level by comparing the value of
    `CMAKE_SOURCE_DIR` with `CMAKE_CURRENT_SOURCE_DIR` is supported by all
    versions of CMake and is a very common pattern.
    ec4c002faa
  4. hebasto commented at 10:23 am on May 11, 2024: member
    cc @theuni
  5. hebasto renamed this:
    Fix CMake cache issue when integrating by downstream project
    cmake: Fix cache issue when integrating by downstream project
    on May 11, 2024
  6. real-or-random added the label build on May 13, 2024
  7. real-or-random added the label refactor/smell on May 13, 2024
  8. real-or-random commented at 2:09 pm on May 13, 2024: contributor
    utACK ec4c002faa350f02919fe0f710279d2922e254a1
  9. theuni commented at 6:25 pm on May 13, 2024: contributor
    Since we (downstream) can/will use EXCLUDE_FROM_ALL, is there any real reason to keep support for PROJECT_IS_TOP_LEVEL ?
  10. hebasto commented at 6:37 pm on May 13, 2024: member
  11. real-or-random commented at 7:39 am on May 14, 2024: contributor

    Since we (downstream) can/will use EXCLUDE_FROM_ALL, is there any real reason to keep support for PROJECT_IS_TOP_LEVEL ?

    And perhaps other downstream projects will need it.

  12. real-or-random commented at 10:12 am on May 22, 2024: contributor
    @theuni Want to review this? :)
  13. theuni commented at 5:41 pm on May 22, 2024: contributor

    I’m still not convinced that PROJECT_IS_TOP_LEVEL should be used at all. It seems to me to indicate we’re not doing something right.

    Downstreams can use EXCLUDE_FROM_ALL or SECP256K1_INSTALL (a simplified, without the PROJECT_IS_TOP_LEVEL conditional) if they don’t want to install.

    For example, this seems to work as expected:

     0diff --git a/src/secp256k1/src/CMakeLists.txt b/src/secp256k1/src/CMakeLists.txt
     1index 4cbaeb914d4..47e39f30bd4 100644
     2--- a/src/secp256k1/src/CMakeLists.txt
     3+++ b/src/secp256k1/src/CMakeLists.txt
     4@@ -33,7 +33,7 @@ set_target_properties(secp256k1_precomputed PROPERTIES POSITION_INDEPENDENT_CODE
     5
     6 target_include_directories(secp256k1 INTERFACE
     7   # Add the include path for parent projects so that they don't have to manually add it.
     8-  $<BUILD_INTERFACE:$<$<NOT:$<BOOL:${PROJECT_IS_TOP_LEVEL}>>:${PROJECT_SOURCE_DIR}/include>>
     9+  $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../include>
    10   $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>
    11 )
    
  14. theuni commented at 5:44 pm on May 22, 2024: contributor

    Ok, I see. The weirdness comes from the way that secp includes headers internally. Rather than adding the include path to every compile invocation (as I’d expect), files are included relatively like #include "../include/secp256k1.h".

    So the above would add an unnecessary include path.

  15. theuni commented at 6:01 pm on May 22, 2024: contributor

    Now I’m even more confused. The above actually seems to do the correct thing in all cases. The path is not included when doing a secp bulid, but it is included when using it from a downstream project.

    What am I missing? :)

  16. hebasto commented at 10:59 am on May 23, 2024: member

    The public header includes were changed in #925. When building the library itself, in the BUILD_INTERFACE context, providing an -I compiler flag that refers to the include subdirectory is not needed anymore.

    However, when the library is integrated by a downstream project in its own build tree, for example, with using add_subdirectory() command, the secp256k1 target must provide the INTERFACE_INCLUDE_DIRECTORIES property with a path to the include subdirectory as a part of its usage requirements. Otherwise, the downstream project will likely fail to find the public headers.

    Let’s break down the following code https://github.com/bitcoin-core/secp256k1/blob/06bff6dec8d038f7b4112664a9b882293ebc5178/src/CMakeLists.txt#L34-L38

    This command sets the INTERFACE_INCLUDE_DIRECTORIES property of the secp256k1 target, which means it is a part of the usage requirements and is dedicated to the library consumers.

    There are two kinds of such consumers: internal and external ones.

    1. Internal consumers are tests and benchmarks that refer to public headers using a relative path like https://github.com/bitcoin-core/secp256k1/blob/06bff6dec8d038f7b4112664a9b882293ebc5178/src/bench.c#L10

    Such consumers do not need any include directories to be specified, and it is the case now because PROJECT_IS_TOP_LEVEL holds TRUE.

    1. External consumers are downstream projects that include this project to their build trees using an add_subdirectory(secp256k1) command. In that case, INTERFACE_INCLUDE_DIRECTORIES will include a path to .../secp256k1/include directory.

    In short, not using PROJECT_IS_TOP_LEVEL leads to unnecessary -I/path/to/include flag when compiling tests and benchmarks.

  17. real-or-random commented at 1:20 pm on May 26, 2024: contributor

    What Hennadii says makes sense to me. And I think we’d like to support building without -I and avoid adding unnecessary include paths. Then it’s easier to spot “wrong” #include paths that work only with -I. (We should add a “manual” build to CI, but that’s a different story.) Unfortunately, it’s impossible to prevent automake from adding -I $srcdir", but we have full control in CMake.

    In any case, this PR seems to be an improvement over what we have currently. We can still remove PROJECT_IS_TOP_LEVEL in another PR if it turns out that we shouldn’t rely on it.

  18. theuni approved
  19. theuni commented at 1:37 pm on June 12, 2024: contributor
    utACK ec4c002faa350f02919fe0f710279d2922e254a1
  20. real-or-random merged this on Jun 12, 2024
  21. real-or-random closed this on Jun 12, 2024

  22. hebasto deleted the branch on Jun 16, 2024

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-06 04:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me