Skip to content

add nanobind include path to CMakeLists#2952

Closed
andrej wants to merge 1 commit intoXilinx:mainfrom
andrej:fix-nanobind
Closed

add nanobind include path to CMakeLists#2952
andrej wants to merge 1 commit intoXilinx:mainfrom
andrej:fix-nanobind

Conversation

@andrej
Copy link
Collaborator

@andrej andrej commented Mar 11, 2026

For some reason, this now seems to be required for me to be able to build? Otherwise, I get include errors. Solution found by Claude, please scrutinize because I don't 100% understand why this is now suddenly required.

@andrej andrej requested a review from jackl-xilinx as a code owner March 11, 2026 20:25
Copilot AI review requested due to automatic review settings March 11, 2026 20:25
@jgmelber
Copy link
Collaborator

@erwei-xilinx did you come across this in the EUDSL bump?

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates the Python bindings CMake configuration to explicitly use nanobind and to add nanobind’s include directory to the build interface, addressing build-time include resolution issues for the Python extension modules.

Changes:

  • Set PYTHON_BINDINGS_LIBRARY nanobind on the MLIR and XRT Python extension declarations.
  • Add ${NB_DIR}/include to the build include paths for AIEPythonExtensions.MLIR (python-passes/omnibus branch).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 230 to 234
target_include_directories(
AIEPythonExtensions.MLIR
INTERFACE $<BUILD_INTERFACE:${PYBINDINGS_SRC}>
INTERFACE $<BUILD_INTERFACE:${NB_DIR}/include>
)
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

${NB_DIR} is not defined anywhere in this repository, so if it isn't set by the included AddMLIRPython module/configure step it will expand to an empty string here and effectively add /include to the include path. Consider guarding this with an explicit if (NOT DEFINED NB_DIR) error (or deriving the include directory from the nanobind CMake package/target) so misconfiguration fails fast with a clear message instead of producing a confusing include path.

Copilot uses AI. Check for mistakes.
@fifield
Copy link
Collaborator

fifield commented Mar 11, 2026

@erwei-xilinx did you come across this in the EUDSL bump?

I ran into similar issue and it went away by updating llvm + eudsl.

@erwei-xilinx
Copy link
Collaborator

Let me investigate

@hunhoffe
Copy link
Collaborator

I also ran into this issue, but it disappeared when I got fresh eudsl + llvm

@erwei-xilinx
Copy link
Collaborator

erwei-xilinx commented Mar 11, 2026

CC investigated this. The PYTHON_BINDINGS_LIBRARY parameter was removed from upstream LLVM's declare_mlir_python_extension CMake function in commit 3d7018c70b97 ("[MLIR][Python] remove pybind11 support (#172581)", Dec 19 2025) by @makslevental. Since pybind11 had been deprecated for over a year, that commit made nanobind the only (and default) binding library, so the parameter is no longer accepted.

mlir-aie's current main branch pins LLVM at 979132a02d14 (Feb 2026), which already includes that removal. PR #2918 specifically removed PYTHON_BINDINGS_LIBRARY nanobind from these exact call sites for this reason — passing the now-unrecognized parameter causes CMake to treat it as unparsed arguments and forward them as literal linker flags (-lPYTHON_BINDINGS_LIBRARY -lnanobind), leading to linker failures.

Re-adding PYTHON_BINDINGS_LIBRARY nanobind here would reintroduce that breakage on the current LLVM version. The include path issue (${NB_DIR}/include) is likely a separate problem — as @fifield and @hunhoffe noted, it goes away with a fresh LLVM + eudsl build, which suggests it's a stale build artifact rather than a missing CMake configuration.

@andrej
Copy link
Collaborator Author

andrej commented Mar 12, 2026

Excellent, thanks for the quick helpful replies. I'm using build-mlir-aie-from-wheels.sh and it probably just picked up an old cached version of the LLVM wheel. Let me retry after deleting it.

@andrej andrej closed this Mar 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants