feat: Rework CMake search path settings #880

LecrisUT · 2024-08-29T12:00:36Z

Closes #831
Closes #885
Relates to #860 (still keeping it open because it doesn't address the specific issue)

codecov · 2024-08-29T13:36:32Z

Codecov Report

Attention: Patch coverage is 67.14286% with 23 lines in your changes missing coverage. Please review.

Project coverage is 83.62%. Comparing base (1124da7) to head (836c51e).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/scikit_build_core/builder/builder.py	53.65%	19 Missing ⚠️
src/scikit_build_core/cmake.py	33.33%	4 Missing ⚠️

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.
📢 Have feedback on the report? Share it here.

LecrisUT · 2024-08-29T15:46:11Z

docs/search_paths.md

+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


This link does indeed work, but the CI is reporting as not working 🤷

src/scikit_build_core/_compat/importlib/resources.py

LecrisUT · 2024-09-03T10:14:56Z

src/scikit_build_core/builder/builder.py

+        _handle_search_paths(
+            "cmake.module", self.settings.search.modules, self.config.module_dirs
+        )


Should we unify the naming convention across these 3 to the entry-point name? I.e.:

Suggested change

_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

)

henryiii · 2024-09-03T18:01:59Z

(Sorry I'm a bit slow reviewing, classes start this week and I'm teaching again)

README.md

src/scikit_build_core/builder/builder.py

henryiii · 2024-09-06T19:51:05Z

docs/search_paths.md

+
+```toml
+[tool.scikit-build.search]
+prefixes = ["/path/to/prefixA", "/path/to/prefixB"]


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.

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

Entry points are supposed to be modules or objects in modules, not arbitrary strings. The syntax is <module>[:<attribute>].

And you can still do it via overrides and CLI using defines.

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

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.

henryiii · 2024-09-06T19:53:46Z

src/scikit_build_core/builder/builder.py


        # 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:


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.

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.

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.

You cannot view any other site-packages besides the isolated one (that's the isolated part!).

Wait, but we saw that breaking in #860/#862 where the installing site-packages were included. I'll run the build for spglib again to confirm that it was indeed that case.

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

self.config.prefix_dirs.append(DIR.parent.parent)

logger.debug("Extra SITE_PACKAGES: {}", site_packages)

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

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?

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?

It will be reported in the debug log after #896. :)

(And yes, sys.path inside the build steps is needed)

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

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 <[email protected]> Co-authored-by: Cristian Le <[email protected]>

Signed-off-by: Cristian Le <[email protected]>

LecrisUT force-pushed the feat/ignore-prefix branch 2 times, most recently from 6b44e12 to a178c3b Compare August 29, 2024 13:01

LecrisUT changed the title ~~feat: Rework CMake prefix calculations~~ feat: Rework CMake search path settings Aug 29, 2024

LecrisUT force-pushed the feat/ignore-prefix branch 2 times, most recently from 81742a7 to c9454cd Compare August 29, 2024 13:20

LecrisUT marked this pull request as ready for review August 29, 2024 14:57

LecrisUT requested a review from henryiii August 29, 2024 14:57

LecrisUT mentioned this pull request Aug 29, 2024

fix: Python installation spglib/spglib#520

Open

LecrisUT commented Aug 29, 2024

View reviewed changes

henryiii added this to the v0.11.0 milestone Aug 29, 2024

LecrisUT mentioned this pull request Sep 2, 2024

cmake.prefix entry-point can fail when it is pointing to a MultiplexedPath #885

Open

LecrisUT force-pushed the feat/ignore-prefix branch 2 times, most recently from 5770c6a to 732eab5 Compare September 3, 2024 09:38

LecrisUT commented Sep 3, 2024

View reviewed changes

src/scikit_build_core/_compat/importlib/resources.py Outdated Show resolved Hide resolved

LecrisUT force-pushed the feat/ignore-prefix branch 2 times, most recently from 659e0cb to 836c51e Compare September 3, 2024 10:09

LecrisUT commented Sep 3, 2024

View reviewed changes

henryiii reviewed Sep 6, 2024

View reviewed changes

henryiii mentioned this pull request Sep 10, 2024

fix: support multiplexed path #896

Merged

henryiii added a commit that referenced this pull request Sep 10, 2024

fix: support multiplexed path (#896)

37daf61

Pulling out this fix from #880 for inclusion in a patch release. --------- Signed-off-by: Henry Schreiner <[email protected]> Co-authored-by: Cristian Le <[email protected]>

LecrisUT force-pushed the feat/ignore-prefix branch 2 times, most recently from ba50760 to 03e46d6 Compare September 24, 2024 12:51

Gate site_packages CMake prefix

68d9366

Signed-off-by: Cristian Le <[email protected]>

LecrisUT force-pushed the feat/ignore-prefix branch from 03e46d6 to 57514c7 Compare September 24, 2024 12:53

LecrisUT added 2 commits September 24, 2024 15:05

Read cmake.root entry_points and unify search path handling

f0caafc

Signed-off-by: Cristian Le <[email protected]>

Document search path behavior

a6bccfa

Signed-off-by: Cristian Le <[email protected]>

LecrisUT force-pushed the feat/ignore-prefix branch from 57514c7 to a6bccfa Compare September 24, 2024 13:05

LecrisUT mentioned this pull request Oct 8, 2024

Support semicolon-separated lists in SKBUILD_CMAKE_ARGS #927

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Rework CMake search path settings #880

feat: Rework CMake search path settings #880

LecrisUT commented Aug 29, 2024 •

edited

Loading

codecov bot commented Aug 29, 2024 •

edited

Loading

LecrisUT Aug 29, 2024

LecrisUT Sep 3, 2024

henryiii commented Sep 3, 2024

henryiii Sep 6, 2024

LecrisUT Sep 6, 2024

henryiii Sep 7, 2024

henryiii Sep 7, 2024

LecrisUT Sep 7, 2024

LecrisUT Sep 24, 2024

henryiii Sep 6, 2024

LecrisUT Sep 6, 2024 •

edited

Loading

henryiii Sep 7, 2024

LecrisUT Sep 7, 2024 •

edited

Loading

LecrisUT Sep 9, 2024

henryiii Sep 10, 2024

LecrisUT Sep 10, 2024

henryiii Sep 10, 2024

LecrisUT Sep 11, 2024

LecrisUT Sep 24, 2024 •

edited

Loading

	self.config.prefix_dirs.append(DIR.parent.parent)
	logger.debug("Extra SITE_PACKAGES: {}", site_packages)

	`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

feat: Rework CMake search path settings #880

Are you sure you want to change the base?

feat: Rework CMake search path settings #880

Conversation

LecrisUT commented Aug 29, 2024 • edited Loading

codecov bot commented Aug 29, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henryiii commented Sep 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LecrisUT Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LecrisUT Sep 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LecrisUT Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

LecrisUT commented Aug 29, 2024 •

edited

Loading

codecov bot commented Aug 29, 2024 •

edited

Loading

LecrisUT Sep 6, 2024 •

edited

Loading

LecrisUT Sep 7, 2024 •

edited

Loading

LecrisUT Sep 24, 2024 •

edited

Loading