cmake: Avoid contaminating parent project's cache with `BUILD_SHARED

hebasto commented at 11:25 am on June 19, 2025: member

The CMake cache is global in scope. Therefore, setting the standard cache variable BUILD_SHARED_LIBS can inadvertently affect the behavior of a parent project.

Consider configuring Bitcoin Core without explicit setting BUILD_SHARED_LIBS:

0$ cmake -B build -DBUILD_KERNEL_LIB=ON

According to CMake’s documentation, this should configure libbitcoinkernel as STATIC. However, that’s not the case:

0$ cmake --build build -t libbitcoinkernel
1[143/143] Linking CXX shared library lib/libbitcoinkernel.so

This PR:

Sets the BUILD_SHARED_LIBS cache variable only when libsecp256k1 is the top-level project.
Removes the SECP256K1_DISABLE_SHARED cache variable. This enables parent projects that include libsecp256k1 as a subproject to rely solely on standard CMake variables for configuring the library type. During integration into a parent project, the static library can be forced as demonstrated here.

hebasto commented at 11:25 am on June 19, 2025: member

cc @theuni

real-or-random added the label build on Jun 19, 2025

real-or-random added the label refactor/smell on Jun 19, 2025

real-or-random commented at 5:17 am on July 17, 2025: contributor

@theuni @purpleKarrot Can one of you review this?

purpleKarrot commented at 8:03 am on July 17, 2025: contributor

I’d rather drop the complete block of code. Every CMake project should behave well as a subproject. This becomes even more relevant with FetchContent getting more and more adoption. Not only should projects not interfere with superprojects (like setting BUILD_SHARED_LIBS or CMAKE_BUILD_TYPE in the cache), they should also not define their own settings that already have a default in CMake. Managing a superproject is much easier when you can rely on the assumption that projects can be controlled through standard CMake variables and you don’t have to remember separate names like FOO_BUILD_SHARED, BAR_NO_SHARED, BAZ_DISABLE_STATIC, etc.

real-or-random commented at 7:17 am on July 18, 2025: contributor

The CMake cache is global in scope

Sigh, this is again confusing as hell (at least to me as someone who is not a CMake expert). Let me try to organize the discussion.

What is the cleanest thing to do in a library?

@hebasto:

According to CMake’s documentation, […] @purpleKarrot: Not only should projects not interfere with superprojects (like setting BUILD_SHARED_LIBS or CMAKE_BUILD_TYPE in the cache) […]

The docs say that top-level projects should call option(BUILD_SHARED_LIBS ...). Do we agree that, under the assumption that we are in control of both the top-level project and the subproject, this is only the second-best thing? And the best thing to do is to avoid setting this variable in the subproject?

Can we drop the entire block?

I might be wrong, but wouldn’t dropping the option(BUILD_SHARED_LIBS ...) here mean that this variable can’t be set on the command line using -D (because it’s not even a cache variable then)? At least, this is how I read the docs. @purpleKarrot:

they should also not define their own settings that already have a default in CMake.

The thing is that, according to the docs, BUILD_SHARED_LIBS does not have a default.

If we can drop the entire block, should we do it?

I’m not sure, but I tend to say no. I’d rather keep the default of building a shared library and accept a few lines of CMake code. If we decide to drop it, we should at least document this in the README.

Do we need `SECP256K1_DISABLE_SHARED`?

Managing a superproject is much easier when you can rely on the assumption that projects can be controlled through standard CMake variables and you don’t have to remember separate names like FOO_BUILD_SHARED, BAR_NO_SHARED, BAZ_DISABLE_STATIC, etc.

I had to look this up. The reason why we have SECP256K1_DISABLE_SHARED is this: @theuni in #1230#issue-1615989593:

A new BUILD_SHARED_LIBS option is added to match CMake convention, as well as a SECP256K1_DISABLE_SHARED option which overrides it. That way even projects which have BUILD_SHARED_LIBS=1 can opt-into a static libsecp in particular. @purpleKarrot Are you saying this is not necessary? (Real question, I really don’t understand all of this stuff.)

hebasto commented at 10:54 am on July 18, 2025: member

The CMake cache is global in scope

Sigh, this is again confusing as hell (at least to me as someone who is not a CMake expert). Let me try to organize the discussion.

What is the cleanest thing to do in a library?

@hebasto:

According to CMake’s documentation, […]

@purpleKarrot:

Not only should projects not interfere with superprojects (like setting BUILD_SHARED_LIBS or CMAKE_BUILD_TYPE in the cache) […]

The docs say that top-level projects should call option(BUILD_SHARED_LIBS ...). Do we agree that, under the assumption that we are in control of both the top-level project and the subproject, this is only the second-best thing? And the best thing to do is to avoid setting this variable in the subproject?

This excerpt from the docs literally describes how fragile it is to set BUILD_SHARED_LIBS as a cache variable:

… if bringing external dependencies directly into the build … and one of those dependencies has such a call to option(BUILD_SHARED_LIBS …), the top level project must also call option(BUILD_SHARED_LIBS …) before bringing in its dependencies.

Additionally, from Professional CMake: A Practical Guide 21st Edition, Section 40.2:

Another common problem is modifying variables that have the potential to affect the whole build, not just the project… Quite often, these variables [BUILD_SHARED_LIBS, …] shouldn’t be defined by the project as cache variables at all…

Can we drop the entire block?

I might be wrong, but wouldn’t dropping the option(BUILD_SHARED_LIBS ...) here mean that this variable can’t be set on the command line using -D (because it’s not even a cache variable then)? At least, this is how I read the docs.

When set using -D, the variable becomes a cache variable.

@purpleKarrot:

they should also not define their own settings that already have a default in CMake.

The thing is that, according to the docs, BUILD_SHARED_LIBS does not have a default.

The default state of BUILD_SHARED_LIBS is “undefined”. And the behaviour of the add_library command is well-defined in this case.

If we can drop the entire block, should we do it?

I’m not sure, but I tend to say no. I’d rather keep the default of building a shared library and accept a few lines of CMake code. If we decide to drop it, we should at least document this in the README.

I agree with that. That’s why I chose the suggested approach.

A shared library is a real library and should be considered as the default by-product of the build process.

Do we need SECP256K1_DISABLE_SHARED?

Managing a superproject is much easier when you can rely on the assumption that projects can be controlled through standard CMake variables and you don’t have to remember separate names like FOO_BUILD_SHARED, BAR_NO_SHARED, BAZ_DISABLE_STATIC, etc.

I had to look this up. The reason why we have SECP256K1_DISABLE_SHARED is this:

@theuni in #1230 (comment):

A new BUILD_SHARED_LIBS option is added to match CMake convention, as well as a SECP256K1_DISABLE_SHARED option which overrides it. That way even projects which have BUILD_SHARED_LIBS=1 can opt-into a static libsecp in particular.

@purpleKarrot Are you saying this is not necessary? (Real question, I really don’t understand all of this stuff.)

Using project-specific versions of well-known CMake variables is hard to maintain consistent. This project defines its own variable to override CMake’s BUILD_SHARED_LIBS, but omits others such as CMAKE_EXPORT_COMPILE_COMMANDS. Specifying the library type is not the only concern when integrating into a parent project. In Bitcoin Core, we currently use an approach that encapsulates the entire integration logic within a function.

purpleKarrot commented at 12:48 pm on July 18, 2025: contributor

Real question, I really don’t understand all of this stuff.

I hear you, @real-or-random. That deserves a longer explanation. I will provide extensive information with examples on the weekend.

purpleKarrot commented at 0:19 am on July 22, 2025: contributor

I hope to answer most of your questions here: https://njump.me/nevent1qqsyfmk89g3cfvq8u9p6hg3x4nd0j66tjgwkth45wpz493hv4f5z70spp4mhxue69uhkummn9ekx7mqzyq8sv8lyu6zjy5p8fmfa0pn4j95rvmr2rj7urnpdhvkk5r55fndjkd3z5dj

real-or-random commented at 11:51 am on July 22, 2025: contributor

That’s a great post!

What’s your conclusion for this PR? From the post, it appears that you support the if(PROJECT_IS_TOP_LEVEL) approach as currently implemented in this PR. But you said above that you’d rather drop the complete block of code. Did you change your mind when writing the post?

It does not base the condition on GNUC, because it regards the necessary declaration as a property of the platform rather than a capability of the compiler.

The point of the __GNUC__ is to check that the compiler supports __attribute__((visibility("default"))) at all. You wouldn’t want to set it on some non-GCC and non-Clang compiler.

theuni commented at 2:07 pm on July 22, 2025: contributor

The point of the __GNUC__ is to check that the compiler supports __attribute__((visibility("default"))) at all. You wouldn’t want to set it on some non-GCC and non-Clang compiler.

Note that modern gcc/clang support has_attribute. It can be used like this:

0#if defined(__has_attribute)
1#  if __has_attribute(visibility)
2#    define HAS_VISIBILITY_ATTRIBUTE
3#  endif
4#endif
5#ifdef HAS_VISIBILITY_ATTRIBUTE
6#  define DEFAULT_VISIBILITY_ATTRIBUTE __attribute__((visibility("default")))
7#else
8#  define DEFAULT_VISIBILITY_ATTRIBUTE
9#endif

As illustrated here.

Of course to be useful for libsecp it’d need a fallback for older versions.

purpleKarrot commented at 4:17 pm on July 22, 2025: contributor

That’s a great post!

Thanks! It was a huge effort.

What’s your conclusion for this PR? From the post, it appears that you support the if(PROJECT_IS_TOP_LEVEL) approach as currently implemented in this PR. But you said above that you’d rather drop the complete block of code. Did you change your mind when writing the post?

In my post, I show several approaches and possibilities.

Project Qux sets BUILD_SHARED_LIBS as an option based on Qux_IS_TOP_LEVEL, similar to this PR. I prefer to use project specific variables in CMakeLists.txt files and use generic variables like PROJECT_IS_TOP_LEVEL in generic code.

In project Bar, I show that no such option is actually needed for building with both BUILD_SHARED_LIBS=ON and BUILD_SHARED_LIBS=OFF.

In project Baz, I show that even with BUILD_SHARED_LIBS globally set to ON, it is possible to set it to a different value for a subproject, without there being a QUX_DISABLE_SHARED option.

For this PR, it means I would definitely drop the else part, and preferably the complete block of code.

purpleKarrot commented at 4:49 pm on July 22, 2025: contributor

Note that modern gcc/clang support has_attribute.

Clang supports visibility since version 2.0 and __has_attribute since version 2.9. How should __has_attribute(visibility) ever be false? Maybe for targeting embedded platforms, but you would not want to build shared libraries for those.

I am aware of https://github.com/bitcoin-core/secp256k1/commit/1309c03c45beece646a7d21fdb6a0e3d38adee2b, but I cannot tell whether there is a actual use case to support compilers older than bitcoin, or whether that change was motivated by because-we-can.

But yeah, that is off-topic for this PR. Please use nostr to comment on the blog post.

real-or-random commented at 8:02 am on July 23, 2025: contributor

In project Baz, I show that even with BUILD_SHARED_LIBS globally set to ON, it is possible to set it to a different value for a subproject, without there being a QUX_DISABLE_SHARED option.

For this PR, it means I would definitely drop the else part, and preferably the complete block of code.

Ok, I get that we don’t need SECP256K1_DISABLE_SHARED because the parent project can put BUILD_SHARED_LIBS in a block() (IIUC), so we don’t need the “else” branch.

Maybe it’s just me, but I still can’t follow why you prefer dropping the “then” branch? I still think that defaulting to a shared library is a good idea.

Project Qux sets BUILD_SHARED_LIBS as an option based on Qux_IS_TOP_LEVEL, similar to this PR. I prefer to use project specific variables in CMakeLists.txt files and use generic variables like PROJECT_IS_TOP_LEVEL in generic code.

Okay, I wasn’t aware of <PROJECT-NAME>_IS_TOP_LEVEL, and your convention makes sense to me.

in CMakeLists.txt:39 in 533aea12f1 outdated

42+else()
43+  option(SECP256K1_DISABLE_SHARED "Disable shared library. Overrides BUILD_SHARED_LIBS." OFF)
44+  if(SECP256K1_DISABLE_SHARED)
45+    set(BUILD_SHARED_LIBS OFF)
46+  endif()
47 endif()

real-or-random commented at 8:03 am on July 23, 2025:

So my current thinking is that we do this:

0if(libsecp256k1_IS_TOP_LEVEL)
1  option(BUILD_SHARED_LIBS "Build shared libraries." ON)
2endif()

cmake: Avoid contaminating parent project's cache with BUILD_SHARED_LIBS

The CMake cache is global in scope. Therefore, setting the standard
cache variable `BUILD_SHARED_LIBS` can inadvertently affect the behavior
of a parent project.

This change:
1. Sets the `BUILD_SHARED_LIBS` cache variable only when libsecp256k1 is
   the top-level project.
2. Removes the `SECP256K1_DISABLE_SHARED` cache variable. This enables
   parent projects that include libsecp256k1 as a subproject to rely
   solely on standard CMake variables for configuring the library type.

7b07b22957

hebasto force-pushed on Jul 27, 2025

hebasto commented at 2:36 pm on July 27, 2025: member

Reworked per discussion.

theuni approved

theuni commented at 5:11 pm on July 29, 2025: contributor

utACK 7b07b229571d80c228f7719c4ffdaf48e397f1a0

real-or-random commented at 7:11 am on July 30, 2025: contributor

@purpleKarrot Want to take a look at the updated version?

purpleKarrot commented at 8:29 am on July 30, 2025: contributor

ACK 7b07b229571d80c228f7719c4ffdaf48e397f1a0

real-or-random merged this on Jul 30, 2025

real-or-random closed this on Jul 30, 2025

hebasto deleted the branch on Jul 30, 2025

cmake: Avoid contaminating parent project’s cache with BUILD_SHARED_LIBS #1688

What is the cleanest thing to do in a library?

Can we drop the entire block?

If we can drop the entire block, should we do it?

Do we need SECP256K1_DISABLE_SHARED?

What is the cleanest thing to do in a library?

Can we drop the entire block?

If we can drop the entire block, should we do it?

Do we need SECP256K1_DISABLE_SHARED?

cmake: Avoid contaminating parent project’s cache with `BUILD_SHARED_LIBS` #1688

Do we need `SECP256K1_DISABLE_SHARED`?

Do we need `SECP256K1_DISABLE_SHARED`?