Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes Ubuntu symbol drop issues by modifying linker behavior and library ordering. The changes aim to prevent the linker from dropping symbols from libraries that may appear unused but are actually needed at runtime.
Changes:
- Modified library ordering in setup.py to place CUDA libraries last (as providers) and non-CUDA libraries first (as consumers)
- Added
--no-as-neededlinker flags to Linux builds in both Python setup.py and CMake macros - Updated three CMake macros (test, problem, and CUDA test) to include the linker flag for Linux systems
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| setup.py | Reordered library linking to place CUDA libraries last; added --no-as-needed/--as-needed linker flags for Linux builds (old code left commented) |
| cmake/macros/macro_setup_test.cmake | Added --no-as-needed linker flag for Linux platforms to prevent symbol dropping in test builds |
| cmake/macros/macro_setup_problem.cmake | Added --no-as-needed linker flag for Linux platforms to prevent symbol dropping in problem builds |
| cmake/macros/macro_setup_cuda_test.cmake | Added --no-as-needed linker flag for Linux platforms to prevent symbol dropping in CUDA test builds |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #current = [item for item in library_names if item.startswith('snap')] | ||
| #other = [item for item in library_names if not item.startswith('snap')] |
There was a problem hiding this comment.
The commented-out code should be removed rather than kept as comments. Leaving old code commented out reduces code maintainability and can cause confusion for future developers.
| #current = [item for item in library_names if item.startswith('snap')] | |
| #other = [item for item in library_names if not item.startswith('snap')] |
| "-Wl,--no-as-needed", | ||
| "-Wl,-rpath,$ORIGIN/lib", | ||
| "-Wl,-rpath,$ORIGIN/../torch/lib", | ||
| "-Wl,-rpath,$ORIGIN/../pydisort/lib", | ||
| "-Wl,-rpath,$ORIGIN/../pyharp/lib", | ||
| "-Wl,-rpath,$ORIGIN/../kintera/lib", | ||
| "-Wl,--as-needed", |
There was a problem hiding this comment.
The --no-as-needed and --as-needed linker flags may not work as intended when placed in extra_link_args. The effectiveness of these flags depends on their position relative to the library link commands. In many build systems, extra_link_args are processed either before or after the libraries specified in the 'libraries' parameter, which could make these flags ineffective. The library ordering changes (lines 27-33) may be sufficient to fix the symbol drop issue without these flags. Consider testing whether these linker flags are actually necessary, or if the library reordering alone resolves the Ubuntu symbol drop problem.
| "-Wl,--no-as-needed", | |
| "-Wl,-rpath,$ORIGIN/lib", | |
| "-Wl,-rpath,$ORIGIN/../torch/lib", | |
| "-Wl,-rpath,$ORIGIN/../pydisort/lib", | |
| "-Wl,-rpath,$ORIGIN/../pyharp/lib", | |
| "-Wl,-rpath,$ORIGIN/../kintera/lib", | |
| "-Wl,--as-needed", | |
| "-Wl,-rpath,$ORIGIN/lib", | |
| "-Wl,-rpath,$ORIGIN/../torch/lib", | |
| "-Wl,-rpath,$ORIGIN/../pydisort/lib", | |
| "-Wl,-rpath,$ORIGIN/../pyharp/lib", | |
| "-Wl,-rpath,$ORIGIN/../kintera/lib", |
|
🎉 Released v1.2.7! What's Changed
Full Changelog: v1.2.6...v1.2.7 |
No description provided.