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

Integrate librepa and kdpart into the build system #4474

Draft
wants to merge 1 commit into
base: python
Choose a base branch
from

Conversation

RiccardoFrenner
Copy link
Contributor

@RiccardoFrenner RiccardoFrenner commented Mar 14, 2022

Fixes #4429

Description of changes:

  • Added CMake option WITH_LOAD_BALANCING
  • If enabled, librepa and kdpart are fetched from their repositories and a flag LOAD_BALANCING is set, similar to the walberla integration.
  • kdpart is installed at configure time, since it uses Makefiles and librepa tries to find it at configure time
  • librepa is only built when calling e.g. make after configuring
  • both kdpart and librepa needed extensive patching, some of the changes could be proposed upstream

@jngrad jngrad self-requested a review March 14, 2022 19:25
@RiccardoFrenner RiccardoFrenner marked this pull request as draft March 15, 2022 18:51
@jngrad jngrad force-pushed the fix-4429 branch 5 times, most recently from 0e6569b to 4757b14 Compare August 25, 2022 16:30
@jngrad jngrad changed the title WIP: Integrate librepa and kdpart into the build system Integrate librepa and kdpart into the build system Aug 25, 2022
Copy link
Member

@jngrad jngrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my side, this should work for a local build. I'm not sure if it works when running make install to deploy ESPResSo, since libkdpart.so is generated from a Makefile instead of being a proper target. Not sure if we can make it a target because it's consumed by librepa instead of ESPResSo, and it's unclear to me if librepa.so hardcodes the libkdpart.so runtime path or relies on the standard LD_LIBRARY_PATH environment variable. The only way to know for sure it to write some code in ESPResSo that calls a librepa symbol that itself calls a kdpart symbol.

@RudolfWeeber could you please take a look and make sure, this PR satisfies your requirements? I was able to compile and link ESPResSo against librepa.so and run the testsuite with this:

diff --git a/src/core/CMakeLists.txt b/src/core/CMakeLists.txt
index 9426568be..6944df47f 100644
--- a/src/core/CMakeLists.txt
+++ b/src/core/CMakeLists.txt
@@ -106,4 +106,6 @@ target_link_libraries(
          Boost::serialization Boost::mpi)
 
+target_link_libraries(espresso_core PRIVATE repa)
+target_include_directories(espresso_core PRIVATE ${librepa_SOURCE_DIR})
 target_include_directories(espresso_core PUBLIC ${CMAKE_CURRENT_SOURCE_DIR})
 
diff --git a/src/core/integrate.cpp b/src/core/integrate.cpp
index a424f7c24..61414c13d 100644
--- a/src/core/integrate.cpp
+++ b/src/core/integrate.cpp
@@ -70,4 +70,6 @@
 #include <utility>
 
+#include <repa/repa.hpp>
+
 #ifdef VALGRIND_INSTRUMENTATION
 #include <callgrind.h>

@jngrad
Copy link
Member

jngrad commented Sep 1, 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. This PR introduces a patch to add a prefix to all librepa CMake options that clash with ESPResSo.

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, and convince library developers to do the same to their projects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build-system integration for librepa
2 participants