Replies: 7 comments 1 reply
-
|
Let me add @joaander and @henryiii who worked on the current build system. In general, I will say that the current one serves its purpose, so I am not very inclined to make huge disruptive changes. |
Beta Was this translation helpful? Give feedback.
-
|
FWIW |
Beta Was this translation helpful? Give feedback.
-
|
If Lines 81 to 96 in b7c4f1a There is no need to redesign the whole build system to solve this problem. |
Beta Was this translation helpful? Give feedback.
-
The motivation for the redesign is to avoid having a whack-a-mole situation where one issue is fixed only to uncover another unforeseen edge-case. The presence of the The suggestion of Method1 is in fact very minimal, just re-arranging the project files to better signal the structure of the project, and using more standard CMake conventions such as |
Beta Was this translation helpful? Give feedback.
-
|
I hope option (2) as documented in https://nanobind.readthedocs.io/en/latest/building.html#finding-nanobind remains supported. |
Beta Was this translation helpful? Give feedback.
-
|
Of course that can be supported, but I would recommend method 2 in that case in order to support more modern workflows like That is the reason for raising the design requirements question. |
Beta Was this translation helpful? Give feedback.
-
|
This ticket has been open for 3 weeks and discusses relatively disruptive changes to the build system. I prefer to keep issues for things that are truly broken and will convert it to a discussion for now. |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
-
@nicholasjng found an interesting issue here with the current build system, basically because all files including those that are not in the git repo are installed, it can fail the
scikit-build-coreinstallation because it hits issues when trying to handle a.DS_Storefile. For production this is generally not a problem, but it got me looking at the design choices here and I am confused.Design requirements
First let's figure out the design requirements for this project:
find_package/FetchContent?sdistonly? Compiled wheel with pre-compiled (shared) library files? Anoarchwheel (the current option)?I don't think any of these choices are a requirement, i.e. you could design the build system to support (almost) any combination of those, but it is a question of what the project maintainers want to support and design.
Redesign suggestion
Focusing on the
pip installpath, I have a few proposals.Method 1
scikit-build-corehas awheel.cmakeoption which can be set tofalse, meaning the only process done is to install the files underwheel.packagesare installed. Here is how it can be used:src/nanobindfolder. This has the advantage ofscikit-build-corehandling which files to be installed which accounts for.gitignore(also un-committed files maybe?).CMakeLists.txtor use it only to generate files likenanobindVersion.cmakeor such. If the top-levelCMakeLists.txtis removed, setwheel.cmake=falsecmake.prefixentry-point inpyproject.tomland point it tonanobindpython package, which should allow it to pick upcmake/nanobind-config.cmakefile which would in turn create the necessary CMake targets and functionsThis is the least involved method to implement, and should be equivalent with the current design.
Method 2
This is mostly to show other ways that this can be handled. The design for this method is to align more with a CMake project and allow more ways of distribution, e.g. via
FetchContentorfind_packagewithout Python package metadata and files.CMakeLists.txtbuilds and installs a library target, either as a compiled library orINTERFACEone. CMake'sFILE_SETandtarget_sourcesshould help minimize the files installed and consumed to only the relevant filespython/nanobindorsrc/nanobindfolder, which could be limited to python packaging distribution, or it could have manual installation instructions toPython_SITEARCH(but I strongly recommend against the manual installation because there is no method ofpip removeit)wheel.platlibmight need to be changed${CMAKE_INSTALL_INCLUDEDIR},${CMAKE_INSTALL_DATADIR}/cmake/nanobindwhich would be expanded asnanobind/include,nanobind/cmake/nanobind, etc. when combined withwheel.install-dir=nanobindcmake.prefixentry-point pointing tonanobindpython package. CMake internals should handle all the rest from thereFetchContentIn either approach the user is unaffected, i.e they would still be able to simply use
(PS: if the consumer uses
scikit-build-coreyou don't really need the documentedexecute_processbecause it populates theCMAKE_PREFIX_PATHwith the build'ssite_packages, but addingcmake.prefixshould make it even more robust)Beta Was this translation helpful? Give feedback.
All reactions