Skip to content

python: switch binding library to nanobind #948

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
Doekin wants to merge 1 commit into
base: master
Choose a base branch
from
Open

python: switch binding library to nanobind #948

Doekin wants to merge 1 commit into from

Conversation

@Doekin
Copy link
Contributor

@Doekin Doekin commented May 8, 2025

This PR migrates the Python bindings for zxingcpp from pybind11 to nanobind.

Key Benefits:

  1. Enhanced IDE Support (Autocompletion & Type Hinting):
    The primary driver for this change is nanobind's straightforward support for generating stub files (.pyi). These stubs enable autocompletion and type hinting in IDEs, significantly improving the development experience when using the zxingcpp library.

    439856664-5b6c6dfc-3e86-450a-b75d-fb6ca92cd863

  2. Smaller Binary Sizes

  3. Faster Compilation Times

Important Structural Change:

  • As a result of this migration and to better accommodate the stub files and nanobind's typical project structure, the zxingcpp Python extension has been refactored from a single-file module (e.g., zxingcpp.pyd or zxingcpp.so) into a Python package (a directory named zxingcpp containing an __init__.py and the compiled extension module).

@axxel
Copy link
Collaborator

axxel commented May 20, 2025

Sorry for not reacting to your PR so far. This is definitively interesting, I just don't have the time right now to properly look into this. But I will at some point in time.

@Doekin
Copy link
Contributor Author

Doekin commented May 21, 2025

No worries, take your time!

@stefan6419846
Copy link

stefan6419846 commented Sep 16, 2025

Does nanobind require switching the whole build backend from setuptools to scikit-build? If not, what are the reasons for this change and which benefits and side effects does this have?

@axxel
Copy link
Collaborator

axxel commented Dec 15, 2025

@Doekin Sorry for letting this PR wait so long without feedback. I plan to make a 2.4 release soonish, so I'm trying to catch up... Are you still around to discuss details? Would you be willing to fix the conflicts? Do have you have something to say to the question from @stefan6419846 ?

@Doekin
Copy link
Contributor Author

Doekin commented Dec 15, 2025

Thanks for checking in. Yes, I'll update the PR and fix the conflicts.

As for the build backend: It's not mandatory, but scikit-build-core is the modern standard for CMake-based Python extensions. It offers better integration than setuptools and simplifies the configuration. It is the primary recommendation in the nanobind documentation.

@stefan6419846
Copy link

stefan6419846 commented Dec 16, 2025

With this changes, it seems like the build is broken - before, building the source distribution worked fine. Running PIP_INDEX_URL=https://pypi.org/simple python3.11 -m build . yields:

* Creating isolated environment: venv+pip...
* Installing packages in isolated environment:
  - nanobind >=1.3.2
  - scikit-build-core >=0.10
  - typing_extensions >=4.12;python_version<"3.11"
* Getting build dependencies for sdist...
* Building sdist...
*** scikit-build-core 0.11.6 (sdist)

Traceback (most recent call last):
  File "/home/stefan/.local/lib/python3.11/site-packages/build/__main__.py", line 178, in _handle_build_error
    yield
  File "/home/stefan/.local/lib/python3.11/site-packages/build/__main__.py", line 429, in main
    built = build_call(
            ^^^^^^^^^^^
  File "/home/stefan/.local/lib/python3.11/site-packages/build/__main__.py", line 276, in build_package_via_sdist
    t.extractall(sdist_out)
  File "/usr/lib64/python3.11/tarfile.py", line 2305, in extractall
    tarinfo, unfiltered = self._get_extract_tarinfo(
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib64/python3.11/tarfile.py", line 2394, in _get_extract_tarinfo
    self._handle_fatal_error(e)
  File "/usr/lib64/python3.11/tarfile.py", line 2392, in _get_extract_tarinfo
    filtered = filter_function(unfiltered, path)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib64/python3.11/tarfile.py", line 843, in data_filter
    new_attrs = _get_filtered_attrs(member, dest_path, True)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib64/python3.11/tarfile.py", line 830, in _get_filtered_attrs
    raise LinkOutsideDestinationError(member, target_path)
tarfile.LinkOutsideDestinationError: 'zxing_cpp-2.3.0/LICENSE' would link to '/tmp/LICENSE', which is outside the destination

ERROR 'zxing_cpp-2.3.0/LICENSE' would link to '/tmp/LICENSE', which is outside the destination

axxel
axxel reviewed Dec 16, 2025
View reviewed changes
Copy link
Collaborator

@axxel axxel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for coming back to this after this long delay. As you can see, I have a couple of questions.

I'd need to make sure the whole buffer_protocol + __array_interface__ code still works. I'm not sure all uses cases are covered in the tests...

wrappers/python/CMakeLists.txt
# Target the stable ABI for Python 3.12+, which reduces
# the number of binary wheels that must be built. This
# does nothing on older Python versions
STABLE_ABI
Copy link
Collaborator

@axxel axxel Dec 16, 2025

Choose a reason for hiding this comment

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

That sounds interesting. Do you know how the publish-pyhton.yml file would need to change to benefit from that? I'd also like to point out this commit by @chamalgomes. Which is touching on the same issue, at least in terms of build-times.

wrappers/python/CMakeLists.txt
set(PYBIND11_FINDPYTHON ON) # see https://github.com/pybind/pybind11/issues/4785
zxing_add_package(pybind11 pybind11 https://github.com/pybind/pybind11.git v3.0.1)
# Try to import all Python components potentially needed by nanobind
find_package(Python 3.8
Copy link
Collaborator

@axxel axxel Dec 16, 2025

Choose a reason for hiding this comment

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

Why exactly 3.8?

wrappers/python/CMakeLists.txt
set(ZXING_PYTHON_INSTALL_BINDIR "${CMAKE_INSTALL_BINDIR}")
if (SKBUILD)
set_target_properties(zxingcpp PROPERTIES
LIBRARY_OUTPUT_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
Copy link
Collaborator

@axxel axxel Dec 16, 2025

Choose a reason for hiding this comment

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

Does this really need to be an in-source-tree build?

wrappers/python/CMakeLists.txt
LIBRARY_OUTPUT_DIRECTORY_DEBUG ${CMAKE_CURRENT_SOURCE_DIR})
endif()

# Copy ZXing.dll alongside zxingcpp.pyd to solve ImportError during stub generation
Copy link
Collaborator

@axxel axxel Dec 16, 2025

Choose a reason for hiding this comment

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

So you are actively developing on windows? Very helpful to find those kind of special handling requirements...

wrappers/python/CMakeLists.txt
LIBRARY DESTINATION "${ZXING_PYTHON_INSTALL_LIBDIR}")
install(FILES ${CMAKE_CURRENT_SOURCE_DIR}/init.py
COMPONENT python
RENAME __init__.py
Copy link
Collaborator

@axxel axxel Dec 16, 2025

Choose a reason for hiding this comment

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

This file is generated by nanobind, right? Why would they name it init.py when it has to be renamed anyway?

wrappers/python/pyproject.toml
# Build stable ABI wheels for CPython 3.12+
wheel.py-api = "cp312"

[tool.cibuildwheel]
Copy link
Collaborator

@axxel axxel Dec 16, 2025

Choose a reason for hiding this comment

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

How does this section conflict/interact with

zxing-cpp/.github/workflows/publish-python.yml

Line 51 in 95a1c41

CIBW_TEST_COMMAND: python {package}/test.py

wrappers/python/pyproject.toml
"pybind11[global]",
]
build-backend = "setuptools.build_meta"
requires = ["scikit-build-core >=0.10", "nanobind >=1.3.2", "typing_extensions >=4.12;python_version<\"3.11\""]
Copy link
Collaborator

@axxel axxel Dec 16, 2025

Choose a reason for hiding this comment

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

What is typing_extension used for and why does the python_version need to be < 3.11?

wrappers/python/zxing.cpp
auto shape = ai["shape"].cast<std::vector<py::ssize_t>>();
auto typestr = ai["typestr"].cast<std::string>();
auto ai = nb::cast<nb::dict>(_image.attr("__array_interface__"));
auto shape = nb::cast<std::vector<size_t>>(ai["shape"]);
Copy link
Collaborator

@axxel axxel Dec 16, 2025

Choose a reason for hiding this comment

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

Are you sure, this is suddenly unsigned?

wrappers/python/zxing.cpp
imgfmt = ImageFormat::BGR;
else
throw py::value_error("Unsupported number of channels for buffer: " + std::to_string(channels));
throw nb::value_error(("Unsupported number of channels for buffer: " + std::to_string(channels)).c_str());
Copy link
Collaborator

@axxel axxel Dec 16, 2025

Choose a reason for hiding this comment

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

Are you sure that passing a pointer to a temporary const char* is valid here?

wrappers/python/zxing.cpp
return 0;
}

extern "C" inline int ImageView_getbuffer(PyObject* obj, Py_buffer* view, int flags) {
Copy link
Collaborator

@axxel axxel Dec 16, 2025

Choose a reason for hiding this comment

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

extern "C" + inline? I'm confused. :)

@Doekin
Copy link
Contributor Author

Doekin commented Dec 16, 2025

OK, I'll try to address these in the next few days.

@Doekin
Copy link
Contributor Author

Doekin commented Dec 16, 2025

Re: @stefan6419846 : Perhaps it is the same issue as scikit-build/scikit-build-core#801 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@axxel axxel axxel left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Doekin @axxel @stefan6419846