cmake: Fix cache issue when integrating by downstream project

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.

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

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

hebasto commented at 10:23 am on May 11, 2024: member

cc @theuni

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

real-or-random added the label build on May 13, 2024

real-or-random added the label refactor/smell on May 13, 2024

real-or-random commented at 2:09 pm on May 13, 2024: contributor

utACK ec4c002faa350f02919fe0f710279d2922e254a1

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 ?

hebasto commented at 6:37 pm on May 13, 2024: member

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

https://github.com/bitcoin-core/secp256k1/blob/06bff6dec8d038f7b4112664a9b882293ebc5178/src/CMakeLists.txt#L34-L38

UPD: https://github.com/bitcoin-core/secp256k1/blob/06bff6dec8d038f7b4112664a9b882293ebc5178/CMakeLists.txt#L52

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.

real-or-random commented at 10:12 am on May 22, 2024: contributor

@theuni Want to review this? :)

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 )

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.

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? :)

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.

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.

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.

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.

theuni approved

theuni commented at 1:37 pm on June 12, 2024: contributor

utACK ec4c002faa350f02919fe0f710279d2922e254a1

real-or-random merged this on Jun 12, 2024

real-or-random closed this on Jun 12, 2024

hebasto deleted the branch on Jun 16, 2024

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