wjakob / nanobind

nanobind: tiny and efficient C++/Python bindings
BSD 3-Clause "New" or "Revised" License
2.29k stars 191 forks source link

Add nanobindConfigVersion.cmake and convert build to scikit-build-core #618

Closed joaander closed 2 months ago

joaander commented 3 months ago

TODO:

joaander commented 3 months ago

Checking with a diff, the CMake solution installs a few extra files not present in the setup.py solution. This can be fixed with a few small changes to the CMake code:

diff -r .venv2/lib/python3.12/site-packages/nanobind .venv/lib/python3.12/site-packages/nanobind
Only in .venv/lib/python3.12/site-packages/nanobind/cmake: collect-symbols-pypy.py
Only in .venv/lib/python3.12/site-packages/nanobind/cmake: collect-symbols.py
Only in .venv/lib/python3.12/site-packages/nanobind/cmake: nanobindConfigVersion.cmake
Only in .venv/lib/python3.12/site-packages/nanobind/ext/robin_map: CMakeLists.txt
Only in .venv/lib/python3.12/site-packages/nanobind/ext/robin_map/include/tsl: robin_set.h
henryiii commented 3 months ago

sdist solution

What's wrong with SDists with this? It should Just Work™.

henryiii commented 3 months ago

If you'd like to add me to your fork I can of the metadata conversion using hatch.

joaander commented 3 months ago

sdist solution

What's wrong with SDists with this? It should Just Work™.

That's what I like to hear.

If you'd like to add me to your fork I can of the metadata conversion using hatch.

I invited you, go ahead and help out where you can. I appreciate it.

henryiii commented 3 months ago

Capturing the version isn't easy with the current regex plugin, since it's spread out. I can make it easier, though, in a future release, by adding a recombine option. Or a "combined" version could be placed in the header, with a compile time assert to verify it matches.

henryiii commented 3 months ago

How is the PROJECT_VERSION being set? Not seeing it in the CMakeLists.txt.

joaander commented 3 months ago

How is the PROJECT_VERSION being set? Not seeing it in the CMakeLists.txt.

I added it to the PROJECT command. I assumed that the project used bumpversion to update for release, but this is not the case: https://github.com/wjakob/nanobind/blob/master/docs/release.rst

https://github.com/joaander/nanobind/blob/91785cf770d5f6acfc6f7b5c9a097ed34ea1bdb1/CMakeLists.txt#L2

We could rewrite this to match the original setup.py behavior and read the version from nanobind.h.

joaander commented 3 months ago

I implemented the regex in cmake to pull the version from nanobind.h.

Aside from the duplicate version in pyproject.toml (which @henryiii) discusses in https://github.com/wjakob/nanobind/pull/618#issuecomment-2164351074, this branch now completely re-implements setup.py with CMakeLists.txt and scikit-build-core and adds nanobind-config-version.cmake as discussed in #596.

I tested

find_package(nanobind 2.0 CONFIG REQUIRED)

It enforces the version requirement and sets nanobind_VERSION as expected.

wojdyr commented 2 months ago

my two cents: if the move to scikit-build-core is not decided, it'd still be useful to apply the part of this PR that installs nanobindVersionConfig.cmake, so that users can call find_package(nanobind …) with version number.

wjakob commented 2 months ago

I've been mulling over this PR. The simplification is significant enough that I changed my mind and will accept it. If you would like to make further changes, please open a separate PR.

joaander commented 2 months ago

Thanks! Glad I could help.