[mlir][python][cmake] Allow skipping nanobind compile options changes.

[ScottTodd]

Context: #107103 (comment)

This code is brittle, especially when called from a superproject that adds the nanobind-* target in a different source directory:

get_property(all_targets DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} PROPERTY BUILDSYSTEM_TARGETS)

The changes here do help with my downstream build, but I'm not sure if using the MLIR_DISABLE_CONFIGURE_PYTHON_DEV_PACKAGES option introduced in #117934 is the right fix given that the option is currently scoped directly to one location with a matching name:

llvm-project/mlir/cmake/modules/MLIRDetectPythonEnv.cmake

Lines 4 to 5 in 7ad8a3d

	macro(mlir_configure_python_dev_packages)
	if(NOT MLIR_DISABLE_CONFIGURE_PYTHON_DEV_PACKAGES)

Some other solutions to consider:

1. Search through an explicit list of target names using if (TARGET)
2. Iterate over all targets in the project, not just the targets in the current directory, using code like https://stackoverflow.com/a/62311397
3. Iterate over targets in the directory known to MLIR (llvm-project/mlir/python)
4. Move this target_compile_options setup into mlir_configure_python_dev_packages (I started on this, but that runs into similar issues where the target is defined in a different directory)

[llvmbot]

@llvm/pr-subscribers-mlir

Author: Scott Todd (ScottTodd)

Changes

Context: #107103 (comment)

This code is brittle, especially when called from a superproject that adds the nanobind-* target in a different source directory:

get_property(all_targets DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} PROPERTY BUILDSYSTEM_TARGETS)

The changes here do help with my downstream build, but I'm not sure if using the MLIR_DISABLE_CONFIGURE_PYTHON_DEV_PACKAGES option introduced in #117934 is the right fix given that the option is currently scoped directly to one location with a matching name:

llvm-project/mlir/cmake/modules/MLIRDetectPythonEnv.cmake

Lines 4 to 5 in 7ad8a3d

	macro(mlir_configure_python_dev_packages)
	if(NOT MLIR_DISABLE_CONFIGURE_PYTHON_DEV_PACKAGES)

Some other solutions to consider:

1. Search through an explicit list of target names using if (TARGET)
2. Iterate over all targets in the project, not just the targets in the current directory, using code like https://stackoverflow.com/a/62311397
3. Iterate over targets in the directory known to MLIR (llvm-project/mlir/python)
4. Move this target_compile_options setup into mlir_configure_python_dev_packages (I started on this, but that runs into similar issues where the target is defined in a different directory)

---

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

1 Files Affected:

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

diff --git a/mlir/cmake/modules/AddMLIRPython.cmake b/mlir/cmake/modules/AddMLIRPython.cmake
index 815f65b106d945..018a81f1ad5500 100644
--- a/mlir/cmake/modules/AddMLIRPython.cmake
+++ b/mlir/cmake/modules/AddMLIRPython.cmake
@@ -672,8 +672,11 @@ function(add_mlir_python_extension libname extname)
       ${ARG_SOURCES}
     )
 
-    if (LLVM_COMPILER_IS_GCC_COMPATIBLE OR CLANG_CL)
-      # Avoids warnings from upstream nanobind.
+    if (NOT MLIR_DISABLE_CONFIGURE_PYTHON_DEV_PACKAGES
+        AND (LLVM_COMPILER_IS_GCC_COMPATIBLE OR CLANG_CL))
+      # Avoid some warnings from upstream nanobind.
+      # If a superproject set MLIR_DISABLE_CONFIGURE_PYTHON_DEV_PACKAGES, let
+      # the super project handle compile options as it wishes.
       set(nanobind_target "nanobind-static")
       if (NOT TARGET ${nanobind_target})
         # Get correct nanobind target name: nanobind-static-ft or something else
@@ -702,7 +705,7 @@ function(add_mlir_python_extension libname extname)
           ${eh_rtti_enable}
       )
     endif()
-    
+
     if(APPLE)
       # 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

[ScottTodd]

cc @vfdev-5 @hawkinsp (feel free to add reviews too)

I can also try some other solutions and iterate a bit

[marbre]

Looks reasonable to me, thanks!

[ScottTodd]

Tested a bit more downstream and I'm confident that this solves our issues. The code here could still be reworked, ideally by fixing the nanobind warnings upstream and not needing to fiddle with compile options at all.

[marbre]

Tested a bit more downstream and I'm confident that this solves our issues. The code here could still be reworked, ideally by fixing the nanobind warnings upstream and not needing to fiddle with compile options at all.

I can confirm that this does resolve the downstream issue. I agree that this code still deserves to be reworked.