Skip to content

Conversation

sebastiankreutzer
Copy link
Member

This patch cleans up the CMake options related to CGPatch.
Changes:

  • Move options from main CMakeLists.txt into tools or cgpatch subdirectory
  • Remove project() declaration in CGPatch CMakeLIsts.txt

(I closed the previous PR because I accidentally pushed the branch to this repo instead of my fork.)

@sebastiankreutzer sebastiankreutzer self-assigned this Aug 21, 2025
@sebastiankreutzer sebastiankreutzer changed the title Feat/refactor cgpatch cmake [NFC] Refactor CGPatch CMake Aug 21, 2025
Copy link
Member

@pearzt pearzt left a comment

Choose a reason for hiding this comment

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

So far, we have defined (almost) all CMake options in the top-level CMakeLists.txt (although there are a few exceptions tbf). Is this something we want to keep up? No strong opinion from my side, but I think if there is no central file where we define options, I guess we should add an overview table in a README. Not something that should stop us from merging this as soon as the CI is fixed, though.

@sebastiankreutzer
Copy link
Member Author

So far, we have defined (almost) all CMake options in the top-level CMakeLists.txt (although there are a few exceptions tbf). Is this something we want to keep up? No strong opinion from my side, but I think if there is no central file where we define options, I guess we should add an overview table in a README. Not something that should stop us from merging this as soon as the CI is fixed, though.

That's something we can maybe discuss in the next meeting. For now, I'm changing this to a draft because the CI failures are confusing me 😅

@sebastiankreutzer sebastiankreutzer marked this pull request as draft August 22, 2025 10:55
@silas-martens
Copy link
Contributor

So far, we have defined (almost) all CMake options in the top-level CMakeLists.txt (although there are a few exceptions tbf). Is this something we want to keep up? No strong opinion from my side, but I think if there is no central file where we define options, I guess we should add an overview table in a README. Not something that should stop us from merging this as soon as the CI is fixed, though.

That's something we can maybe discuss in the next meeting. For now, I'm changing this to a draft because the CI failures are confusing me 😅

I think the reason integration tests fail is that when CGPATCH_USE_MPI is enabled the runtime depends on mpi. Therefore all test cases that include the runtime need to be compiled with mpicxx. However in the integration tests only the mpi testcases are compiled using mpicxx, while the "regular" testcases in input/general are compiled using clang++ leading to the error in the CI.

A simple fix would be to have a global variable COMPILER in the integration tests that is set to clang++ or mpicxx wether CGPATCH_USE_MPI (already present in the runner) is enabled or not.
I’m a bit surprised that this hasn’t come up earlier.

@sebastiankreutzer
Copy link
Member Author

So far, we have defined (almost) all CMake options in the top-level CMakeLists.txt (although there are a few exceptions tbf). Is this something we want to keep up? No strong opinion from my side, but I think if there is no central file where we define options, I guess we should add an overview table in a README. Not something that should stop us from merging this as soon as the CI is fixed, though.

That's something we can maybe discuss in the next meeting. For now, I'm changing this to a draft because the CI failures are confusing me 😅

I think the reason integration tests fail is that when CGPATCH_USE_MPI is enabled the runtime depends on mpi. Therefore all test cases that include the runtime need to be compiled with mpicxx. However in the integration tests only the mpi testcases are compiled using mpicxx, while the "regular" testcases in input/general are compiled using clang++ leading to the error in the CI.

A simple fix would be to have a global variable COMPILER in the integration tests that is set to clang++ or mpicxx wether CGPATCH_USE_MPI (already present in the runner) is enabled or not. I’m a bit surprised that this hasn’t come up earlier.

Thanks for the investigation, that sounds like it could be the issue

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.

3 participants