Skip to content
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.

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

Editable install #114

Open
henryiii opened this issue Nov 18, 2022 · 4 comments
Open

Editable install #114

henryiii opened this issue Nov 18, 2022 · 4 comments

Comments

@henryiii
Copy link
Collaborator

henryiii commented Nov 18, 2022

The first step is to support a persistent build directory (done in 0.2). However, I could possibly implement this the slow way (always rebuild) as an experiment before implementing it properly with a static build directory, if that is a simpler order. (One problem with a static build directory is the virtualenv moves around, so the paths change).

@henryiii
Copy link
Collaborator Author

henryiii commented Jan 27, 2023

Persistent build dir work step 1

  1. Source dir

This is the first error:

CMake Error: The source "/private/var/folders/_8/xtbws09n017fbzdx9dmgnyyr0000gn/T/build-via-sdist-q5x52ag6/cmake-example-0.0.1/CMakeLists.txt" does not match the source "/private/var/folders/_8/xtbws09n017fbzdx9dmgnyyr0000gn/T/build-via-sdist-4wn1l97i/cmake-example-0.0.1/CMakeLists.txt" used to generate cache.  Re-run cmake with a different source directory for now.

Build unpacks the SDist into a temporary directory since it's doing Source -> SDist -> Wheel. We could either try to fix this by patching the recorded paths, or we could detect this and wipe the build directory, or throw a nicer error. I think I'm in favor of wiping the build directory.

  1. Build dir

If we just directly go to wheel, then Python discovery breaks. This is because we are setting the initial cache value without forcing it, so it gets the old cached value. There are two solutions - either we add "FORCE" to our initial cache file (which is not really "supported", but works in all CMake versions), or we rewrite the cache with the updated values. I'm probably going to start with the former since it's much easier to implement - @thewtex, @jcfr thoughts? Specifically, we are doing -CCMakeInit.txt and putting a set(... CACHE FORCE) in it instead of the "supported" (according to old mailing lists) set(... CACHE) command. But it's really just a normal CMake file that runs before the full file. My only worry is if this initial cache support is ever refactored to not support arbitrary CMake commands, it could start only supporting normal set commands. I don't need other arbitrary CMake code, but I do need FORCE.

Adding the FORCE does cause a direct source -> Wheel build to work with a reused build directory.

@jcfr
Copy link
Contributor

jcfr commented Jan 28, 2023

Thanks for the detailed analysis🙏

I should be able to review in the next days 🚀

@LecrisUT
Copy link
Collaborator

LecrisUT commented Apr 12, 2023

Maybe should edit this issue to track what are the missing parts. I will copy the suggestion from spglib:

  • Add a CMAKE_INSTALL_RPATH pointing to the library dependencies. For this we can either:
    • Manually specify the paths of the librareis, e.g. tool.scikit-build.installed-library-path calculated relative to CMAKE_INSTALL_PREFIX
    • More advanced, use cmake-file-api to fetch the details of the cmake targets. In the pyproject.toml, we just add the target(s) that contains the python library binding and we navigate through the dependency targets, and adding those to the equivalent of tool.scikit-build.installed-library-path above
  • Expose the cmake configurations that scikit-build-core uses as cmake-preset

@casperdcl
Copy link

casperdcl commented Dec 8, 2023

Hacky work-around for now using in-place builds to run pytest: https://amypad.github.io/CuVec/contrib... Hopefully can be improved drastically 🤞😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

No branches or pull requests

4 participants