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

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.

cc @theuni

utACK ec4c002faa350f02919fe0f710279d2922e254a1

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

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

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.

@theuni Want to review this? :)

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:

diff --git a/src/secp256k1/src/CMakeLists.txt b/src/secp256k1/src/CMakeLists.txt
index 4cbaeb914d4..47e39f30bd4 100644
--- a/src/secp256k1/src/CMakeLists.txt
+++ b/src/secp256k1/src/CMakeLists.txt
@@ -33,7 +33,7 @@ set_target_properties(secp256k1_precomputed PROPERTIES POSITION_INDEPENDENT_CODE

 target_include_directories(secp256k1 INTERFACE
   # Add the include path for parent projects so that they don't have to manually add it.
-  $<BUILD_INTERFACE:$<$<NOT:$<BOOL:${PROJECT_IS_TOP_LEVEL}>>:${PROJECT_SOURCE_DIR}/include>>
+  $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../include>
   $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>
 )

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.

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

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.

2. 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.

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.

utACK ec4c002faa350f02919fe0f710279d2922e254a1