feat: move Pythonizations implementations to source files#908
feat: move Pythonizations implementations to source files#908tmadlener merged 4 commits intoAIDASoft:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the Pythonizations implementation by moving function definitions from inline functions in the header file to a separate source file. This change enables the headers to be included in C++20 modules while maintaining the same functionality.
- Function implementations moved from
static inlinein header to regular definitions in source file - Added
target_link_libraries(podio PRIVATE Python3::Python)to CMakeLists.txt - Header file now contains only forward declarations
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/Pythonizations.cc |
New source file containing implementations of subscript and pythonize_subscript functions |
include/podio/detail/Pythonizations.h |
Refactored to contain only function declarations, removed inline implementations and <stdexcept> include |
src/CMakeLists.txt |
Added Pythonizations.cc to core_sources and linked Python3 library |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
|
||
| PODIO_ADD_LIB_AND_DICT(podio "${core_headers}" "${core_sources}" selection.xml) | ||
| target_compile_options(podio PRIVATE -pthread) | ||
| target_link_libraries(podio PRIVATE Python3::Python) |
There was a problem hiding this comment.
Ah, this probably should have been in the Python:: namespace since that's what is added as a dependency in the config
podio/cmake/podioConfig.cmake.in
Lines 28 to 32 in 5d82748
and in the
PODIO_GENERATE_DATAMODEL module:Line 158 in 5d82748
There was a problem hiding this comment.
Then again, the package that is found is Python3...
Lines 197 to 200 in 5d82748
|
I've investigated this a bit after the Mac build started failing in spack for me. The way I see it, this now compiles in a strict dependency on the exact Python runtime lib into the podio library, which means that you need to actually link against python in a way you usually do for embedding the interpreter. This then causes issues in Spack, I believe because the python build from the package there does not correctly propagate it's own build configuration on macOS, and relies on |
|
Looks like moving these into a dedicated TU and building them into edit: Just read in #907 that Some additional information of what we have learned in the meantime about the behavior with and without these changes.
|
Certainly the use case of modules is tangential here. If this needs to be reverted, we can figure out how to make it work with modules given the additional constraint of header-only |
This PR moves the implementation of Pythonizations detail functions into a source file. This allows the headers to be included in C++20 modules (#907).
BEGINRELEASENOTES
ENDRELEASENOTES