[mlir python][cmake] Fixed nanobind target used in target_compile_options in AddMLIRPython.cmake

[vfdev-5]

Description:

target_compile_options(nanobind-static ...) is only works for default configuration of nanobind module nanobind_add_module(...) and the target name is different for other passed options like FREE_THREADED: nanobind-static-ft.

In this PR we first try the default name and if not found we search among all targets of the current cmake source directory (e.g. llvm-project/mlir/python).

Related to #107103

cc @jpienaar @makslevental

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-mlir

Author: vfdev (vfdev-5)

Changes

Description:

target_compile_options(nanobind-static ...) is only works for default configuration of nanobind module nanobind_add_module(...) and the target name is different for other passed options like FREE_THREADED: nanobind-static-ft.

In this PR we first try the default name and if not found we search among all targets of the current cmake source directory (e.g. llvm-project/mlir/python).

Related to #107103

cc @jpienaar

---

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

1 Files Affected:

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

diff --git a/mlir/cmake/modules/AddMLIRPython.cmake b/mlir/cmake/modules/AddMLIRPython.cmake
index 9d4e06c7909c81..5cf3303203950f 100644
--- a/mlir/cmake/modules/AddMLIRPython.cmake
+++ b/mlir/cmake/modules/AddMLIRPython.cmake
@@ -673,7 +673,25 @@ function(add_mlir_python_extension libname extname)
 
     if (LLVM_COMPILER_IS_GCC_COMPATIBLE OR CLANG_CL)
       # Avoids warnings from upstream nanobind.
-      target_compile_options(nanobind-static
+      set(nanobind_target "nanobind-static")
+      if (NOT TARGET ${nanobind_target})
+        # Get correct nanobind target name: nanobind-static-ft or something else
+        # It is set by nanobind_add_module function according to the passed options
+        get_property(all_targets DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} PROPERTY BUILDSYSTEM_TARGETS)
+
+        # Iterate over the list of targets
+        foreach(target ${all_targets})
+          # Check if the target name matches the given string
+          if("${target}" MATCHES "nanobind-")
+            set(nanobind_target "${target}")
+          endif()
+        endforeach()
+
+        if (NOT TARGET ${nanobind_target})
+          message(FATAL_ERROR "Could not find nanobind target to set compile options to")
+        endif()
+      endif()
+      target_compile_options(${nanobind_target}
         PRIVATE
           -Wno-cast-qual
           -Wno-zero-length-array

[makslevental]

I'm approving to unblock (because the impl is ~correct) but I think we should have a quick discussion about steps going forward because I think this is really brittle. I guess the correct thing to do here is either

1. remove those compile options
2. fix them upstream in nanobind itself
   • unfortunately some of them (at least -Wcast-qual) cannot be fixed just because of how nanobind works.

cc @jpienaar @hawkinsp @stellaraccident

[hawkinsp]

I'm approving to unblock (because the impl is ~correct) but I think we should have a quick discussion about steps going forward because I think this is really brittle. I guess the correct thing to do here is either

1. remove those compile options

2. fix them upstream in nanobind itself

   • unfortunately some of them (at least -Wcast-qual) cannot be fixed just because of how nanobind works.

cc @jpienaar @hawkinsp @stellaraccident

Note the upstream is fairly opinionated about the underlying source of the warnings: https://github.com/wjakob/nanobind/blob/b7c4f1abfcab9cc5a8f0ef758926d92ff5eac3a3/include/nanobind/nb_attr.h#L263-L288

(If it were me, I'd probably write, say, const_cast instead of using a C-style cast, but: their code base, their rules.)

I don't know cmake at all and find it incomprehensible, but could we upstream a change to their cmake macros to silence the warnings?

[joker-eph]

I thought @jpienaar implemented a solution where we suppress the warning with pragmas instead, why do we need these compile option in CMake still?

[makslevental]

I thought jpienaar implemented a solution where we suppress the warning with pragmas instead

It couldn't/can't be done - the warnings stem from both templates and the lib and in fact templates in the headers (so you can't bracket the #includes with the pragmas).

why do we need these compile option in CMake still?

the compile options are already being set (that's what @jpienaar landed), this just handles the upcoming transition to free threading which incurs a rename to the nanobind lib.

[vfdev-5]

I don't know cmake at all and find it incomprehensible, but could we upstream a change to their cmake macros to silence the warnings?

We maybe can try to upstream an optional kwarg which defines nanobind compile options (NB_COMPILE_OPTIONS) as following:

cmake_minimum_required(VERSION 3.28)
project("test")

# nanobind nanobind_add_module function implementation
# with additional multi-value arg NB_COMPILE_OPTIONS
# https://cmake.org/cmake/help/latest/command/cmake_parse_arguments.html
function(nanobind_add_module2 name)
    cmake_parse_arguments(PARSE_ARGV 1 ARG
        "STABLE_ABI;FREE_THREADED;NB_STATIC;NB_SHARED;PROTECT_STACK;LTO;NOMINSIZE;NOSTRIP;MUSL_DYNAMIC_LIBCPP"
        "NB_DOMAIN" "NB_COMPILE_OPTIONS")

    message("---- START of nanobind_add_module2")
    message("ARG_NB_SHARED: ${ARG_NB_SHARED}")
    message("ARG_STABLE_ABI: ${ARG_STABLE_ABI}")
    message("ARG_FREE_THREADED: ${ARG_FREE_THREADED}")
    message("ARG_NB_STATIC: ${ARG_NB_STATIC}")
    message("ARG_UNPARSED_ARGUMENTS: ${ARG_UNPARSED_ARGUMENTS}")
    message("ARG_NB_COMPILE_OPTIONS: ${ARG_NB_COMPILE_OPTIONS}")
    message("---- END of nanobind_add_module2")

    add_library(${name} MODULE ${ARG_UNPARSED_ARGUMENTS})

    if (ARG_NB_COMPILE_OPTIONS)
        target_compile_options(
            ${name}  # in nanobind this should be libname and not name, here it is to simplify the example
            PRIVATE
            ${ARG_NB_COMPILE_OPTIONS}
        )
    endif()

endfunction()


nanobind_add_module2(
    my_ext                   # Target name
    NB_STATIC STABLE_ABI LTO # Optional flags (see below)
    my_ext.h                 # Source code files below
    my_ext.cpp
)


nanobind_add_module2(
    my_ext2                  # Target name
    NB_STATIC STABLE_ABI LTO # Optional flags (see below)
    my_ext.h                 # Source code files below
    my_ext.cpp
    NB_COMPILE_OPTIONS
    -Wno-cast-qual
    -Wno-zero-length-array
    -Wno-nested-anon-types
    -Wno-c++98-compat-extra-semi
    -Wno-covered-switch-default
)

in this case, for my_ext flags.make:

CXX_FLAGS = -fPIC

and for my_ext2 flags.make:

CXX_FLAGS = -fPIC -Wno-cast-qual -Wno-zero-length-array -Wno-nested-anon-types -Wno-c++98-compat-extra-semi -Wno-covered-switch-default

[hpkfft]

Does the following work instead of iterating over the list of all targets:

get_property(nanobind_target TARGET ${libname} PROPERTY LINK_LIBRARIES)

[jpienaar]

OOC was the macro change proposed?

[hpkfft]

An option NB_SUPPRESS_WARNINGS was added to nanobind_add_module.
See: wjakob/nanobind#868

Hopefully, this is suitable for your use case.