Skip to content

fix: macos dynamic link libpaddle #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gouzil wants to merge 2 commits into
base: paddle
Choose a base branch
from
+57 −4
Open

fix: macos dynamic link libpaddle #2

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ed17499
fix: macos dynamic link
gouzil Feb 24, 2026
d249d96
fix macos dynamic link error
gouzil Feb 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 57 additions & 4 deletions src/torchcodec/_core/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,50 @@ find_package(pybind11 REQUIRED)
#find_package(Torch REQUIRED)
set(TORCH_CXX_FLAGS "")

function(resolve_paddle_library out_var)
foreach(candidate IN LISTS ARGN)
if(EXISTS "${candidate}")
set(${out_var} "${candidate}" PARENT_SCOPE)
return()
endif()
endforeach()
message(FATAL_ERROR "Unable to locate Paddle library for ${out_var}. Tried: ${ARGN}")
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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}")

Copilot uses AI. Check for mistakes.
endfunction()

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"
Comment on lines +28 to +29
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
"${PADDLE_PATH}/base/libpaddle.dll"
"${PADDLE_PATH}/base/libpaddle.lib"
"${PADDLE_PATH}/base/libpaddle.lib"
"${PADDLE_PATH}/base/libpaddle.dll"

Copilot uses AI. Check for mistakes.
)
set(PADDLE_LIBCOMMON_CANDIDATES
"${PADDLE_PATH}/libs/libcommon.dll"
"${PADDLE_PATH}/libs/common.dll"
)
Comment on lines +31 to +34
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
set(PADDLE_LIBPHI_CORE_CANDIDATES
"${PADDLE_PATH}/libs/libphi_core.dll"
"${PADDLE_PATH}/libs/phi_core.dll"
)
Comment on lines +35 to +38
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
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()
Comment on lines +21 to +43
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

resolve_paddle_library(PADDLE_LIBPADDLE ${PADDLE_LIBPADDLE_CANDIDATES})
resolve_paddle_library(PADDLE_LIBCOMMON ${PADDLE_LIBCOMMON_CANDIDATES})
resolve_paddle_library(PADDLE_LIBPHI_CORE ${PADDLE_LIBPHI_CORE_CANDIDATES})

set(
TORCH_LIBRARIES
"${PADDLE_PATH}/base/libpaddle.so"
"${PADDLE_PATH}/libs/libcommon.so"
"${PADDLE_LIBPADDLE}"
"${PADDLE_LIBCOMMON}"
# "${PADDLE_PATH}/libs/libphi.so" # currently libphi.so is static linked, we need remove it when it's shared linked
"${PADDLE_PATH}/libs/libphi_core.so"
"${PADDLE_LIBPHI_CORE}"
)
set(
TORCH_INSTALL_PREFIX
Expand Down Expand Up @@ -82,9 +120,24 @@ function(make_torchcodec_sublibrary
${library_name}
PUBLIC
${library_dependencies}
"-Wl,--as-needed"
)

if(UNIX AND NOT APPLE)
target_link_options(
${library_name}
PUBLIC
"-Wl,--as-needed"
)
endif()

if(APPLE)
target_link_options(
${library_name}
PUBLIC
"LINKER:-undefined,dynamic_lookup"
Copy link
Copy Markdown

@ShigureNyako ShigureNyako Mar 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this addresses the macOS runtime failure described in the PR.

-undefined,dynamic_lookup only relaxes symbol resolution at link time; it does not add any runtime search path for FFmpeg dylibs. The failure we usually hit on macOS is Library not loaded: @rpath/libavutil*.dylib during import torchcodec / torch.ops.load_library(...).

Upstream torchcodec fixed that class of bug in meta-pytorch#1152 (see issue meta-pytorch#570) by adding an INSTALL_RPATH for Homebrew FFmpeg. This patch does not add any INSTALL_RPATH, @loader_path, or install-name adjustment, so users will still need to set DYLD_LIBRARY_PATH manually.

Can we port the actual runtime search-path fix here, and add at least one macOS validation for import torchcodec?

)
endif()

endfunction()

function(make_torchcodec_libraries
Expand Down
Loading