cmake: Fix cache issue when integrating by downstream project

[hebasto]

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.

[hebasto]

cc @theuni

[real-or-random]

utACK ec4c002

[theuni]

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

[hebasto]

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

secp256k1/src/CMakeLists.txt

Lines 34 to 38 in 06bff6d

	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>>
	$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>
	)

UPD:

secp256k1/CMakeLists.txt

Line 52 in 06bff6d

option(SECP256K1_INSTALL "Enable installation." ${PROJECT_IS_TOP_LEVEL})

[real-or-random]

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]

@theuni Want to review this? :)

[theuni]

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}>
 )

[theuni]

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]

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]

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

secp256k1/src/CMakeLists.txt

Lines 34 to 38 in 06bff6d

	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>>
	$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>
	)

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

   secp256k1/src/bench.c

   Line 10 in 06bff6d

   #include "../include/secp256k1.h"

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.

[real-or-random]

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]

utACK ec4c002