Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMake option name clashes with FetchContent #4618

Closed
jngrad opened this issue Nov 29, 2022 · 0 comments · Fixed by #4612
Closed

CMake option name clashes with FetchContent #4618

jngrad opened this issue Nov 29, 2022 · 0 comments · Fixed by #4612
Assignees
Labels

Comments

@jngrad
Copy link
Member

jngrad commented Nov 29, 2022

This is a follow-up to the dev-meeting discussion about name clashes when including external libraries with FetchContent.

CMake target name clashes

ESPResSo CMake target names cannot clash with included libraries thanks to the use of a unique prefix:

add_library(espresso_core SHARED)
add_library(espresso::core ALIAS espresso_core)
target_sources(espresso_core PRIVATE cells.cpp ...)
target_link_libraries(espresso_core PRIVATE espresso::config ...)
install(TARGETS espresso_core
        LIBRARY DESTINATION ${ESPRESSO_INSTALL_PYTHON}/espressomd)

Advantages:

  • no name clashes for shared objects: files always prefixed by espresso_
    • this was also necessary for Cython: shapes.py no longer clashes with shapes.so (renamed to espresso_shapes.so)
  • no name clashes for CMake targets: targets always prefixed by espresso_
    • all modifiers have to use the espresso_ target: target_sources(), target_link_libraries(), install()
    • all consumers have to use the espresso:: alias, including any parent CMake project
  • if a consumer tries to link against a non-existing espresso::target_name, CMake throws an error; linking against a non-existing espresso_target_name doesn't throw an error, instead the string is passed to the linker, which may or may not throw an error

CMake project option clashes

Regarding CMake project options, to my knowledge there isn't a built-in way to namespace them. Contrary to ExternalProject, which provides CMAKE_ARGS and CMAKE_CACHE_ARGS to pass CMake options to the external CMake project, FetchContent doesn't allow providing different CMake options, because the CMakeCache.txt of the subproject is merged into the root project. There have been several discussions about improving FetchContent, see discussions in cmake/cmake#23710 (comment) and cmake/cmake#22687, but none of them addresses this situation.

There are several workarounds.

The first one consists in defining a subset of common CMake options that can be safely inherited from a parent project, and automatically disable all other options. The MWE below is adapted from
Preparing libraries for CMake FetchContent by Jonathan:

project(ESPRESSO LANGUAGES CXX)

# define common build options that are inherited from parent projects
option(WITH_COVERAGE "Enable code coverage instrumentation" OFF)
add_subdirectory(src)

# define build options specific to this project, that
# cannot be activated if this project is a subproject
if(CMAKE_CURRENT_SOURCE_DIR STREQUAL CMAKE_SOURCE_DIR)
  # we're in the root, define additional targets for developers.
  option(ESPRESSO_WITH_TESTS      "Enable tests" ON)
  if(ESPRESSO_WITH_TESTS)
    add_subdirectory(tests)
  endif()
endif()

The downside is, some project-specific options shouldn't be hidden, e.g. WITH_HDF5, but at the same time they may clash with parent projects.

The second workaround consists in setting the CMake options before the add_subdirectory() command, while taking care of stashing the root project variables and unstashing them later with the same properties (cache, docstring) to avoid issues with variable lifetime and ccmake, and making sure these properties are always manually updated whenever the original option declarations change. The MWE below is adapted from Using FetchContent_Declare together with CMAKE_ARGS by Tsyvarev:

if(NOT librepa_POPULATED)
  FetchContent_Populate(librepa)
  set(WITH_TESTS_OLD ${WITH_TESTS})
  set(WITH_TESTS OFF CACHE INTERNAL "")
  add_subdirectory(${librepa_SOURCE_DIR} ${librepa_BINARY_DIR})
  set(WITH_TESTS ${WITH_TESTS_OLD} CACHE BOOL "Enable tests" FORCE)
endif()

The third method consists in manually prefixing all CMake options with the project name, i.e.

cmake .. -D ESPRESSO_WITH_HDF5=ON -D ESPRESSO_WITH_TESTS=ON

This is the solution taken by the waLBerla project.

In my opinion, the third method is the safest and most sustainable strategy. We would need to add a prefix to our own CMake options to avoid name collisions, and rely on these options instead of e.g. FFTW_FOUND variables that can be inherited from a parent project that includes ESPResSo. We should also convince library developers to do the same in their projects (SC-SGS/repa#51, walberla/walberla#191).

Originally posted by @jngrad in #4474 (comment)

@jngrad jngrad added the CMake label Nov 29, 2022
@jngrad jngrad self-assigned this Nov 30, 2022
@kodiakhq kodiakhq bot closed this as completed in #4612 Dec 5, 2022
kodiakhq bot added a commit that referenced this issue Dec 5, 2022
Fixes #4618

We have concrete plans to integrate more external libraries into ESPResSo, such as waLBerla for hydrodynamics and librepa+kdpart for MD load balancing. We opted for the CMake `FetchContent` method, which allows us to finely tune the way these libraries are compiled and integrated into ESPResSo. However, our build system has imperfections that makes it difficult to integrate new libraries, or to use ESPResSo as a library in a parent project. In particular, CMake flags such as `WITH_FFTW` are inherited by included projects, which sometimes cause them to build components that are not needed in ESPResSo. This unnecessarily increases build times, and in a few cases, forces users to install extra dependencies to complete the build.

It is good practice to put a project-specific prefix on all CMake options to avoid this kind of name conflict. While we worked with external libraries maintainers to rename their CMake options, we obviously need to rename ESPResSo CMake options too. As such, this PR adds a prefix `ESPRESSO_BUILD_` to all options that control compilation, and a prefix `ESPRESSO_` to the other options. The new option names can be found in the user guide, section 2.4.4. "Options and Variables".

Since the waLBerla project requires C++17, and CUDA versions 10.x only support C++14, we will be migrating to C++17 and CUDA 11.x. This change is also motivated by the `thrust` library, which generates compiler warnings for certain containers until CUDA 11.5, and Cython which now uses C++17 by default in Ubuntu 22.04.

Description of changes:
- rename ESPResSo-specific CMake options and functions
   - e.g. for build options: `-D WITH_CUDA=ON` becomes `-D ESPRESSO_BUILD_WITH_CUDA=ON`
   - e.g. for other options: `-D CTEST_ARGS=-j16` becomes `-D ESPRESSO_CTEST_ARGS=-j16`
- require a C++17-capable compiler
   - replace 400 lines of code in the core by equivalent STL functions
   - reduce usage of Boost algorithms
   - check the project builds without external dependencies in C++20 mode
- bump all version requirements
   - CMake >= 3.18, Python >= 3.9, Cython >= 0.29.21, Boost >= 1.74, CUDA >= 11.0, OpenMPI >= 4.0, MPICH >= 3.4.1, GCC >= 8.0, Clang >= 9.0.0, AppleClang >= 11.0.0, Intel Classic >= 18.0, Intel oneAPI >= 2021.0
   - Python package version requirements are based on versions available in Ubuntu 22.04 and Debian 11
- `FindCython` now stops CMake generation when Cython was not found
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant