fix: macos dynamic link libpaddle#2
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes macOS compatibility issues with Paddle library linking by adding platform-specific dynamic library resolution logic. The changes add build-time validation to ensure required Paddle libraries exist before attempting to link them.
Changes:
- Introduced
resolve_paddle_library()function to validate library paths at CMake configuration time - Added platform-specific library path detection for macOS, Windows, and Linux
- Restricted Linux-specific
--as-neededlinker flag to prevent macOS linker errors
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| set(PADDLE_LIBPHI_CORE_CANDIDATES | ||
| "${PADDLE_PATH}/libs/libphi_core.dll" | ||
| "${PADDLE_PATH}/libs/phi_core.dll" | ||
| ) |
There was a problem hiding this comment.
On Windows, the library candidates should list .lib files before .dll files. Import libraries (.lib) are typically used for linking, while dynamic libraries (.dll) are loaded at runtime. If both files exist, the current order will select the .dll first, which may not work correctly with CMake's target_link_libraries. Consider reordering to check for .lib files first.
| if(APPLE) | ||
| # Paddle on macOS ships libpaddle as .so, while runtime deps are .dylib. | ||
| set(PADDLE_LIBPADDLE_CANDIDATES "${PADDLE_PATH}/base/libpaddle.so") | ||
| set(PADDLE_LIBCOMMON_CANDIDATES "${PADDLE_PATH}/libs/libcommon.dylib") | ||
| set(PADDLE_LIBPHI_CORE_CANDIDATES "${PADDLE_PATH}/libs/libphi_core.dylib") | ||
| elseif(WIN32) | ||
| set(PADDLE_LIBPADDLE_CANDIDATES | ||
| "${PADDLE_PATH}/base/libpaddle.dll" | ||
| "${PADDLE_PATH}/base/libpaddle.lib" | ||
| ) | ||
| set(PADDLE_LIBCOMMON_CANDIDATES | ||
| "${PADDLE_PATH}/libs/libcommon.dll" | ||
| "${PADDLE_PATH}/libs/common.dll" | ||
| ) | ||
| set(PADDLE_LIBPHI_CORE_CANDIDATES | ||
| "${PADDLE_PATH}/libs/libphi_core.dll" | ||
| "${PADDLE_PATH}/libs/phi_core.dll" | ||
| ) | ||
| else() | ||
| set(PADDLE_LIBPADDLE_CANDIDATES "${PADDLE_PATH}/base/libpaddle.so") | ||
| set(PADDLE_LIBCOMMON_CANDIDATES "${PADDLE_PATH}/libs/libcommon.so") | ||
| set(PADDLE_LIBPHI_CORE_CANDIDATES "${PADDLE_PATH}/libs/libphi_core.so") | ||
| endif() |
There was a problem hiding this comment.
The code should validate that PADDLE_PATH is defined and non-empty before using it to construct library paths. If PADDLE_PATH is not set, the error message from resolve_paddle_library will be confusing (e.g., "Unable to locate Paddle library for PADDLE_LIBPADDLE. Tried: /base/libpaddle.so"). Consider adding a check like if(NOT DEFINED PADDLE_PATH OR PADDLE_PATH STREQUAL "") with a clear error message before this platform-specific code block.
| return() | ||
| endif() | ||
| endforeach() | ||
| message(FATAL_ERROR "Unable to locate Paddle library for ${out_var}. Tried: ${ARGN}") |
There was a problem hiding this comment.
The error message could be more user-friendly. Currently it says "Unable to locate Paddle library for PADDLE_LIBPADDLE" which uses the variable name. Consider changing the message to be more descriptive, for example: "Unable to locate Paddle library '${out_var}'. Searched in: ${ARGN}" or maintain a mapping from variable names to descriptive names like "libpaddle", "libcommon", etc.
| message(FATAL_ERROR "Unable to locate Paddle library for ${out_var}. Tried: ${ARGN}") | |
| message(FATAL_ERROR "Unable to locate Paddle library '${out_var}'. Searched in: ${ARGN}") |
| "${PADDLE_PATH}/base/libpaddle.dll" | ||
| "${PADDLE_PATH}/base/libpaddle.lib" |
There was a problem hiding this comment.
On Windows, the library candidates should list .lib files before .dll files. Import libraries (.lib) are typically used for linking, while dynamic libraries (.dll) are loaded at runtime. If both files exist, the current order will select the .dll first, which may not work correctly with CMake's target_link_libraries. Consider reordering to check for .lib files first.
| "${PADDLE_PATH}/base/libpaddle.dll" | |
| "${PADDLE_PATH}/base/libpaddle.lib" | |
| "${PADDLE_PATH}/base/libpaddle.lib" | |
| "${PADDLE_PATH}/base/libpaddle.dll" |
| set(PADDLE_LIBCOMMON_CANDIDATES | ||
| "${PADDLE_PATH}/libs/libcommon.dll" | ||
| "${PADDLE_PATH}/libs/common.dll" | ||
| ) |
There was a problem hiding this comment.
On Windows, the library candidates should list .lib files before .dll files. Import libraries (.lib) are typically used for linking, while dynamic libraries (.dll) are loaded at runtime. If both files exist, the current order will select the .dll first, which may not work correctly with CMake's target_link_libraries. Consider reordering to check for .lib files first.
|
@ShigureNyako 帮我看看这么改动态链接有没有什么问题 |
fix