Conversation
There was a problem hiding this comment.
Pull request overview
This pull request addresses symbol dropping issues on Linux systems by adjusting library linking behavior and ordering.
Changes:
- Reordered library linking in setup.py to place CUDA libraries last, ensuring proper dependency resolution
- Replaced conditional CUDAExtension/CppExtension logic with a single CppExtension since the Python binding code is pure C++
- Added
--no-as-neededlinker flag to test executables on Linux to prevent aggressive symbol dropping
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| setup.py | Modified library ordering to prioritize non-CUDA kintera libraries before CUDA libraries, and switched from conditional CUDA/non-CUDA extension types to always using CppExtension |
| cmake/macros/macro_setup_test.cmake | Added --no-as-needed linker flag for Linux (non-macOS Unix systems) to prevent linker from dropping libraries |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (UNIX AND NOT APPLE) | ||
| target_link_options(${namel}.${buildl} PRIVATE -Wl,--no-as-needed) | ||
| endif() |
There was a problem hiding this comment.
The --no-as-needed linker flag is being added to setup_test to prevent Linux from aggressively dropping symbols. However, macro_setup_problem.cmake has a similar structure and links the same libraries (kintera::kintera and optionally kintera::kintera_cu) but does not have this flag. For consistency and to prevent similar symbol dropping issues in problem executables, consider adding the same linker flag to setup_problem as well.
| ext_module = cpp_extension.CppExtension( | ||
| name='kintera.kintera', | ||
| sources=glob.glob('python/csrc/*.cpp'), | ||
| include_dirs=include_dirs, | ||
| library_dirs=lib_dirs, | ||
| libraries=libraries, | ||
| extra_compile_args=['-Wno-attributes'], | ||
| extra_link_args=extra_link_args, |
There was a problem hiding this comment.
The --no-as-needed linker flag is being added to test executables in CMake to prevent Linux from aggressively dropping symbols. However, the Python extension in setup.py also links to the same kintera libraries and may suffer from the same symbol dropping issue on Linux. Consider adding -Wl,--no-as-needed to extra_link_args for non-Darwin platforms (lines 64-69) to ensure consistent behavior and prevent potential symbol resolution issues in the Python extension.
| # 1) non-cuda libs first (consumers) | ||
| kintera_non_cuda = [l for l in library_names if l.startswith("kintera") and "cuda" not in l] | ||
| # 2) cuda libs last (providers) |
There was a problem hiding this comment.
The comment on line 26 describes kintera non-CUDA libraries as "consumers," but this is potentially misleading. In typical linking scenarios, consumers depend on providers, not the other way around. If kintera_cuda libraries are "providers" (line 28), then kintera non-CUDA libraries would be consumers of those CUDA providers. However, placing providers (kintera_cuda) last in the link order is correct for ensuring that dependencies are resolved properly. Consider clarifying these comments to explain that this ordering ensures proper symbol resolution where libraries that depend on CUDA functionality come before the CUDA provider libraries.
| # 1) non-cuda libs first (consumers) | |
| kintera_non_cuda = [l for l in library_names if l.startswith("kintera") and "cuda" not in l] | |
| # 2) cuda libs last (providers) | |
| # 1) non-CUDA kintera libs first (they may depend on CUDA symbols) | |
| kintera_non_cuda = [l for l in library_names if l.startswith("kintera") and "cuda" not in l] | |
| # 2) CUDA kintera libs last (they provide the CUDA symbols for earlier deps) |
|
🎉 Released v1.2.10! What's Changed
Full Changelog: v1.2.9...v1.2.10 |
1 participant
No description provided.