From 6b02b1bd961dcaad1309031df1a1a268dbe6500e Mon Sep 17 00:00:00 2001 From: Philip Salvaggio Date: Tue, 20 Aug 2024 13:49:08 -0400 Subject: [PATCH] fix: editable installs now respect the value of wheel.install-dir (#867) Attempting to fix: https://github.com/scikit-build/scikit-build-core/issues/866 I'm passing down wheel.install-dir to the editable redirect script and appending it to DIR so that rebuilds happen in the correct path. This worked for me in a local testing environment, but I'm not familiar with this codebase, so I don't know if this is an optimal solution. --------- Signed-off-by: Henry Schreiner Signed-off-by: Cristian Le Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Henry Schreiner Co-authored-by: Cristian Le --- .pre-commit-config.yaml | 2 +- src/scikit_build_core/build/_editable.py | 2 + src/scikit_build_core/build/wheel.py | 5 +- .../resources/_editable_redirect.py | 19 ++++++- tests/test_editable.py | 51 +++++++++++++++++++ tests/test_editable_redirect.py | 1 + tests/test_editable_unit.py | 1 + 7 files changed, 77 insertions(+), 4 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 4f886a13..268fcf7d 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -88,7 +88,7 @@ repos: - rich - setuptools-scm - tomli - - types-setuptools>=69.2 + - types-setuptools~=70.0 - repo: https://github.com/henryiii/check-sdist rev: "v1.0.0rc2" diff --git a/src/scikit_build_core/build/_editable.py b/src/scikit_build_core/build/_editable.py index 5cb1ae46..9a1cca1b 100644 --- a/src/scikit_build_core/build/_editable.py +++ b/src/scikit_build_core/build/_editable.py @@ -30,6 +30,7 @@ def editable_redirect( verbose: bool, build_options: Sequence[str], install_options: Sequence[str], + install_dir: str, ) -> str: """ Prepare the contents of the _editable_redirect.py file. @@ -46,6 +47,7 @@ def editable_redirect( verbose, build_options, install_options, + install_dir, ) arguments_str = ", ".join(repr(x) for x in arguments) editable_txt += f"\n\ninstall({arguments_str})\n" diff --git a/src/scikit_build_core/build/wheel.py b/src/scikit_build_core/build/wheel.py index 26106930..a389df04 100644 --- a/src/scikit_build_core/build/wheel.py +++ b/src/scikit_build_core/build/wheel.py @@ -58,7 +58,9 @@ def _make_editable( ) -> None: modules = mapping_to_modules(mapping, libdir) installed = libdir_to_installed(libdir) - + if settings.wheel.install_dir.startswith("/"): + msg = "Editable installs cannot rebuild an absolute wheel.install-dir. Use an override to change if needed." + raise AssertionError(msg) editable_txt = editable_redirect( modules=modules, installed=installed, @@ -67,6 +69,7 @@ def _make_editable( verbose=settings.editable.verbose, build_options=build_options, install_options=install_options, + install_dir=settings.wheel.install_dir, ) wheel.writestr( diff --git a/src/scikit_build_core/resources/_editable_redirect.py b/src/scikit_build_core/resources/_editable_redirect.py index af75519f..0c25c410 100644 --- a/src/scikit_build_core/resources/_editable_redirect.py +++ b/src/scikit_build_core/resources/_editable_redirect.py @@ -32,7 +32,8 @@ def __init__( verbose: bool, build_options: list[str], install_options: list[str], - dir: str = DIR, + dir: str, + install_dir: str, ) -> None: self.known_source_files = known_source_files self.known_wheel_files = known_wheel_files @@ -42,6 +43,8 @@ def __init__( self.build_options = build_options self.install_options = install_options self.dir = dir + self.install_dir = os.path.join(DIR, install_dir) + # Construct the __path__ of all resource files # I.e. the paths of all package-like objects submodule_search_locations: dict[str, set[str]] = {} @@ -136,7 +139,14 @@ def rebuild(self) -> None: result.check_returncode() result = subprocess.run( - ["cmake", "--install", ".", "--prefix", DIR, *self.install_options], + [ + "cmake", + "--install", + ".", + "--prefix", + self.install_dir, + *self.install_options, + ], cwd=self.path, stdout=sys.stderr if verbose else subprocess.PIPE, env=env, @@ -159,6 +169,7 @@ def install( verbose: bool = False, build_options: list[str] | None = None, install_options: list[str] | None = None, + install_dir: str = "", ) -> None: """ Install a meta path finder that redirects imports to the source files, and @@ -169,6 +180,8 @@ def install( :param path: The path to the build directory, or None :param verbose: Whether to print the cmake commands (also controlled by the SKBUILD_EDITABLE_VERBOSE environment variable) + :param install_dir: The wheel install directory override, if one was + specified """ sys.meta_path.insert( 0, @@ -180,5 +193,7 @@ def install( verbose, build_options or [], install_options or [], + DIR, + install_dir, ), ) diff --git a/tests/test_editable.py b/tests/test_editable.py index 6dcf3ff1..507ba12e 100644 --- a/tests/test_editable.py +++ b/tests/test_editable.py @@ -1,4 +1,5 @@ import sys +import textwrap from pathlib import Path import pytest @@ -105,3 +106,53 @@ def test_cython_pxd(monkeypatch, tmp_path, editable, editable_mode, isolated): *editable_flag, ".", ) + + +@pytest.mark.compile() +@pytest.mark.configure() +@pytest.mark.integration() +@pytest.mark.usefixtures("package_simplest_c") +def test_install_dir(isolated): + isolated.install("pip>=23") + isolated.install("scikit-build-core") + + settings_overrides = { + "build-dir": "build/{wheel_tag}", + "wheel.install-dir": "other_pkg", + "editable.rebuild": "true", + } + # Create a dummy other_pkg package to satisfy the import + other_pkg_src = Path("./src/other_pkg") + other_pkg_src.joinpath("simplest").mkdir(parents=True) + other_pkg_src.joinpath("__init__.py").write_text( + textwrap.dedent( + """ + from .simplest._module import square + """ + ) + ) + other_pkg_src.joinpath("simplest/__init__.py").touch() + + isolated.install( + "-v", + *[f"--config-settings={k}={v}" for k, v in settings_overrides.items()], + "--no-build-isolation", + "-e", + ".", + ) + + # Make sure the package is correctly installed in the subdirectory + other_pkg_path = isolated.platlib / "other_pkg" + c_module_glob = list(other_pkg_path.glob("simplest/_module*")) + assert len(c_module_glob) == 1 + c_module = c_module_glob[0] + assert c_module.exists() + # If `install-dir` was not taken into account it would install here + failed_c_module = other_pkg_path / "../simplest" / c_module.name + assert not failed_c_module.exists() + + # Run an import in order to re-trigger the rebuild and check paths again + out = isolated.execute("import other_pkg.simplest") + assert "Running cmake" in out + assert c_module.exists() + assert not failed_c_module.exists() diff --git a/tests/test_editable_redirect.py b/tests/test_editable_redirect.py index b646730d..6c672012 100644 --- a/tests/test_editable_redirect.py +++ b/tests/test_editable_redirect.py @@ -44,6 +44,7 @@ def test_editable_redirect(): build_options=[], install_options=[], dir=str(Path("/sitepackages")), + install_dir="", ) assert finder.submodule_search_locations == process_dict_set( diff --git a/tests/test_editable_unit.py b/tests/test_editable_unit.py index ab3e24b2..48584368 100644 --- a/tests/test_editable_unit.py +++ b/tests/test_editable_unit.py @@ -181,6 +181,7 @@ def test_navigate_editable_pkg(editable_package: EditablePackage, virtualenv: VE verbose=False, build_options=[], install_options=[], + install_dir="", ) site_packages.joinpath("_pkg_editable.py").write_text(editable_txt)