Skip to content

Adapt to jrl-cmakemodules v2 and nix flake + nanobind bindings#67

Draft
arntanguy wants to merge 31 commits intojrl-umi3218:masterfrom
arntanguy:nix-nanobind
Draft

Adapt to jrl-cmakemodules v2 and nix flake + nanobind bindings#67
arntanguy wants to merge 31 commits intojrl-umi3218:masterfrom
arntanguy:nix-nanobind

Conversation

@arntanguy
Copy link
Copy Markdown
Contributor

@arntanguy arntanguy commented Jan 6, 2026

This PR is intended as a test of jrl-cmakemodules v2 (jrl-umi3218/jrl-cmakemodules#798) and nix flake integration. It is based on the ongoing nanobind bindings rewrite.

As-is:

  • nix flake integration with gepetto system
    • I could not figure out how to build for aarch64, so I only kept x86_64 in the flake for now
  • Adapt to jrl-cmakemodules v2. As-is it works, including nanobind bindings but:
    • I had to remove the add_subdirectory(src) and instead inline the code in the main CMakeLists. Otherwise jrl_export_package() fails because SpaceVecAlg is not found in BUILDSYSTEM_TARGETS
  • Test sphinx doc for nanobind

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci
path = cmake
url = https://github.com/jrl-umi3218/jrl-cmakemodules
[submodule "cmake-v2"]
path = cmake-v2
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You could use this script to handle the new jrl-cmakemodules.

This allows to remove submodules completely.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I didn't want to bother with figuring out nix setup (we cannot use FetchContent in that case), so I went the easy route of submodule for now. Turns out it was simple to handle in nix, so this is fixed in e590621

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good. Once the JRL V2 PR is merged, don't forget to update the link in the fetchcontent part

arntanguy and others added 7 commits January 7, 2026 12:59
__init__.py:
This is needed to make sphinx doc display as sva.<symbol> instead of
sva.sva_pywrap.<symbol>

__init__.pyi (generated by cmake):
This is needed for LSPs such as pyright to recognise sva.<symbol>

Is this a clean way of handling this issue?
@arntanguy
Copy link
Copy Markdown
Contributor Author

arntanguy commented Jan 7, 2026

Thanks @ahoarau for the review!

Can I ask your opinion on 00ea437 ? When exporting bindings for nanobind, we do

NB_MODULE(sva_pywrap, m)
...

and in cmake

nanobind_add_module(  sva_pywrap ... )

and

nanobind_add_stub(sva_pywrap_stub  MODULE  sva.sva_pywrap)

With the default __init__.py generated by

jrl_python_generate_init_py(sva_pywrap OUTPUT_PATH ${CMAKE_BINARY_DIR}/lib/site-packages/sva/__init__.py)

everything works as expected in the python interpreter, however other tools are less intuitive. Notably symbols appear under sva.sva_pywrap.<symbol name> instead of sva.<symbol name>. For example we get a sphinx+autodoc documentation that referer to classes as sva.sva_pywrap.<classname>, same for pyright LSP.
E.g

import sva
sva.xxx # does not trigger lsp completion
sva.sva_pywrap.xxx # triggers lsp completion

I played around a bit, and:

  • with the following __init__.py,
# Re-export all public names from sva_pywrap
# This is done to get a flat package structure where types appear as
# sva.<classname> instead of sva.sva_pywrap.<symbol>
#
# Consider symbols prefixed with an underscore _<symbol> as private.
# They remain accessible as sva.sva_pywrap._<symbol>
from . import sva_pywrap
for name in dir(sva_pywrap):
    if not name.startswith("_"):
        globals()[name] = getattr(sva_pywrap, name)

__all__ = [name for name in dir(sva_pywrap) if not name.startswith("_")]

we get the expected behaviour in sphinx, however LSP still does not work. To get LSP working, we also need an __init__.pyi file with

from .sva_pywrap import *

With these changes, it seems that everything works as is most intuitive. However I am not very familiar with python initialization, so I am not sure how valid of an approach this is? If this is a valid approach, should we make it the default/an option for jrl-cmakemodules?

@ahoarau
Copy link
Copy Markdown

ahoarau commented Jan 7, 2026

With these changes, it seems that everything works as is most intuitive. However I am not very familiar with python initialization, so I am not sure how valid of an approach this is? If this is a valid approach, should we make it the default/an option for jrl-cmakemodules?

Interesting point. I must admit I only ported the previous behavior. I must invoke @jorisv on this point.

Meanwhile, can you try with:

nanobind_add_stub(sva_pywrap_stub  MODULE  sva)

instead of:

nanobind_add_stub(sva_pywrap_stub  MODULE  sva.sva_pywrap)

Properties depending on eigen only are bound:
- simple eigen types are bound to numpy arrays
- quaternions use eigennanopy
- tests use numpy and eigennanopy

fix pre-commit
arntanguy and others added 3 commits January 9, 2026 15:39
@ManifoldFR
Copy link
Copy Markdown

My point of view is that the generated stub file should twin the compiled module. Kind of weird that LSP doesn't pick up the symbols from the init file and that an init stub file is required.

This shouldn't be the case. In my experience a star import in the init is usually sufficient?

@arntanguy arntanguy force-pushed the nix-nanobind branch 3 times, most recently from 1c09874 to d8c579a Compare February 2, 2026 16:39
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.

3 participants