Bugfix | Unprefixed cmake options

[BenLeadbetter]

The Issue

The cmake options defined in the root project build script have very generic names which can easily clash with other cache variables when bdwgc is included as a subdirectory in a larger project.

The Fix

This patch prefixes the cmake options with bdwgc_ and introduces a deprecation system to warn clients about the change without breaking their builds.

[ivmai]

Could you please refer to some guide or popular project following this naming convention?

[ivmai]

AI output:

Prefixing (Optional but Recommended): For larger projects or when there's a potential for name clashes with external CMake modules, consider prefixing options with the project name or a specific module name. option(MYPROJECT_BUILD_DOCS "Build project documentation" OFF)

[ivmai]

But at the same time as mentioned in https://cmake.org/cmake/help/latest/variable/BUILD_SHARED_LIBS.html

CMake does not define BUILD_SHARED_LIBS by default, but projects often create a cache entry for it using the option command

[ivmai]

Please update the patch based on the comments above. Or let's discuss more if you don't agree.

[ivmai]

/cc @jeaye @plajjan

[ivmai]

This option was particularly problematic because it's actually a standard cmake variable so declaring it as an option here forces it ON globally for the whole project. This means that when bdwgc is included as a cmake subdir, targets higher up the tree will start unexpectedly building as dynamic libraries.

OTOH, are there any alternatives to renaming BUILD_SHARED_LIBS?

[jeaye]

This option was particularly problematic because it's actually a standard cmake variable so declaring it as an option here forces it ON globally for the whole project. This means that when bdwgc is included as a cmake subdir, targets higher up the tree will start unexpectedly building as dynamic libraries.

OTOH, are there any alternatives to renaming BUILD_SHARED_LIBS?

Thanks for the ping, Ivan. It sounds like this could be accomplished by:

1. Removing this line, since it changes a global CMake variable: https://github.com/bdwgc/bdwgc/pull/809/files#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aL62
2. Adding a new BDWGC_BUILD_SHARED_LIBS option

I agree that renaming every existing option is overkill and is going to break a lot of downstream systems. At this point, bdwgc has been around long enough to have a broad reach. We should not haphazardly break the CMake API just to clean things up. While the names bdwgc has can possibly conflict with other options, those problems are manageable as well.

This hasn't been a problem for me, when using bdwgc via add_subdirectory, since I set BUILD_SHARED_LIBS globally anyway. I also handle the caching of old values so I can set new ones just for BDWGC. For example:

Click to see CMake details.

set(CMAKE_C_FLAGS_OLD "${CMAKE_C_FLAGS}")
set(CMAKE_CXX_FLAGS_OLD "${CMAKE_CXX_FLAGS}")
set(BUILD_SHARED_LIBS_OLD ${BUILD_SHARED_LIBS})
set(CMAKE_CXX_CLANG_TIDY_OLD ${CMAKE_CXX_CLANG_TIDY})
  set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -w")
  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -w")

  if(NOT APPLE)
    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -DREDIRECT_MALLOC=GC_malloc_uncollectable -DREDIR_MALLOC_AND_LINUX_THREADS")
    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DREDIRECT_MALLOC=GC_malloc_uncollectable -DREDIR_MALLOC_AND_LINUX_THREADS")
  endif()

  set(BUILD_SHARED_LIBS OFF)
  set(CMAKE_CXX_CLANG_TIDY "")
  set(enable_cplusplus ON CACHE BOOL "Enable C++")
  set(build_cord OFF CACHE BOOL "Build cord")
  set(enable_docs OFF CACHE BOOL "Enable docs")
  set(enable_threads ON CACHE BOOL "Enable multi-threading support")
  set(enable_large_config ON CACHE BOOL "Optimize for large heap or root set")
  set(enable_throw_bad_alloc_library ON CACHE BOOL "Enable C++ gctba library build")
  set(enable_gc_debug OFF CACHE BOOL "Support for pointer back-tracing")

  if(jank_debug_gc)
    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -DGC_ASSERTIONS")
    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DGC_ASSERTIONS")
    set(enable_gc_debug ON CACHE BOOL "Support for pointer back-tracing")
  endif()

  add_subdirectory(third-party/bdwgc EXCLUDE_FROM_ALL)

  unset(enable_cplusplus)
  unset(build_cord)
  unset(enable_docs)
  unset(enable_threads)
  unset(enable_large_config)
  unset(enable_throw_bad_alloc_library)
  unset(enable_gc_debug)
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS_OLD}")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS_OLD}")
set(BUILD_SHARED_LIBS ${BUILD_SHARED_LIBS_OLD})
set(CMAKE_CXX_CLANG_TIDY ${CMAKE_CXX_CLANG_TIDY_OLD})

In general, I can agree that using prefixed variables is a good move. In jank, we do much the same. But such a change doesn't sound like it's worth breaking who-knows-how-many projects downstream.

Click to see CMake details.

# All options are available here.
option(jank_local_clang "Whether or not to use a local Clang/LLVM source build" OFF)
option(jank_install_local_clang "Whether or not to install the local Clang/LLVM alongside jank" ON)
option(jank_coverage "Enable code coverage measurement" OFF)
option(jank_analyze "Enable static analysis" OFF)
option(jank_test "Enable jank's test suite" OFF)
option(jank_unity_build "Optimize translation unit compilation for the number of cores" OFF)
option(jank_debug_gc "Enable GC debug assertions" OFF)
set(jank_sanitize "none" CACHE STRING "The type of Clang sanitization to use (or none)")
set(jank_resource_dir
  "../lib/jank/${CMAKE_PROJECT_VERSION}"
  CACHE STRING
  "Where jank's runtime files are installed. Relative paths are based on the runtime jank binary path")
set(jank_clojure_core_o "${CMAKE_BINARY_DIR}/core-libs/clojure/core.o")

[ivmai]

Okay, more clear now. As I understand the following should be done at least:

1. Prefix BUILD_SHARED_LIBS (with BDWGC_)
2. Pass SHARED or STATIC every add_library depending on BDWGC_BUILD_SHARED_LIBS (as explained in cmake manual)

However, what to do with "legacy" BUILD_SHARED_LIBS? Possible variants:

• ignore: then the clients (that need static build) should be updated by adding -DBDWGC_BUILD_SHARED_LIBS=OFF (or replacing -DBUILD_SHARED_LIBS=OFF)
• if BUILD_SHARED_LIBS is set to false then set BDWGC_BUILD_SHARED_LIBS to false

Correct?

[BenLeadbetter]

Okay, more clear now. As I understand the following should be done at least:

1. Prefix BUILD_SHARED_LIBS (with BDWGC_)

2. Pass SHARED or STATIC every add_library depending on BDWGC_BUILD_SHARED_LIBS (as explained in cmake manual)

However, what to do with "legacy" BUILD_SHARED_LIBS? Possible variants:

• ignore: then the clients (that need static build) should be updated by adding -DBDWGC_BUILD_SHARED_LIBS=OFF (or replacing -DBUILD_SHARED_LIBS=OFF)

• if BUILD_SHARED_LIBS is set to false then set BDWGC_BUILD_SHARED_LIBS to false

Correct?

Sounds good to me 👍🏻. I'll make that change today.

[ivmai]

I'll make that change today.

Wait, let's fully understand the logic first.

[ivmai]

Okay, more clear now. As I understand the following should be done at least:

So, we define BDWGC_BUILD_SHARED_LIBS as option with ON by default. But the client could also define BUILD_SHARED_LIBS variable (or it could be defined by top-level project before add_subdirectory).

And the expected variables usage is:

BDWGC_BUILD_SHARED_LIBS	BUILD_SHARED_LIBS	Linkage
ON/unspecified	unset	SHARED
ON/unspecified	ON	SHARED
ON/unspecified	OFF	?
OFF	*	STATIC

The question is what should we do when:

• cmake -D BDWGC_BUILD_SHARED_LIBS=ON -D BUILD_SHARED_LIBS=OFF (it should be SHARED logically)
• cmake -D BUILD_SHARED_LIBS=OFF (it should be STATIC logically)

But these two cases are not distinguishable in the cmake script because BDWGC_BUILD_SHARED_LIBS is ON by default.

[ivmai]

But these two cases are not distinguishable in the cmake script because BDWGC_BUILD_SHARED_LIBS is ON by default.

AI suggest the following code to distinguish between unset and set default:

option(MY_FEATURE "Enable my awesome feature" OFF) # Default to OFF

if(DEFINED MY_FEATURE AND NOT "${MY_FEATURE}" STREQUAL "")
    # MY_FEATURE is defined and not empty, indicating it was explicitly set.
    # Note: This check is more robust than just 'if(MY_FEATURE)' if you want to distinguish
    # between the default value and an explicitly set value that happens to be the same as the default.
    message(STATUS "MY_FEATURE was explicitly set to: ${MY_FEATURE}")
else()
    message(STATUS "MY_FEATURE was not explicitly set, using default value.")
endif()

[BenLeadbetter]

But at the same time as mentioned in https://cmake.org/cmake/help/latest/variable/BUILD_SHARED_LIBS.html

CMake does not define BUILD_SHARED_LIBS by default, but projects often create a cache entry for it using the option command

That's true, but it also mentions the caveat which is the exact same problem I encountered when pulling in bdwgc via cmake FetchContent:

Note that if bringing external dependencies directly into the build, such as with FetchContent or a direct call to add_subdirectory(), 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. Failure to do so can lead to different behavior between the first and subsequent CMake runs.

Defining such an option is an anti-pattern imo for this exact reason.

[BenLeadbetter]

Yeah the AI is right 😊 - even though both are "falsey" there's a diference between a variable being undefined and a variable being explicitly set OFF, so we should be able to capture the logic that you described above.

[ivmai]

Okay, @BenLeadbetter, please update the patch.

We might need a warning in case 'cmake -D BUILD_SHARED_LIBS=OFF'

[ivmai]

if(DEFINED MY_FEATURE AND NOT "${MY_FEATURE}" STREQUAL "")

No, the code provided by AI is not working correctly. The condition is always true.

[ivmai]

Hello @BenLeadbetter. After considering pros and cons, I've decided to apply part of your patch addressing exactly BUILD_SHARED_LIBS issue. Without backward compatibility. See PR #825. BUILD_SHARED_LIBS option was introduced in bdwgc v8.2.0, with the default value to build shared libs. Now (after the PR), the option is renamed to GC_BUILD_SHARED_LIBS, with the same default. Thus, the clients that need static (or configurable) linkage are affected - I think this should not be difficult for the clients to notice the change and update their part accordingly.

[ivmai]

Closing this PR in favor of #825