From 20c91275dd87f5c88c3f5ef0b8e4c9a39f70cf60 Mon Sep 17 00:00:00 2001 From: Philip Salvaggio Date: Mon, 19 Aug 2024 22:40:12 -0400 Subject: [PATCH 01/11] Editable installs now respect the value of wheel.install-dir --- src/scikit_build_core/build/_editable.py | 3 ++- src/scikit_build_core/build/wheel.py | 1 + .../resources/_editable_redirect.py | 12 ++++++++++-- 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/scikit_build_core/build/_editable.py b/src/scikit_build_core/build/_editable.py index 5cb1ae46..d6969880 100644 --- a/src/scikit_build_core/build/_editable.py +++ b/src/scikit_build_core/build/_editable.py @@ -30,11 +30,11 @@ def editable_redirect( verbose: bool, build_options: Sequence[str], install_options: Sequence[str], + install_dir: str | None, ) -> str: """ Prepare the contents of the _editable_redirect.py file. """ - editable_py = resources / "_editable_redirect.py" editable_txt: str = editable_py.read_text(encoding="utf-8") @@ -46,6 +46,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..8f46208f 100644 --- a/src/scikit_build_core/build/wheel.py +++ b/src/scikit_build_core/build/wheel.py @@ -67,6 +67,7 @@ def _make_editable( verbose=settings.editable.verbose, build_options=build_options, install_options=install_options, + install_dir=settings.wheel.install_dir or None, ) wheel.writestr( diff --git a/src/scikit_build_core/resources/_editable_redirect.py b/src/scikit_build_core/resources/_editable_redirect.py index af75519f..a4315864 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,7 @@ def __init__( self.build_options = build_options self.install_options = install_options self.dir = dir + self.install_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 +138,8 @@ 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 +162,7 @@ def install( verbose: bool = False, build_options: list[str] | None = None, install_options: list[str] | None = None, + install_dir: str | None = None, ) -> None: """ Install a meta path finder that redirects imports to the source files, and @@ -169,6 +173,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 +186,7 @@ def install( verbose, build_options or [], install_options or [], + DIR, + os.path.join(DIR, install_dir) if install_dir else DIR, ), ) From a9e7c7ec8455add0796915b6abe808a7eb9a30b9 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 20 Aug 2024 02:49:55 +0000 Subject: [PATCH 02/11] style: pre-commit fixes --- src/scikit_build_core/resources/_editable_redirect.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/scikit_build_core/resources/_editable_redirect.py b/src/scikit_build_core/resources/_editable_redirect.py index a4315864..c260aaa6 100644 --- a/src/scikit_build_core/resources/_editable_redirect.py +++ b/src/scikit_build_core/resources/_editable_redirect.py @@ -138,8 +138,14 @@ def rebuild(self) -> None: result.check_returncode() result = subprocess.run( - ["cmake", "--install", ".", "--prefix", self.install_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, From ca7504d5bd0e52ec82d205ba8137456903a1e0f2 Mon Sep 17 00:00:00 2001 From: Philip Salvaggio Date: Mon, 19 Aug 2024 23:20:21 -0400 Subject: [PATCH 03/11] Adjusted unit tests to reflect additional parameter for wheel.install_dir --- src/scikit_build_core/build/_editable.py | 1 + tests/test_editable_redirect.py | 1 + tests/test_editable_unit.py | 1 + 3 files changed, 3 insertions(+) diff --git a/src/scikit_build_core/build/_editable.py b/src/scikit_build_core/build/_editable.py index d6969880..47d55450 100644 --- a/src/scikit_build_core/build/_editable.py +++ b/src/scikit_build_core/build/_editable.py @@ -35,6 +35,7 @@ def editable_redirect( """ Prepare the contents of the _editable_redirect.py file. """ + editable_py = resources / "_editable_redirect.py" editable_txt: str = editable_py.read_text(encoding="utf-8") diff --git a/tests/test_editable_redirect.py b/tests/test_editable_redirect.py index b646730d..61c5ae2e 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=None ) assert finder.submodule_search_locations == process_dict_set( diff --git a/tests/test_editable_unit.py b/tests/test_editable_unit.py index ab3e24b2..0a2421a3 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=None, ) site_packages.joinpath("_pkg_editable.py").write_text(editable_txt) From e6c87a27193c0b610ff32790a47b6d4c05017a3b Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 20 Aug 2024 03:20:43 +0000 Subject: [PATCH 04/11] style: pre-commit fixes --- tests/test_editable_redirect.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_editable_redirect.py b/tests/test_editable_redirect.py index 61c5ae2e..127b47e4 100644 --- a/tests/test_editable_redirect.py +++ b/tests/test_editable_redirect.py @@ -44,7 +44,7 @@ def test_editable_redirect(): build_options=[], install_options=[], dir=str(Path("/sitepackages")), - install_dir=None + install_dir=None, ) assert finder.submodule_search_locations == process_dict_set( From 1c2fd74cae9212663cbd360c9e3b58c22e593e83 Mon Sep 17 00:00:00 2001 From: Philip Salvaggio Date: Mon, 19 Aug 2024 23:34:47 -0400 Subject: [PATCH 05/11] Allowed install_dir to be None to fix mypy error --- src/scikit_build_core/resources/_editable_redirect.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/scikit_build_core/resources/_editable_redirect.py b/src/scikit_build_core/resources/_editable_redirect.py index c260aaa6..8a1396fc 100644 --- a/src/scikit_build_core/resources/_editable_redirect.py +++ b/src/scikit_build_core/resources/_editable_redirect.py @@ -33,7 +33,7 @@ def __init__( build_options: list[str], install_options: list[str], dir: str, - install_dir: str, + install_dir: str | None, ) -> None: self.known_source_files = known_source_files self.known_wheel_files = known_wheel_files @@ -43,7 +43,7 @@ def __init__( self.build_options = build_options self.install_options = install_options self.dir = dir - self.install_dir = install_dir + self.install_dir = install_dir or DIR # Construct the __path__ of all resource files # I.e. the paths of all package-like objects submodule_search_locations: dict[str, set[str]] = {} From 8bcf84ee479b737cb77e4b3bed29d042f75b7f88 Mon Sep 17 00:00:00 2001 From: Philip Salvaggio Date: Tue, 20 Aug 2024 07:29:06 -0400 Subject: [PATCH 06/11] Propagated the empty string default for install_dir down to the editable redirect script --- src/scikit_build_core/build/_editable.py | 2 +- src/scikit_build_core/build/wheel.py | 2 +- .../resources/_editable_redirect.py | 12 ++++++++---- tests/test_editable_redirect.py | 2 +- tests/test_editable_unit.py | 2 +- 5 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/scikit_build_core/build/_editable.py b/src/scikit_build_core/build/_editable.py index 47d55450..9a1cca1b 100644 --- a/src/scikit_build_core/build/_editable.py +++ b/src/scikit_build_core/build/_editable.py @@ -30,7 +30,7 @@ def editable_redirect( verbose: bool, build_options: Sequence[str], install_options: Sequence[str], - install_dir: str | None, + install_dir: str, ) -> str: """ Prepare the contents of the _editable_redirect.py file. diff --git a/src/scikit_build_core/build/wheel.py b/src/scikit_build_core/build/wheel.py index 8f46208f..cc8b29f6 100644 --- a/src/scikit_build_core/build/wheel.py +++ b/src/scikit_build_core/build/wheel.py @@ -67,7 +67,7 @@ def _make_editable( verbose=settings.editable.verbose, build_options=build_options, install_options=install_options, - install_dir=settings.wheel.install_dir or None, + 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 8a1396fc..cac9b8dc 100644 --- a/src/scikit_build_core/resources/_editable_redirect.py +++ b/src/scikit_build_core/resources/_editable_redirect.py @@ -33,7 +33,7 @@ def __init__( build_options: list[str], install_options: list[str], dir: str, - install_dir: str | None, + install_dir: str, ) -> None: self.known_source_files = known_source_files self.known_wheel_files = known_wheel_files @@ -43,7 +43,11 @@ def __init__( self.build_options = build_options self.install_options = install_options self.dir = dir - self.install_dir = install_dir or DIR + self.install_dir = ( + install_dir + if os.path.isabs(install_dir) + else 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]] = {} @@ -168,7 +172,7 @@ def install( verbose: bool = False, build_options: list[str] | None = None, install_options: list[str] | None = None, - install_dir: str | None = None, + install_dir: str = "", ) -> None: """ Install a meta path finder that redirects imports to the source files, and @@ -193,6 +197,6 @@ def install( build_options or [], install_options or [], DIR, - os.path.join(DIR, install_dir) if install_dir else DIR, + install_dir, ), ) diff --git a/tests/test_editable_redirect.py b/tests/test_editable_redirect.py index 127b47e4..6c672012 100644 --- a/tests/test_editable_redirect.py +++ b/tests/test_editable_redirect.py @@ -44,7 +44,7 @@ def test_editable_redirect(): build_options=[], install_options=[], dir=str(Path("/sitepackages")), - install_dir=None, + 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 0a2421a3..48584368 100644 --- a/tests/test_editable_unit.py +++ b/tests/test_editable_unit.py @@ -181,7 +181,7 @@ def test_navigate_editable_pkg(editable_package: EditablePackage, virtualenv: VE verbose=False, build_options=[], install_options=[], - install_dir=None, + install_dir="", ) site_packages.joinpath("_pkg_editable.py").write_text(editable_txt) From 4cfafe2b8419b02ec92fed69c70396d413688721 Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Tue, 20 Aug 2024 11:34:14 -0400 Subject: [PATCH 07/11] fix: path will always be relative --- src/scikit_build_core/resources/_editable_redirect.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/scikit_build_core/resources/_editable_redirect.py b/src/scikit_build_core/resources/_editable_redirect.py index cac9b8dc..0c25c410 100644 --- a/src/scikit_build_core/resources/_editable_redirect.py +++ b/src/scikit_build_core/resources/_editable_redirect.py @@ -43,11 +43,8 @@ def __init__( self.build_options = build_options self.install_options = install_options self.dir = dir - self.install_dir = ( - install_dir - if os.path.isabs(install_dir) - else os.path.join(DIR, install_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]] = {} From 2bed64349edbae0e190b3dde0babf04a76c648ff Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Tue, 20 Aug 2024 11:38:51 -0400 Subject: [PATCH 08/11] fix: error if editable install gets a absolute path --- src/scikit_build_core/build/wheel.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/scikit_build_core/build/wheel.py b/src/scikit_build_core/build/wheel.py index cc8b29f6..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, From f53fba49e474f18a6a66e28f69fab45c7162d2d4 Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Tue, 20 Aug 2024 12:05:25 -0400 Subject: [PATCH 09/11] chore: limit types-setuptools for now Signed-off-by: Henry Schreiner --- .pre-commit-config.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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" From 3a178f9e288b14cb1c3fb6030f1c4a1414d466cc Mon Sep 17 00:00:00 2001 From: Cristian Le Date: Tue, 20 Aug 2024 18:15:21 +0200 Subject: [PATCH 10/11] Add test for editable install_dir Signed-off-by: Cristian Le --- tests/test_editable.py | 48 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/tests/test_editable.py b/tests/test_editable.py index 6dcf3ff1..765cb11d 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,50 @@ 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": "sub_pkg", + "editable.rebuild": "true", + } + # Create a dummy _module to satisfy the import + Path("./src/simplest/_module.py").write_text( + textwrap.dedent( + """ + def square(__x: float, __y: float) -> float: + pass + """ + ) + ) + + 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 + sub_pkg_path = isolated.platlib / "sub_pkg" + c_module_glob = list(sub_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 = sub_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 + isolated.execute("import simplest") + assert c_module.exists() + assert not failed_c_module.exists() From ff924dba4b6292f6d1bc6ed4ac530c7311f964b4 Mon Sep 17 00:00:00 2001 From: Cristian Le Date: Tue, 20 Aug 2024 18:30:45 +0200 Subject: [PATCH 11/11] Change test to import the C module Also check that CMake is indeed re-run Signed-off-by: Cristian Le --- tests/test_editable.py | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/tests/test_editable.py b/tests/test_editable.py index 765cb11d..507ba12e 100644 --- a/tests/test_editable.py +++ b/tests/test_editable.py @@ -118,18 +118,20 @@ def test_install_dir(isolated): settings_overrides = { "build-dir": "build/{wheel_tag}", - "wheel.install-dir": "sub_pkg", + "wheel.install-dir": "other_pkg", "editable.rebuild": "true", } - # Create a dummy _module to satisfy the import - Path("./src/simplest/_module.py").write_text( + # 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( """ - def square(__x: float, __y: float) -> float: - pass + from .simplest._module import square """ ) ) + other_pkg_src.joinpath("simplest/__init__.py").touch() isolated.install( "-v", @@ -140,16 +142,17 @@ def square(__x: float, __y: float) -> float: ) # Make sure the package is correctly installed in the subdirectory - sub_pkg_path = isolated.platlib / "sub_pkg" - c_module_glob = list(sub_pkg_path.glob("simplest/_module*")) + 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 = sub_pkg_path / "../simplest" / c_module.name + 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 - isolated.execute("import simplest") + out = isolated.execute("import other_pkg.simplest") + assert "Running cmake" in out assert c_module.exists() assert not failed_c_module.exists()