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

Update cmake #275

Open
wants to merge 22 commits into
base: dev
Choose a base branch
from
Open

Update cmake #275

wants to merge 22 commits into from

Conversation

chillenzer
Copy link
Contributor

@chillenzer chillenzer commented Jan 15, 2025

This is a major reorganisation of the repository including setting up proper CMake support. It is strongly inspired by ModernCppStarter with some notable changes resulting from discussion with and preferences of @psychocoderHPC. As kind of a byproduct, also includes some modern features that are however not yet tweaked to production quality.

This PR is tested with KitGenBench and picongpu. Hint for the review: The first commit removes alpaka and is quite unwieldy. Viewing the diff from the second commit on shows that it's really just cmake stuff and files moved.

I'll try to summarise the new layout and features:

  • The main directory now provides a CMakeLists.txt that provides the library and optionally includes tests and examples via mallocMC_BUILD_TESTING/EXAMPLES.
  • Directories containing source code are include, examples and test. The latter two are self-contained cmake subdirectories that can be built independently.
  • All external source code is removed from the repository (except for a patched version of CPM).
  • CPM is used for adding dependencies. This means that given internet access the library is fully self-contained and can download and build all dependencies itself if it must.
  • Fine-grained control over the dependencies' origin is provided by variables of the form mallocMC_USE_... allowing ON (no downloading), ON_ALLOW_FETCH (default if required), ON_ALWAYS_FETCH (for best reproducibility), AUTO (default for optional dependencies) and OFF. Alternatively, the variable is allowed to contain a path pointing to the source location for this dependency.
  • All details about the dependencies are specified in cmake/package-lock.cmake. Only ever touch this, if you want to change a version or the like. (Updating alpaka should be significantly less painful now.)
  • Without internet connection one can simply download dependencies on another computer specifying the -DCPM_SOURCE_CACHE=/some/convenient/path, copy that to the offline system and point the same variable there for the build -DCPM_SOURCE_CACHE=/where/you/copied/the/source/cache. This is probably the closest we can get to offline support without shipping external code.
  • In copying over from ModernCppStarter, we acquired a number of tools including platform-independent sanitizer, static analysis and code coverage support. At the time of writing, this is not yet properly configured and would need some tinkering with to produce useful output. (Currently, it can be run but most warnings I've seen are about alpaka and Catch2. It's hard to say at this point if there are useful messages about mallocMC among the noise.)

@chillenzer chillenzer force-pushed the update-cmake branch 6 times, most recently from f677e22 to d09d548 Compare January 16, 2025 07:45
@chillenzer chillenzer marked this pull request as ready for review January 16, 2025 07:51
@psychocoderHPC psychocoderHPC added this to the 3.0.0 milestone Jan 17, 2025
Copy link
Member

@psychocoderHPC psychocoderHPC left a comment

Choose a reason for hiding this comment

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

mostly variables without prefix mallocMC_ were exposed.

@@ -1,5 +1,8 @@
name: Continuous Integration
on: [push, pull_request]
env:
CPM_SOURCE_CACHE: ${{ github.workspace }}/cpm_modules
Copy link
Member

Choose a reason for hiding this comment

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

out of interest: Is this variable also required in normal workflows or will the cache by default places somewhere relative to the build folder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Offline discussion: This is not required but recommended. If not set, CPM will redownload for every new build directory. The downloaded files will be put into a local folder inside the build directory (if I recall correctly). The discussion led to another comment: We should document the option to not cache unstable dependencies (like the alpaka dev branch).

@@ -18,26 +21,25 @@ jobs:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: actions/cache@v3
Copy link
Member

Choose a reason for hiding this comment

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

why is all this configuration required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not required but supposed to slightly speed up CI runs by caching downloads. The CI here is only provisional at this point anyways. But concerning caching we should document the option to explicitly disable caching for unstable dependencies.

NAMESPACE ${PROJECT_NAME}
BINARY_DIR ${PROJECT_BINARY_DIR}
INCLUDE_DIR ${PROJECT_SOURCE_DIR}/include
INCLUDE_DESTINATION include/${PROJECT_NAME}-${PROJECT_VERSION}
Copy link
Member

Choose a reason for hiding this comment

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

after installing, are all files in include/malloc-3.0.0/mallocMC/*?


# enables sanitizers support using the the `USE_SANITIZER` flag available values are: Address,
# Memory, MemoryWithOrigins, Undefined, Thread, Leak, 'Address;Undefined'
if(USE_SANITIZER OR USE_STATIC_ANALYZER)
Copy link
Member

Choose a reason for hiding this comment

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

the variables should have the prefix mallocMC_

endif()

# enables CCACHE support through the USE_CCACHE flag possible values are: YES, NO or equivalent
if(USE_CCACHE)
Copy link
Member

Choose a reason for hiding this comment

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

should be mallocMC_USE_CCACHE


# --- Import tools ----

include(../../cmake/tools.cmake)
Copy link
Member

Choose a reason for hiding this comment

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

should all ../.. start with ${CMAKE_CURRENT_LIST_DIR}/../..?


# --- Import tools ----

include(../../cmake/tools.cmake)
Copy link
Member

Choose a reason for hiding this comment

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

maybe ${CMAKE_CURRENT_LIST_DIR}/../.. ?


# ---- Options ----

option(ENABLE_TEST_COVERAGE "Enable test coverage" OFF)
Copy link
Member

Choose a reason for hiding this comment

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

Those variables should start with mallocMC_ to clearly show the users that mallocMC behaviour is changed.
This will also group all variables of mallocMC which helps finding all possible cmake switches.

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

Successfully merging this pull request may close these issues.

2 participants