Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses Python symbol drop issues on Linux by refining the linker flag strategy for CUDA libraries. The aggressive Linux linker was removing symbols from the kintera_cuda_release library, and this fix applies --no-as-needed specifically around the CUDA library to prevent this.
Changes:
- Removed obsolete commented-out library reordering code
- Added conditional logic to wrap kintera_cuda_release library with
--no-as-neededlinker flags on Linux - Removed global
--no-as-neededflags in favor of targeted application only to the CUDA library
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "-Wl,-rpath,$ORIGIN/../pyharp/lib", | ||
| "-Wl,--as-needed", | ||
| ] | ||
| "-Wl,-rpath,$ORIGIN/../pyharp/lib" |
There was a problem hiding this comment.
Missing comma at the end of line 74. This will cause a syntax error when the list is parsed. The last element in the list should have a trailing comma to match the style of other elements.
| "-Wl,-rpath,$ORIGIN/../pyharp/lib" | |
| "-Wl,-rpath,$ORIGIN/../pyharp/lib", |
setup.py
Outdated
| if 'kintera_cuda_release' in libraries: | ||
| libraries.remove('kintera_cuda_release') | ||
| cuda_linker = ["-Wl,--no-as-needed", | ||
| "-lkintera_cuda_release", | ||
| "-Wl,--as-needed"] |
There was a problem hiding this comment.
Potential issue: If the build system creates libraries with different names (e.g., 'kintera_cuda_debug' instead of 'kintera_cuda_release'), this code will not apply the --no-as-needed workaround for those libraries. Consider checking for any library name containing both 'kintera' and 'cuda', or document why only 'kintera_cuda_release' needs this treatment.
| if 'kintera_cuda_release' in libraries: | |
| libraries.remove('kintera_cuda_release') | |
| cuda_linker = ["-Wl,--no-as-needed", | |
| "-lkintera_cuda_release", | |
| "-Wl,--as-needed"] | |
| cuda_libraries = [lib for lib in libraries if lib.startswith("kintera") and "cuda" in lib] | |
| if cuda_libraries: | |
| for lib in cuda_libraries: | |
| libraries.remove(lib) | |
| cuda_linker = ( | |
| ["-Wl,--no-as-needed"] | |
| + [f"-l{lib}" for lib in cuda_libraries] | |
| + ["-Wl,--as-needed"] | |
| ) |
|
🎉 Released v1.3.0! What's Changed
Full Changelog: v1.2.10...v1.3.0 |
No description provided.