Skip to content

Conversation

@wirew0rm
Copy link

@wirew0rm wirew0rm commented Jul 4, 2025

Hey and thanks for maintaining this nice little library! I am having some issues with including pffft via cmake's fetchContent.

This PR adds options to control whether tests/benchmarks/examples should be built and whether unittests should be registered with cmakes ctest runner. The options initialized based on whether pffft is the current top level cmake build directory.

This helps with including pffft via FetchContent with EXCLUDE_FROM_ALL. Because cmake will only build the targets that are actually required by the consuming project, if all tests are unconditionally registered with ctest, the testsuite of the consuming project will fail. This assumes that most library consumers are not interested in running the testsuites of all their dependencies in their own build.

It also moves the compile definitions for float/double into the PFFFT target, so it only has to be defined once and downstream consumers can just include the target.

This allows to use pffft with the following cmake snippet:

include(FetchContent)
set(PFFFT_USE_SIMD OFF)
FetchContent_Declare(
  pffft
  GIT_REPOSITORY https://github.com/fair-acc/pffft.git
  GIT_TAG cmakeTestFlag # feature branch which adds better compatibility with CMake's add_subdirectory/FetchContent
  EXCLUDE_FROM_ALL
)
FetchContent_MakeAvailable(pffft)

add_executable(myApp main.cpp)
target_link_libraries(myApp INTERFACE PFFFT )

I had hoped to be able to enable this with less changes to pffft's cmake and am open to improving on this, but wanted to first get some general feedback on whether you would accept such a change and if there are other usecases that such changes could break that I should be aware of. Also there's always the tradeoff between the minimal amount of changes and making the result most complete/consistent.

The minimal change would be to just add the option to skip the add_test calls.

Adds an option to control whether unittests should be registered with
cmakes ctest runner. It is initialized based on whether pffft is the
current top level cmake build directory.

This helps with including pffft via FetchContent with EXCLUDE_FROM_ALL.
Because cmake will only build the targets that are actually required by
the consuming project, if all tests are unconditionally registered with
ctest, the testsuite of the consuming project will fail. This assumes
that most library consumers are not interested in running the testsuites
of all their dependencies in their own build.

Also move the definition of the compile definition into the PFFFT
target, so it only has to be defined once and downstream consumers can
just include the target.
@wirew0rm wirew0rm changed the title CMake: guard unit-tests CMake: guard unit-tests to allow use with fetchContent and addSubdirectory Jul 4, 2025
Copy link
Owner

@marton78 marton78 left a comment

Choose a reason for hiding this comment

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

Hi, thanks a lot for your PR! I'm happy to merge this, but before: please fix the typos I pointed out and test locally to ensure that tests etc. are running in the stand-alone case.

@wirew0rm
Copy link
Author

wirew0rm commented Jul 7, 2025

Thanks, for having a first look, as written above, it's clear that this is not yet in a finished state, this was more to first see if this is something that is generally welcomed.

I fixed the typos and noticed that they were effectively masking a lot of the build and I noticed that a lot of the testing code also relies on the PFFFT_ENABLE_FLOAT/DOUBLE, so i'll have to rethink some of the changes to come back with a version also works for most general cases.

No need to approve the CI, I have to do some restructuring and testing first.

Signed-off-by: Alexander Krimm <A.Krimm@gsi.de>
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.

2 participants