From 2a2a6eea8ca578ed33cf4fe9ed7e7c8652e292b6 Mon Sep 17 00:00:00 2001 From: Jonas Schuhmacher Date: Tue, 15 Oct 2024 13:51:22 +0200 Subject: [PATCH 1/4] update cmake setup --- CMakeLists.txt | 1 + cmake/clang_format.cmake | 27 +++++++++++++++++++++++++++ cmake/gtest.cmake | 31 ++++++++++++++++++------------- cmake/pybind11.cmake | 20 ++++++++------------ cmake/spdlog.cmake | 35 +++++++++++++---------------------- cmake/tbb.cmake | 29 +++++++++++++++++++---------- cmake/thrust.cmake | 14 ++++++-------- cmake/xsimd.cmake | 18 ++++++------------ cmake/yaml.cmake | 17 ++++++----------- 9 files changed, 104 insertions(+), 88 deletions(-) create mode 100644 cmake/clang_format.cmake diff --git a/CMakeLists.txt b/CMakeLists.txt index 380051c..ff7c9be 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -69,6 +69,7 @@ include(thrust) include(spdlog) include(tetgen) include(xsimd) +include(clang_format) ############################### # Thrust Parallelization Set-Up diff --git a/cmake/clang_format.cmake b/cmake/clang_format.cmake new file mode 100644 index 0000000..87d9bcb --- /dev/null +++ b/cmake/clang_format.cmake @@ -0,0 +1,27 @@ +file(GLOB_RECURSE CLANG_FORMAT_SRC + "${PROJECT_SOURCE_DIR}/src/*.cpp" + "${PROJECT_SOURCE_DIR}/src/*.h" + "${PROJECT_SOURCE_DIR}/test/*.cpp" + "${PROJECT_SOURCE_DIR}/test/*.h" +) + +# Define a variable for clang-format command +find_program(CLANG_FORMAT clang-format) + +# Ensure clang-format was found +if(NOT CLANG_FORMAT) + message(STATUS "HPCLab: clang-format not found. Please install it to use clang-format via CMake") +else() + message(STATUS "HPCLab: clang-format found. You can format all source files via `cmake --build . --target format`") + add_custom_command( + OUTPUT format_all_files + COMMAND ${CLANG_FORMAT} -i ${CLANG_FORMAT_SRC} + WORKING_DIRECTORY ${PROJECT_SOURCE_DIR} + COMMENT "Formatting all source and test files with clang-format" + VERBATIM + ) + + add_custom_target(format + DEPENDS format_all_files + ) +endif() diff --git a/cmake/gtest.cmake b/cmake/gtest.cmake index c968cce..9350769 100644 --- a/cmake/gtest.cmake +++ b/cmake/gtest.cmake @@ -1,18 +1,23 @@ include(FetchContent) -message(STATUS "Setting up gtest") +message(STATUS "Setting up Google Test") +set(GOOGLE_TEST_VERSION 1.15.2) + +find_package(GTest ${GOOGLE_TEST_VERSION} QUIET) -#Adapted from https://cliutils.gitlab.io/modern-cmake/chapters/testing/googletest.html -#Fetches the version 1.13.0 from the official github for googletest -FetchContent_Declare(googletest - GIT_REPOSITORY https://github.com/google/googletest.git - GIT_TAG v1.13.0 - ) -FetchContent_MakeAvailable(googletest) +if(${GTest_FOUND}) + message(STATUS "Found existing Google Test: ${GTest_DIR}") +else() + message(STATUS "Using Google Test from GitHub Release ${GOOGLE_TEST_VERSION}") + + FetchContent_Declare(googletest + GIT_REPOSITORY https://github.com/google/googletest.git + GIT_TAG v${GOOGLE_TEST_VERSION} + ) + FetchContent_MakeAvailable(googletest) -# Disable warnings from the library target -target_compile_options(gtest_main PRIVATE -w) -# Disable warnings from included headers -get_target_property(propval gtest_main INTERFACE_INCLUDE_DIRECTORIES) -target_include_directories(gtest_main SYSTEM PUBLIC "${propval}") \ No newline at end of file + target_compile_options(gtest_main PRIVATE -w) + get_target_property(propval gtest_main INTERFACE_INCLUDE_DIRECTORIES) + target_include_directories(gtest_main SYSTEM PUBLIC "${propval}") +endif() diff --git a/cmake/pybind11.cmake b/cmake/pybind11.cmake index be320d9..8c85ae2 100644 --- a/cmake/pybind11.cmake +++ b/cmake/pybind11.cmake @@ -1,22 +1,18 @@ include(FetchContent) -message(STATUS "Setting up pybind11") +message(STATUS "Setting up Pybind11 Library") +set(PYBIND11_VERSION 2.12.0) -find_package(pybind11 2.12.0 QUIET) - -if (${pybind11_FOUND}) - - message(STATUS "Using local pybind11 installation") +find_package(pybind11 ${PYBIND11_VERSION} QUIET) +if(${pybind11_FOUND}) + message(STATUS "Found existing Pybind11 Library: ${pybind11_DIR}") else() - - message(STATUS "Using pybind11 from git repository") + message(STATUS "Using Pybind11 Library from GitHub Release ${PYBIND11_VERSION}") FetchContent_Declare(pybind11 GIT_REPOSITORY https://github.com/pybind/pybind11 - GIT_TAG v2.12.0 + GIT_TAG v${PYBIND11_VERSION} ) - FetchContent_MakeAvailable(pybind11) - -endif() +endif() \ No newline at end of file diff --git a/cmake/spdlog.cmake b/cmake/spdlog.cmake index d5a8d0f..cbb0e49 100644 --- a/cmake/spdlog.cmake +++ b/cmake/spdlog.cmake @@ -1,33 +1,24 @@ include(FetchContent) message(STATUS "Setting up spdlog") +set(SPDLOG_VERSION 1.14.1) -find_package(spdlog 1.13.0 QUIET) - -if (${spdlog_FOUND}) - - message(STATUS "Using existing spdlog installation") +# Known Issue: +# If you install spdlog@1.14.1 via homebrew on ARM macOS, CMake will find spdlog +# However, there is a version mismatch between the `fmt` library installed as dependency, and the one actually +# being required leading to a linking error (i.e. missing symbols) while compiling! +find_package(spdlog ${SPDLOG_VERSION} QUIET) +if(${spdlog_FOUND}) + message(STATUS "Found existing spdlog Library: ${spdlog_DIR}") else() - - message(STATUS "Using spdlog from git repository") - + message(STATUS "Using Spdlog Library from GitHub Release ${SPDLOG_VERSION}") FetchContent_Declare(spdlog GIT_REPOSITORY https://github.com/gabime/spdlog.git - GIT_TAG v1.13.0 + GIT_TAG v${SPDLOG_VERSION} ) - - # Disable stuff we don't need - option(SPDLOG_BUILD_EXAMPLE "" OFF) - option(SPDLOG_BUILD_TESTS "" OFF) - option(SPDLOG_INSTALL "" OFF) - + set(SPDLOG_BUILD_EXAMPLE OFF CACHE BOOL "" FORCE) + set(SPDLOG_BUILD_TESTS OFF CACHE BOOL "" FORCE) + set(SPDLOG_INSTALL OFF CACHE BOOL "" FORCE) FetchContent_MakeAvailable(spdlog) - - # Disable warnings from the library target - target_compile_options(spdlog PRIVATE -w) - # Disable warnings from included headers - get_target_property(propval spdlog INTERFACE_INCLUDE_DIRECTORIES) - target_include_directories(spdlog SYSTEM PUBLIC "${propval}") - endif() \ No newline at end of file diff --git a/cmake/tbb.cmake b/cmake/tbb.cmake index 3f6136d..2be02c6 100644 --- a/cmake/tbb.cmake +++ b/cmake/tbb.cmake @@ -1,15 +1,24 @@ include(FetchContent) -message(STATUS "Setting up tbb via CMake") +message(STATUS "Setting up tbb") +set(TBB_VERSION 2021.12.0) -#Fetches the version v2021.12.0 from the official github of tbb -FetchContent_Declare(tbb - GIT_REPOSITORY https://github.com/oneapi-src/oneTBB.git - GIT_TAG v2021.12.0 -) +find_package(TBB QUIET HINTS /opt/homebrew/Cellar/tbb) -# Disable tests & and do not treat tbb-compile errors as warnings -option(TBB_TEST "Enable testing" OFF) -option(TBB_STRICT "Treat compiler warnings as errors" OFF) +if(${TBB_FOUND}) + message(STATUS "Found existing TBB library: ${TBB_DIR}") +else() + message(STATUS "Using TBB from GitHub Release ${TBB_VERSION}") -FetchContent_MakeAvailable(tbb) + #Fetches the version v2021.12.0 from the official github of tbb + FetchContent_Declare(tbb + GIT_REPOSITORY https://github.com/oneapi-src/oneTBB.git + GIT_TAG v${TBB_VERSION} + ) + + # Disable tests & and do not treat tbb-compile errors as warnings + set(TBB_TEST OFF CACHE BOOL "" FORCE) + set(TBB_STRICT OFF CACHE BOOl "" FORCE) + + FetchContent_MakeAvailable(tbb) +endif() diff --git a/cmake/thrust.cmake b/cmake/thrust.cmake index a7ef8bf..0511df7 100644 --- a/cmake/thrust.cmake +++ b/cmake/thrust.cmake @@ -1,28 +1,26 @@ include(FetchContent) message(STATUS "Setting up thrust") +set(THRUST_VERSION 1.16.0) # Set custom variables, policies, etc. # Disable stuff not needed set(THRUST_ENABLE_HEADER_TESTING "OFF") set(THRUST_ENABLE_TESTING "OFF") set(THRUST_ENABLE_EXAMPLES "OFF") - # Set standard CPP Dialect to 17 (default of thrust would be 14) set(THRUST_CPP_DIALECT 17) -find_package(Thrust 1.16.0 QUIET) - -if (${Thrust_FOUND}) +find_package(Thrust ${THRUST_VERSION} QUIET) - message(STATUS "Using existing thrust installation") +if (${Thrust_FOUND}) + message(STATUS "Found existing thrust installation: ${Thrust_DIR}") else() - message(STATUS "Using thrust from git repository") - # Fetches the version 1.16.0 of the official NVIDIA Thrust repository + message(STATUS "Using thrust from GitHub Release ${THRUST_VERSION}") FetchContent_Declare(thrust GIT_REPOSITORY https://github.com/NVIDIA/thrust.git - GIT_TAG 1.16.0 + GIT_TAG ${THRUST_VERSION} ) FetchContent_MakeAvailable(thrust) endif() diff --git a/cmake/xsimd.cmake b/cmake/xsimd.cmake index 7fa01a1..a0d4c9e 100644 --- a/cmake/xsimd.cmake +++ b/cmake/xsimd.cmake @@ -1,24 +1,18 @@ include(FetchContent) -message(STATUS "Setting up xsimd via CMake") +message(STATUS "Setting up xsimd Library") +set(XSIMD_VERSION 11.1.0) - -find_package(xsimd 11.1 QUIET) +find_package(xsimd ${XSIMD_VERSION} QUIET) if (${xsimd_FOUND}) - - message(STATUS "Using existing xsimd installation") - + message(STATUS "Found existing xsimd Library: ${xsimd_DIR}") else() - - message(STATUS "Using xsimd from git repository") - - #Fetches the version 11.1.0 from the official github of tbb + message(STATUS "Using xsimd Library from GitHub Release ${XSIMD_VERSION}") FetchContent_Declare(xsimd GIT_REPOSITORY https://github.com/xtensor-stack/xsimd.git - GIT_TAG 11.1.0 + GIT_TAG ${XSIMD_VERSION} ) FetchContent_MakeAvailable(xsimd) - endif() diff --git a/cmake/yaml.cmake b/cmake/yaml.cmake index 8896a98..aeece0a 100644 --- a/cmake/yaml.cmake +++ b/cmake/yaml.cmake @@ -1,32 +1,27 @@ include(FetchContent) message(STATUS "Setting up yaml-cpp") +set(YAML_CPP_VERSION 0.8.0) -find_package(yaml-cpp 0.8.0 QUIET) +find_package(yaml-cpp ${YAML_CPP_VERSION} QUIET) if (${yaml-cpp_FOUND}) - - message(STATUS "Using existing yaml-cpp installation") - + message(STATUS "Found existing yaml-cpp library: ${yaml-cpp_DIR}") else() - - #Fetches the version 0.8.0 for yaml-cpp + message(STATUS "Using yaml-cpp from GitHub Release ${YAML_CPP_VERSION}") FetchContent_Declare(yaml-cpp GIT_REPOSITORY https://github.com/jbeder/yaml-cpp.git - GIT_TAG 0.8.0 + GIT_TAG ${YAML_CPP_VERSION} ) - # Disable everything we don't need set(YAML_CPP_BUILD_TESTS OFF CACHE INTERNAL "") set(YAML_CPP_BUILD_CONTRIB OFF CACHE INTERNAL "") set(YAML_CPP_BUILD_TOOLS OFF CACHE INTERNAL "") - + set(YAML_CPP_FORMAT_SOURCE OFF CACHE INTERNAL "") FetchContent_MakeAvailable(yaml-cpp) - # Disable warnings from the library target target_compile_options(yaml-cpp PRIVATE -w) # Disable warnings from included headers get_target_property(propval yaml-cpp INTERFACE_INCLUDE_DIRECTORIES) target_include_directories(yaml-cpp SYSTEM PUBLIC "${propval}") - endif() From 5dec5ef433b0977d5fcbec4fd5eea1dd8f8924a9 Mon Sep 17 00:00:00 2001 From: Jonas Schuhmacher Date: Tue, 15 Oct 2024 13:56:10 +0200 Subject: [PATCH 2/4] update mircomamba CI action - fix the Python Version to 3.12 - add setuptools dependency - update the deprecated workflow --- .github/workflows/build-and-test.yml | 9 ++++++--- environment.yml | 2 ++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml index 3eb8a56..6a70bd2 100644 --- a/.github/workflows/build-and-test.yml +++ b/.github/workflows/build-and-test.yml @@ -46,14 +46,17 @@ jobs: - uses: ilammy/msvc-dev-cmd@v1 if: matrix.os == 'windows-latest' - name: Install Conda environment from environment.yml - uses: mamba-org/provision-with-micromamba@main + uses: mamba-org/setup-micromamba@v1 with: + micromamba-version: '1.5.6-0' environment-file: environment.yml cache-downloads: true - cache-env: true + cache-environment: true + init-shell: bash powershell - name: Install & Test polyhedral-gravity - shell: bash -l {0} run: | pip install . -vv --no-build-isolation pytest -n 3 + shell: bash -el {0} + diff --git a/environment.yml b/environment.yml index e8087c0..5f2ccde 100644 --- a/environment.yml +++ b/environment.yml @@ -2,6 +2,8 @@ name: polyhedral-gravity-env channels: - conda-forge dependencies: + - python==3.12 + - setuptools - numpy - pytest - pytest-xdist \ No newline at end of file From 55b1b06bff8848f37fdca7c84eb1639bb007062c Mon Sep 17 00:00:00 2001 From: Jonas Schuhmacher Date: Tue, 15 Oct 2024 16:41:43 +0200 Subject: [PATCH 3/4] remove the `USE_LOCAL_TBB` option, rely on CMake; fix issues with yaml-cpp linking (when not fetched) --- CMakeLists.txt | 8 ++------ README.md | 3 +-- docs/quickstart/installation.rst | 3 +-- setup.py | 2 -- src/CMakeLists.txt | 2 +- test/CMakeLists.txt | 5 +++-- 6 files changed, 8 insertions(+), 15 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index ff7c9be..787a3dd 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -14,10 +14,6 @@ set(POLYHEDRAL_GRAVITY_PARALLELIZATION "CPP" CACHE STRING "Host parallelization (CPP= Serial, OMP = OpenMP, TBB = Intel Threading Building Blocks") set_property(CACHE POLYHEDRAL_GRAVITY_PARALLELIZATION PROPERTY STRINGS CPP, OMP, TBB) -# Enforce to use an already installed tbb library instead of compiling from source -option(USE_LOCAL_TBB "Uses the local tbb installation rather than on using the automatically fetched version from -GitHub via CMake (Default: OFF)" OFF) - # Set the Logging Level set(LOGGING_LEVEL_LIST "TRACE" "DEBUG" "INFO" "WARN" "ERROR" "CRITICAL" "OFF") set(LOGGING_LEVEL "INFO" CACHE STRING "Set the Logging level, default (INFO), available options: TRACE, DEBUG, INFO, WARN, ERROR, CRITICAL, OFF") @@ -77,9 +73,9 @@ include(clang_format) # Get a version of tbb from the github repository, simplifies compilation for the user since tbb does not need to be # preinstalled but rather gets automatically set up via CMake # Nevertheless, there is still the option to enforce to use a local installation if one exists -if (NOT USE_LOCAL_TBB AND ${POLYHEDRAL_GRAVITY_PARALLELIZATION} STREQUAL "TBB") +if (${POLYHEDRAL_GRAVITY_PARALLELIZATION} STREQUAL "TBB") include(tbb) - thrust_set_TBB_target(tbb) + thrust_set_TBB_target(TBB::tbb) endif () # Thrust set-up i.e. the parallelization library, create targets according to the users specification diff --git a/README.md b/README.md index 217eec0..194bd1a 100644 --- a/README.md +++ b/README.md @@ -259,7 +259,7 @@ you have a C++17 capable compiler and CMake installed. The project uses the following dependencies, all of them are **automatically** set-up via CMake: -- GoogleTest (1.13.0 or compatible), only required for testing +- GoogleTest (1.15.2 or compatible), only required for testing - spdlog (1.13.0 or compatible), required for logging - tetgen (1.6 or compatible), required for I/O - yaml-cpp (0.8.0 or compatible), required for I/O @@ -309,7 +309,6 @@ The following options are available: |-------------------------------------------:|:--------------------------------------------------------------------------------------------| | POLYHEDRAL_GRAVITY_PARALLELIZATION (`CPP`) | `CPP` = Serial Execution / `OMP` or `TBB` = Parallel Execution with OpenMP or Intel\'s TBB | | LOGGING_LEVEL (`INFO`) | `TRACE`, `DEBUG`, `INFO`, `WARN`, `ERROR`, `CRITICAL`, `OFF` | -| USE_LOCAL_TBB (`OFF`) | Use a local installation of `TBB` instead of setting it up via `CMake` | | BUILD_POLYHEDRAL_GRAVITY_DOCS (`OFF`) | Build this documentation | | BUILD_POLYHEDRAL_GRAVITY_TESTS (`ON`) | Build the Tests | | BUILD_POLYHEDRAL_PYTHON_INTERFACE (`ON`) | Build the Python interface | diff --git a/docs/quickstart/installation.rst b/docs/quickstart/installation.rst index 3ed7545..f529a47 100644 --- a/docs/quickstart/installation.rst +++ b/docs/quickstart/installation.rst @@ -92,7 +92,6 @@ Name (Default) Options ================================================ =================================================================================================================================== POLYHEDRAL_GRAVITY_PARALLELIZATION (:code:`CPP`) :code:`CPP` = Serial Execution / :code:`OMP` or :code:`TBB` = Parallel Execution with OpenMP or Intel's TBB LOGGING_LEVEL (:code:`INFO`) :code:`TRACE`, :code:`DEBUG`, :code:`INFO`, :code:`WARN`, :code:`ERROR`, :code:`CRITICAL`, :code:`OFF` -USE_LOCAL_TBB (:code:`OFF`) Use a local installation of :code:`TBB` instead of setting it up via :code:`CMake` BUILD_POLYHEDRAL_GRAVITY_DOCS (:code:`OFF`) Build this documentation BUILD_POLYHEDRAL_GRAVITY_TESTS (:code:`ON`) Build the Tests BUILD_POLYHEDRAL_PYTHON_INTERFACE (:code:`ON`) Build the Python interface @@ -104,7 +103,7 @@ Dependencies (automatically set-up) Dependencies (all of them are automatically set-up via :code:`CMake`): -- GoogleTest (1.13.0 or compatible), only required for testing +- GoogleTest (1.15.2 or compatible), only required for testing - spdlog (1.13.0 or compatible), required for logging - tetgen (1.6 or compatible), required for I/O - yaml-cpp (0.8.0 or compatible), required for I/O diff --git a/setup.py b/setup.py index 5123228..26cd85f 100644 --- a/setup.py +++ b/setup.py @@ -19,8 +19,6 @@ "POLYHEDRAL_GRAVITY_PARALLELIZATION": "TBB", # Default value (INFO=2) "LOGGING_LEVEL": "INFO", - # Default value (OFF) - "USE_LOCAL_TBB": "OFF", # Not required for the python interface (--> OFF) "BUILD_POLYHEDRAL_GRAVITY_DOCS": "OFF", # Not required for the python interface (--> OFF) diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 5875530..08b5bbb 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -17,7 +17,7 @@ if (BUILD_POLYHEDRAL_GRAVITY_LIBRARY) # Link libraries needed in all targets target_link_libraries(${PROJECT_NAME}_lib spdlog::spdlog - yaml-cpp + yaml-cpp::yaml-cpp tetgen xsimd Thrust diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 1ea8876..4559a0b 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -12,8 +12,9 @@ add_executable(${PROJECT_NAME}_test ${TEST_SRC}) # Links target breakupModelTest against gtest_main (entry point, so no own main method is required) target_link_libraries(${PROJECT_NAME}_test - gtest_main - gmock + GTest::gtest_main + GTest::gtest + GTest::gmock ${PROJECT_NAME}_lib ) From f6088c237463c470f637129611550bac2d7f3f4a Mon Sep 17 00:00:00 2001 From: Jonas Schuhmacher Date: Tue, 15 Oct 2024 16:52:33 +0200 Subject: [PATCH 4/4] mention clang-format in CONTRIBUTING.md --- CONTRIBUTING.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a665d73..0ffbbb7 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -109,7 +109,10 @@ Have you found an issue you would like to work on? Or do you have a great idea f - Ensure that you work on a new branch for your contribution. The name does not need to follow a specific pattern, but it should be descriptive (e.g. `fix-issue-123` or `fancy-feature-xyz`). - Make sure that you are using the latest version from `main`. Instructions on how to set up the project can be found in the [documentation](https://esa.github.io/polyhedral-gravity-model/) or in the [README](README.md). -- Please try to follow the existing code style and conventions. +- Please try to follow the existing code style and conventions. You can apply our clang-format via CMake by executing in the build-folder: + ```bash + cmake --build . --target format + ``` - Please try to follow the best practices and guidelines for your code's quality and documentation. This includes writing tests for your code. We do not enforce a specific coverage, but we expect reasonable tests for new code. - Please try to be short and concise with your commit messages. In case you want to provide more information, you can use the commit description.