[MLIR] Fix nanobind linker args on macOS

[nikalra]

Set the "-U,_PyClassMethod_New" linker flag on the Python library itself isntead of on the CMAKE_MODULE_LINKER_FLAGS property.

[llvmbot]

@llvm/pr-subscribers-mlir

Author: Nikhil Kalra (nikalra)

Changes

Set the "-U,_PyClassMethod_New" linker flag on the Python library itself isntead of on the CMAKE_MODULE_LINKER_FLAGS property.

---

Full diff: https://github.com/llvm/llvm-project/pull/125733.diff

1 Files Affected:

• (modified) mlir/cmake/modules/AddMLIRPython.cmake (+1-1)

diff --git a/mlir/cmake/modules/AddMLIRPython.cmake b/mlir/cmake/modules/AddMLIRPython.cmake
index a23de004eb014ff..a21688213339b05 100644
--- a/mlir/cmake/modules/AddMLIRPython.cmake
+++ b/mlir/cmake/modules/AddMLIRPython.cmake
@@ -709,7 +709,7 @@ function(add_mlir_python_extension libname extname)
       # NanobindAdaptors.h uses PyClassMethod_New to build `pure_subclass`es but nanobind
       # doesn't declare this API as undefined in its linker flags. So we need to declare it as such
       # for downstream users that do not do something like `-undefined dynamic_lookup`.
-      set(CMAKE_MODULE_LINKER_FLAGS "${CMAKE_MODULE_LINKER_FLAGS} -Wl,-U -Wl,_PyClassMethod_New")
+      target_link_options(${libname} PRIVATE "LINKER:-U,_PyClassMethod_New")
     endif()
   endif()
 

[makslevental]

But NanobindAdaptors.h is a header and so this flag does need to be set on all users of the header not just those building/using the library. Possibly actually this should be an extern in the header itself?

Just curious what issue do you have with the current solution?

[nikalra]

But NanobindAdaptors.h is a header and so this flag does need to be set on all users of the header not just those building/using the library. Possibly actually this should be an extern in the header itself?

Just curious what issue do you have with the current solution?

Would it be better if I made it a PUBLIC link property so that downstream link dependencies also inherited the symbol? They could include the header in isolation, but presumably they'd be linking against the core MLIR Python bindings.

I gave extern a shot, but that doesn't work because the symbol is still missing at link time (and we can't link against a Python dylib for the linker to recognize the symbol exists).

I'm not sure what exactly is different between our build setup and the upstream one, but CMAKE_MODULE_LINKER_FLAGS were not getting propagated to the target so we'd fail to link when creating the Python .so. Our Python library is declared with declare_mlir_python_extension and executes through the exact flow where the property was getting applied.

[makslevental]

Would it be better if I made it a PUBLIC link property so that downstream link dependencies also inherited the symbol?

Ya that's a reasonable in-between.

Btw cc @marbre this change wouldn't affect IREE right because we just duplicated this same patch locally (in IREE CMake)?

[marbre]

Would it be better if I made it a PUBLIC link property so that downstream link dependencies also inherited the symbol?

Ya that's a reasonable in-between.

Btw cc @marbre this change wouldn't affect IREE right because we just duplicated this same patch locally (in IREE CMake)?

What we have in IREE atm to work around the issue is

    set(CMAKE_MODULE_LINKER_FLAGS "${CMAKE_MODULE_LINKER_FLAGS} -Wl,-U -Wl,_PyClassMethod_New")

at top-level (https://github.com/iree-org/iree/blob/56bb652fd6b03b7473994426aa4a698f87a1418a/CMakeLists.txt#L655) if building for macOS. IREE also uses declare_mlir_python_extension for its compiler Python bindings and what is there upstream didn't kicked in.

Happy to give this patch a try before merging upstream if you want me to (?).

[makslevental]

Happy to give this patch a try before merging upstream if you want me to (?).

Ya can you please.

[marbre]

Happy to give this patch a try before merging upstream if you want me to (?).

Ya can you please.

I did in the meantime, and the patch upstream allows us to drop the downstream one, see https://github.com/iree-org/iree/actions/runs/13155988107.

[makslevental]

[makslevental]

@nikalra let me know if you need me to merge this (not sure if you have commit access)