feat: Rework CMake search path settings #880
Conversation
LecrisUT
force-pushed
the
feat/ignore-prefix
branch
2 times, most recently
from
6b44e12
to
a178c3b
Compare
LecrisUT
changed the title
LecrisUT
force-pushed
the
feat/ignore-prefix
branch
2 times, most recently
from
81742a7
to
c9454cd
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #880 +/- ##
==========================================
- Coverage 83.90% 83.62% -0.29%
==========================================
Files 74 75 +1
Lines 4387 4451 +64
==========================================
+ Hits 3681 3722 +41
- Misses 706 729 +23 ☔ View full report in Codecov by Sentry. |
| This variable is populated from the entry-point `cmake.module` and the option | ||
| `search.modules` similar to [`CMAKE_PREFIX_PATH`] section. | ||
|
|
||
| [`CMAKE_PREFIX_PATH`]: #cmake-prefix-path |
There was a problem hiding this comment.
This link does indeed work, but the CI is reporting as not working 🤷
LecrisUT
force-pushed
the
feat/ignore-prefix
branch
2 times, most recently
from
5770c6a
to
732eab5
Compare
LecrisUT
force-pushed
the
feat/ignore-prefix
branch
2 times, most recently
from
659e0cb
to
836c51e
Compare
| _handle_search_paths( | ||
| "cmake.module", self.settings.search.modules, self.config.module_dirs | ||
| ) |
There was a problem hiding this comment.
Should we unify the naming convention across these 3 to the entry-point name? I.e.:
| _handle_search_paths( | |
| "cmake.module", self.settings.search.modules, self.config.module_dirs | |
| ) | |
| _handle_search_paths( | |
| "cmake.module", self.settings.search.module, self.config.module | |
| ) |
|
(Sorry I'm a bit slow reviewing, classes start this week and I'm teaching again) |
docs/search_paths.md
Outdated
|
|
||
| ```toml | ||
| [tool.scikit-build.search] | ||
| prefixes = ["/path/to/prefixA", "/path/to/prefixB"] |
There was a problem hiding this comment.
Yes, these are just paths. You can replace this with the following one line in CMakeLists.txt:
list(APPEND CMAKE_PREFIX_PATH /path/to/prefixA /path/to/prefixB)We should only add features hard to implement in CMake.
There was a problem hiding this comment.
I think it would be good to support this anyway because it could be used via overrides and cli. Similarly for the entry-point having a path as value instead of an importable python reference
There was a problem hiding this comment.
Entry points are supposed to be modules or objects in modules, not arbitrary strings. The syntax is <module>[:<attribute>].
There was a problem hiding this comment.
And you can still do it via overrides and CLI using defines.
There was a problem hiding this comment.
Hmm, I guess deleting the entry-point would also work. I was considering when packaging for Fedora and we would want the entry-point to point to the system path in /usr/local. But I guess there are other issues with that since the entry-points are defined when building the sdist.
What about checking if the entry-point points to a str or pathlib.Path/traversable or list of those. If not use the current logic of loading importlib.resources.files
There was a problem hiding this comment.
Hmm, this point needs to be revisited. If the user sets cmake.define.CMAKE_PREFIX_PATH, who would win? Other than that, shall we keep the list definition like this? It can be useful for setting via cli and resolving the ambiguity with scikit-build-core also setting that variable.
|
|
||
| # Add site-packages to the prefix path for CMake | ||
| site_packages = Path(sysconfig.get_path("purelib")) | ||
| self.config.prefix_dirs.append(site_packages) | ||
| if self.settings.search.use_install_prefix: |
There was a problem hiding this comment.
As I said above, that's not what this is. We are trying to add site-packages, and in some cases, that's not where scikit-build-core is installed, so we make sure we always add that too. We should probably be adding platlib too.
There was a problem hiding this comment.
Hmm, so each isolated build path is unique to the dependent packages being built and not to the call of pip install? I was mostly considering about that situation where you would need the prefixes of dependencies that are in build-system.requires only.
There was a problem hiding this comment.
You cannot view any other site-packages besides the isolated one (that's the isolated part!). The wheel is built separately from installing it. This was just a workaround for an issue where "site-packages" wasn't pointing to the place scikit-build-core was installed, I think due to user site-packages for non-isolated installs, but I don't remember exactly. It could also have been the include-system-site-packages = true setting for a (non-isolated) venv. But it's not install vs. build.
There was a problem hiding this comment.
There was a problem hiding this comment.
Indeed this is what I got:
$ pip -v install ./spglib --config-settings=logging.level=DEBUG
2024-09-09 10:41:40,787 - scikit_build_core - DEBUG - SITE_PACKAGES: /home/lecris/CLionProjects/spglib/venv/lib/python3.12/site-packages
2024-09-09 10:41:40,787 - scikit_build_core - DEBUG - Extra SITE_PACKAGES: /home/lecris/CLionProjects/spglib/venv/lib/python3.12/site-packages
2024-09-09 10:41:40,798 - scikit_build_core - DEBUG - Default generator: Ninja
set(CMAKE_PREFIX_PATH [===[/home/lecris/CLionProjects/spglib/venv/lib/python3.12/site-packages;/tmp/pip-build-env-b9zh9nkx/overlay/lib/python3.12/site-packages]===] CACHE PATH "" FORCE)note the actual value is in CMAKE_PREFIX_PATH due to a minor bug:
scikit-build-core/src/scikit_build_core/builder/builder.py
Lines 138 to 139 in 9f54443
In this case we have /tmp/pip-build-env- site-packages which points to where scikit-build-core was installed, which in the issolated build it would be this "build environment". And what I find is that the numpy is also installed in the same paths, and this is mostly what I was thinking of making configurable.
In contrast if I run it without build-isolation:
$ pip install scikit-build-core numpy
$ pip -v install ./spglib --config-settings=logging.level=DEBUG --no-build-isolation
2024-09-09 10:53:58,703 - scikit_build_core - DEBUG - SITE_PACKAGES: /home/lecris/CLionProjects/spglib/venv/lib/python3.12/site-packages
2024-09-09 10:53:58,703 - scikit_build_core - DEBUG - FindPython backport activated at /home/lecris/CLionProjects/spglib/venv/lib/python3.12/site-packages/scikit_build_core/resources/find_python
2024-09-09 10:53:58,706 - scikit_build_core - DEBUG - Default generator: Ninja
set(CMAKE_PREFIX_PATH [===[/home/lecris/CLionProjects/spglib/venv/lib/python3.12/site-packages]===] CACHE PATH "" FORCE)Of course site_packages != DIR.parent.parent is not a good check for if it's in the build-isolation because it intersect with scikit-build-core being editable installed, but in that case we know for sure we are running under --no-build-isolation and the other build dependencies are either in site_packages or their own editable path, which we do not have a way to control without them exporting their entry-point, so it technically still does what it "should do" similar to if scikit-build-core is not editable installed but --no-build-isolation is still used.
site_packages != DIR.parent.parent |
DIR.parent.parent |
use-build-prefix |
|
|---|---|---|---|
scikit-build-core in isolation |
True |
/tmp/pip-build-env-*/... |
"as intended" |
pip install scikit-build-core (without isolation) |
False |
venv/... |
ignored |
pip install -e scikit-build-core (without isolation) |
True |
~/scikit-build-core/src |
edge case |
There was a problem hiding this comment.
Strange, why is Path(sysconfig.get_path("purelib")) reporting the original venv when this is "isolated"? It seems to me it should report the isolated environment and you should not be able to get to the original one at all. Is that path in sys.path?
There was a problem hiding this comment.
Welp, how would I test that? It is not reported in the debug log, and if I would run it within the venv than yeah it should be included right?
$ source venv/bin/activate && python3 -c "import sys;print(sys.path)"
['', '/usr/lib64/python312.zip', '/usr/lib64/python3.12', '/usr/lib64/python3.12/lib-dynload', '/home/lecris/CLionProjects/spglib/venv/lib64/python3.12/site-packages', '/home/lecris/CLionProjects/spglib/venv/lib/python3.12/site-packages']
Probably when running with isolation it would run in a different created environment in which case it would need to call sys.path within the build steps to show its true value right?
There was a problem hiding this comment.
It will be reported in the debug log after #896. :)
(And yes, sys.path inside the build steps is needed)
There was a problem hiding this comment.
Here's an update on it:
2024-09-11 11:52:04,356 - scikit_build_core - DEBUG - SITE_PACKAGES: /home/lecris/CLionProjects/spglib/venv/lib/python3.12/site-packages
2024-09-11 11:52:04,357 - scikit_build_core - DEBUG - Extra SITE_PACKAGES: /tmp/pip-build-env-oiycakwm/overlay/lib/python3.12/site-packages
2024-09-11 11:52:04,357 - scikit_build_core - DEBUG - PATH: ['/home/lecris/CLionProjects/spglib/venv/lib/python3.12/site-packages/pip/_vendor/pyproject_hooks/_in_process', '/tmp/pip-build-env-oiycakwm/site', '/usr/lib64/python312.zip', '/usr/lib64/python3.12', '/usr/lib64/python3.12/lib-dynload', '/tmp/pip-build-env-oiycakwm/overlay/lib/python3.12/site-packages', '/tmp/pip-build-env-oiycakwm/overlay/lib64/python3.12/site-packages', '/tmp/pip-build-env-oiycakwm/normal/lib/python3.12/site-packages', '/tmp/pip-build-env-oiycakwm/normal/lib64/python3.12/site-packages', '/tmp/pip-build-env-oiycakwm/normal/lib/python3.12/site-packages/setuptools/_vendor']
The only path that is in the same venv is from pip/_vendor/pyproject_hooks/_in_process
There was a problem hiding this comment.
So this one is probably a pip bug. If I run python3 -m build or uv pip install then platlib is pointing to the isolated site-packages. Opened the upstream issue: pypa/pip#12976
Pulling out this fix from #880 for inclusion in a patch release. --------- Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> Co-authored-by: Cristian Le <cristian.le@mpsd.mpg.de>
LecrisUT
force-pushed
the
feat/ignore-prefix
branch
2 times, most recently
from
ba50760
to
03e46d6
Compare
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
LecrisUT
force-pushed
the
feat/ignore-prefix
branch
from
03e46d6
to
57514c7
Compare
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
LecrisUT
force-pushed
the
feat/ignore-prefix
branch
from
57514c7
to
a6bccfa
Compare
CMAKE_PREFIX_*,*_ROOT, etc. search settings and overridescmakerather than at the topI suspect this is related to
if site_packages != DIR.parent.parent:?entry_points(group="cmake.root"):<Pkg>_ROOT<Pkg>This allows to override the entry-points, e.g. setting to
""to delete that entrysite-packagesor others, e.g. config option could have{platlib}/other_projectNot sure how to do this one.
cmake.rootovercmake.prefix(cannot deprecate because some consumers might not usefind_package, e.g.swig)Closes #831
Closes #885
Relates to #860 (still keeping it open because it doesn't address the specific issue)