Passing optional compile options to nanobind library in nanobind_add_module

[vfdev-5]

MLIR Python bindings are now using nanobind (migrated from pybind11 by @hawkinsp).
There is however a need to silent some compiler warning by passing additional flags to the nanobind library after nanobind_add_module call:

https://github.com/llvm/llvm-project/blob/1eed780a6a01b5b10de8a723318f0153b5adea0a/mlir/cmake/modules/AddMLIRPython.cmake#L676-L684

This approach however does not work due to nanobind target library name changes according to the options passed to nanobind_add_module, e.g. FREE_THREADED. A workaround is proposed here: llvm/llvm-project#121477 but ideally we would like have a solution for that in nanobind.

The question: @wjakob, would it be acceptable to add an optional arg to nanobind_add_module (e.g. NBLIB_COMPILE_OPTIONS) such that it would be used to define target compile options:

function(nanobind_add_module 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" "NBLIB_COMPILE_OPTIONS")
    ...
    add_library(${name} MODULE ${ARG_UNPARSED_ARGUMENTS})
    ...
    nanobind_build_library(${libname})
    
    if (ARG_NB_COMPILE_OPTIONS)
        target_compile_options(
            ${libname}
            PRIVATE
            ${ARG_NBLIB_COMPILE_OPTIONS}
        )
    endif()
    ...
endfunction()

If you have a better suggestion please share it as well.
Thanks!

[wjakob]

That's cool! I have concerns about ARG_NB_COMPILE_OPTIONS as an official API since this will also affect other module declarations in a non-obvious way.

What's the issue with inferring ${libname} from the outside and then directly calling target_compile_options? There really aren't that many possible names that could result.

[vfdev-5]

What's the issue with inferring ${libname} from the outside and then directly calling target_compile_options? There really aren't that many possible names that could result.

I think it means that llvm would need to replicate nanobind rules of target name creation which is not great. Searching for the target name is more like a workaround...

Ideally, they wanted to fix them in nanobind, but this is another story...

I have concerns about ARG_NB_COMPILE_OPTIONS as an official API since this will also affect other module declarations in a non-obvious way.

What kind of implication of ARG_NBLIB_COMPILE_OPTIONS you think about?

[wjakob]

What kind of implication of ARG_NBLIB_COMPILE_OPTIONS you think about?

The libnanobind component can be shared by multiple extensions in the same project. This feature has a non-local effect of applying flags that also impact the other users, which I think is confusing.

[hpkfft]

Would it work for everyone if nanobind were to set a CACHE variable from inside the nanobind_add_module function to the value of ${libname}? Something like NB_LIBRARY_TARGET_NAME....

[wjakob]

There could be multiple different libnanobind targets (e.g. simultaneous stable ABI and default build).

What I would instead suggest is to add an entry to the low-level CMake set of functions that computes the target name from a set of arguments.

[vfdev-5]

I can try to implement such function a propose a PR

[wjakob]

Sounds good!

[hpkfft]

Instead of a cache variable, maybe target properties are worth investigating?
I'm not at all a CMake expert, but just playing around with a simple toy extension, the following:

nanobind_add_module(my_extension myx.cpp)
get_property(NB_LIBRARY_TARGET_NAME TARGET my_extension PROPERTY LINK_LIBRARIES)
message(STATUS "nanobind library target name is: ${NB_LIBRARY_TARGET_NAME}")

prints

-- nanobind library target name is: nanobind-static

[hpkfft]

Note that #868 has been merged, so using NB_SUPPRESS_WARNINGS may be a solution.

[wjakob]

@vfdev-5 Is the ability to compute the target name still relevant given the addition of NB_SUPPRESS_WARNINGS?

[vfdev-5]

I think NB_SUPPRESS_WARNINGS may work for llvm. We can close this disscusion and optionally open a new one if needed. Thanks!