vcpkg icon indicating copy to clipboard operation
vcpkg copied to clipboard

[nanobind] New port

Open ekilmer opened this issue 1 year ago • 16 comments

~This is a WIP until upstream makes a new release with the necessary changes to support packaging. After this happens, I will change the version to match upstream (instead of using date).~ I've decided to backport/cherry-pick the necessary CMake patches required for packaging the latest released version, 1.8.0.

Nanobind distributes its source files to build on-demand, so I have added a CI port to make sure this continues to work with the provided upstream example by the author of nanobind.

After testing, cross-compilation for a different architecture (arm64-windows in CI) is not supported due to the execution of the Python executable to figure out the Python module extension.

Closes: #33401

Supersedes: #31323

  • [x] Changes comply with the maintainer guide
  • [x] The name of the port matches an existing name for this component on https://repology.org/ if possible, and/or is strongly associated with that component on search engines.
  • [x] Optional dependencies are resolved in exactly one way. For example, if the component is built with CMake, all find_package calls are REQUIRED, are satisfied by vcpkg.json's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx
  • [x] The versioning scheme in vcpkg.json matches what upstream says.
  • [x] The license declaration in vcpkg.json matches what upstream says.
  • [x] The installed as the "copyright" file matches what upstream says.
  • [x] The source code of the component installed comes from an authoritative source.
  • [x] The generated "usage text" is accurate. See adding-usage for context.
  • [x] The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • [x] Only one version is in the new port's versions file.
  • [x] Only one version is added to each modified port's versions file.

ekilmer avatar Dec 04 '23 20:12 ekilmer

I'm very confused about what's happening. Why is python311.lib being listed for the Debug builds? And why does this only happen on Windows dynamic triplets? It's not listed in the link command (python311_d.lib is).

Snippet:

[13/13] cmd.exe /C "cd . && D:\downloads\tools\cmake-3.27.1-windows\cmake-3.27.1-windows-i386\bin\cmake.exe -E vs_link_dll --intdir=CMakeFiles\nanobind_example_ext.dir --rc=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\rc.exe --mt=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\mt.exe --manifests  -- C:\PROGRA~1\MICROS~1\2022\ENTERP~1\VC\Tools\MSVC\1437~1.328\bin\Hostx64\x64\link.exe  CMakeFiles\nanobind_example_ext.dir\src\nanobind_example_ext.cpp.obj  /out:nanobind_example_ext.cp311-win_amd64.pyd /implib:nanobind_example_ext.lib /pdb:nanobind_example_ext.pdb /dll /version:0.0 /machine:x64 /nologo    /debug /INCREMENTAL  nanobind-static.lib  D:\installed\x64-windows\debug\lib\python311_d.lib  kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib  && cd ."
FAILED: nanobind_example_ext.cp311-win_amd64.pyd 
cmd.exe /C "cd . && D:\downloads\tools\cmake-3.27.1-windows\cmake-3.27.1-windows-i386\bin\cmake.exe -E vs_link_dll --intdir=CMakeFiles\nanobind_example_ext.dir --rc=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\rc.exe --mt=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\mt.exe --manifests  -- C:\PROGRA~1\MICROS~1\2022\ENTERP~1\VC\Tools\MSVC\1437~1.328\bin\Hostx64\x64\link.exe  CMakeFiles\nanobind_example_ext.dir\src\nanobind_example_ext.cpp.obj  /out:nanobind_example_ext.cp311-win_amd64.pyd /implib:nanobind_example_ext.lib /pdb:nanobind_example_ext.pdb /dll /version:0.0 /machine:x64 /nologo    /debug /INCREMENTAL  nanobind-static.lib  D:\installed\x64-windows\debug\lib\python311_d.lib  kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib  && cd ."
LINK Pass 1: command "C:\PROGRA~1\MICROS~1\2022\ENTERP~1\VC\Tools\MSVC\1437~1.328\bin\Hostx64\x64\link.exe CMakeFiles\nanobind_example_ext.dir\src\nanobind_example_ext.cpp.obj /out:nanobind_example_ext.cp311-win_amd64.pyd /implib:nanobind_example_ext.lib /pdb:nanobind_example_ext.pdb /dll /version:0.0 /machine:x64 /nologo /debug /INCREMENTAL nanobind-static.lib D:\installed\x64-windows\debug\lib\python311_d.lib kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib /MANIFEST /MANIFESTFILE:CMakeFiles\nanobind_example_ext.dir/intermediate.manifest CMakeFiles\nanobind_example_ext.dir/manifest.res" failed (exit code 1104) with the following output:
LINK : fatal error LNK1104: cannot open file 'python311.lib'

ekilmer avatar Dec 07 '23 20:12 ekilmer

After running the link.exe command locally with /VERBOSE, I see python311.lib mentioned at the top of the output and then not again until it fails to find it:

Starting pass 1
Processed /DEFAULTLIB:msvcprtd
Processed /DEFAULTLIB:python311.lib
Processed /DEFAULTLIB:MSVCRTD
Processed /DEFAULTLIB:OLDNAMES

Searching for DEFAULTLIB doesn't yield any obvious locations that would cause python311.lib to appear here. I searched the source code of nanobind, CMake, vcpkg, and nanobind_example.

After some searching, I think this same issue (https://github.com/microsoft/vcpkg/issues/23052#issuecomment-1345600775) is present in nanobind: https://github.com/wjakob/nanobind/blob/eb308de0e3099e5abd8e2e9b60a8e0af81ee93db/include/nanobind/nb_python.h#L11-L19

ekilmer avatar Dec 07 '23 20:12 ekilmer

~This likely would have worked if the debug headers were not removed because pyconfig.h sets Py_DEBUG 1 for debug builds, and nanobind respects that variable.~ Nevermind, these files were different on my mac but not on Windows. Also, nanobind undefines _DEBUG before including the Python headers.

ekilmer avatar Dec 07 '23 21:12 ekilmer

Convert this PR to draft since there are errors on CI pipeline need to fix. Please ping us if this PR is ready for review again.

LilyWangLL avatar Dec 15 '23 07:12 LilyWangLL

CI is now passing. I am waiting for upstream to make a new release that includes the changes that make packaging easier.

I will leave this PR as a draft until nanobind makes a new release, but if there are any other comments on the content of this PR, I am happy to address them!

ekilmer avatar Dec 17 '23 22:12 ekilmer

@ekilmer When do you expect nano bind to release a version with the expected changes for vcpkg compatibility?

ADGrant avatar Dec 29 '23 20:12 ADGrant

Great news. Thanks.

ADGrant avatar Dec 29 '23 22:12 ADGrant

CI failures don't look related to this PR

ekilmer avatar Dec 30 '23 03:12 ekilmer

This needs to depend on the python3 port, IMO.

Hoikas avatar Jan 13 '24 18:01 Hoikas

Note. The following notes do not apply to your port, but to the implementation of the nanobind itself. nanobind does not provide any CMake export targets. As a result, in many cases it is not useful.

  1. I can't create a library using my own CMake functions without the nanobind_add_module helper function. I would like to use ninobind, for example, like other binding libraries:
    target_link_libraries(mylib
      PRIVATE
        nanobind::link_options
        nanobind::lto
        nanobind::extension_abi3
      PUBLIC
        nanobind::nonobind-abi3
    )
    
  2. I cannot export and install targets for mylib correctly with public dependency to the shared nanobind lib:
    export(TARGETS mylib
        FILE "${PROJECT_BINARY_DIR}/${export_name}.cmake"
    )
    export(PACKAGE ${package_name})
    install(TARGETS mylib
        EXPORT ${export_name}
        RUNTIME DESTINATION "${CMAKE_INSTALL_BINDIR}"
        LIBRARY DESTINATION "${CMAKE_INSTALL_LIBDIR}"
        ARCHIVE DESTINATION "${CMAKE_INSTALL_LIBDIR}"
        PUBLIC_HEADER DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}"
    )
    

yurybura avatar Jan 24 '24 14:01 yurybura

@yurybura I agree with your concerns, but it looks like a PR was closed that tries to provide what you say---https://github.com/wjakob/nanobind/pull/201.

There are still cases where this port is useful, like building and packaging a Python native module, where the installation is only expected to be used/loaded by the Python interpreter (and not other CMake projects).


@Hoikas

This needs to depend on the python3 port, IMO.

I can make that change if that's required for getting this merged, but I would argue the primary use case would be for building Python native modules where the majority of users would want Python to come from the system to build against a specific version of Python for their module.

Sure, people could create an empty overlay port for Python, but I feel like that is a bit advanced (but maybe I'm wrong). Maybe we could add some info to the usage file? But users would still have to build Python before seeing that text. I'm not sure of the best solution.

ekilmer avatar Jan 24 '24 15:01 ekilmer

@Hoikas I don't see why this port would need to depend on the Python port. I am using pybind11 to wrap a C++ api via the vcpkg port, I would just like the same support for nanobind. I am not interested in building Python 3 and I don't really want to maintain my own fork of the vcpkg repo with this PR merged.

ADGrant avatar Feb 14 '24 16:02 ADGrant

I don't see why this port would need to depend on the Python port. I am using pybind11 to wrap a C++ api via the vcpkg port, I would just like the same support for nanobind. I am not interested in building Python 3 and I don't really want to maintain my own fork of the vcpkg repo with this PR merged.

Your use case is outside the scope of vcpkg. vcpkg intends to control its own ecosystem and not pull from system libraries. It sounds like you're interested in FetchContent to include nanbind, IMO. If you really what to build nanobind with vcpkg, you can use an overlay port that doesn't depend on python3. Just copy and paste this port into your project and delete the python3 dependency from the manifest. For the main catalog, however, we should provide a solution that just works.

Hoikas avatar Feb 16 '24 16:02 Hoikas

@Hoikas The Python3 dependency is redundant for the nanobond port since it only copies the source files to the installation directory.

yurybura avatar Feb 16 '24 17:02 yurybura

I don't see why this port would need to depend on the Python port. I am using pybind11 to wrap a C++ api via the vcpkg port, I would just like the same support for nanobind. I am not interested in building Python 3 and I don't really want to maintain my own fork of the vcpkg repo with this PR merged.

Your use case is outside the scope of vcpkg. vcpkg intends to control its own ecosystem and not pull from system libraries. It sounds like you're interested in FetchContent to include nanbind, IMO. If you really what to build nanobind with vcpkg, you can use an overlay port that doesn't depend on python3. Just copy and paste this port into your project and delete the python3 dependency from the manifest. For the main catalog, however, we should provide a solution that just works.

I don't see how my use case is outside the scope of vcpkg. I have a project with a dependency on pybind11, I use vcpkg to consume that dependency without using an overlay port. It just works when I add pybind11 to my vcpkg.json manifest. I would like to be able to switch to nanobind by just changing my manifest to point to nanobind instead of pybind11. I am not interested in creating an overlay port or building Python 3 from source. Nor am I interested in using this particular library with another CMake project. The reason I am using nanobind or pybind11 is to provide access to native C++ code from Python.

ADGrant avatar Feb 16 '24 21:02 ADGrant

The binary results must not depend on installation order, or ABI tracking and caching would be broken. Using nanobind after installing port python3 must not behave different from installing just nanobind. Installation order is enforced by ... dependencies.

dg0yt avatar Feb 17 '24 07:02 dg0yt

@ekilmer can you re-mark this PR as 'ready' by cycling to draft and back once more to catch the maintainers attention

ebenali avatar Mar 17 '24 21:03 ebenali

I apologize for the delayed review. I have some questions regarding this new port, so I'm changing the status of this PR to draft.

LilyWangLL avatar Mar 18 '24 07:03 LilyWangLL

Thanks for the new port!

BillyONeal avatar Mar 22 '24 02:03 BillyONeal