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

Improve backend loading from backend-path #165

Merged
merged 14 commits into from
Aug 19, 2023
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions src/pyproject_hooks/_impl.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,12 @@ def read_json(path):
class BackendUnavailable(Exception):
"""Will be raised if the backend cannot be imported in the hook process."""

def __init__(self, traceback):
def __init__(self, traceback, message=None, backend_name=None, backend_path=None):
# Preserving arg order for the sake of API backward compatibility.
self.backend_name = backend_name
self.backend_path = backend_path
self.traceback = traceback
super().__init__(message or "Error while importing backend")


class BackendInvalid(Exception):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this exception, since this is no longer being caught and used?

Copy link
Contributor Author

@abravalheri abravalheri Aug 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for the review @pradyunsg.

I was considering that removing BackendInvalid would cause backward compatibility problems (e.g. it seems to be in __init__.py's __all__, some folks might consider that public API, so it might break someone's script if they do a from pyproject_hooks import BackendInvalid).

I am happy to remove the class if the project is OK with taking the (potential) backwards incompatibility. Alternative, also happy to change the docstring to "deprecated" or something like that. What is your take on that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added 7bb74e2 in response to this review comment.
But I am also happy to completely remove the class or do something different.

Just let me know and I will implement the change.

Copy link
Member

@pradyunsg pradyunsg Aug 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So... I don't see anyone using this on GitHub so it seems reasonably safe to remove.

We can bump the major version since this is backwards-incompatible (and version numbers are cheap), and then we don't have to bother with having dead code in the codebase that no one relies on. The fix from the users' perspective is relatively straightforward here too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems to be in __init__.py's __all__, some folks might consider that public API,

FWIW, I'd be surprised if anyone doesn't consider it public API. 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problems, in a318f8e, I removed the unused exception class.

In the case a deprecation cycle is preferred we can just revert this commit.

Expand Down Expand Up @@ -334,12 +338,11 @@ def _call_hook(self, hook_name, kwargs):
if data.get("unsupported"):
raise UnsupportedOperation(data.get("traceback", ""))
if data.get("no_backend"):
raise BackendUnavailable(data.get("traceback", ""))
if data.get("backend_invalid"):
raise BackendInvalid(
raise BackendUnavailable(
data.get("traceback", ""),
message=data.get("backend_error", ""),
backend_name=self.build_backend,
backend_path=self.backend_path,
message=data.get("backend_error", ""),
)
if data.get("hook_missing"):
raise HookMissing(data.get("missing_hook_name") or hook_name)
Expand Down
60 changes: 34 additions & 26 deletions src/pyproject_hooks/_in_process/_in_process.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import traceback
from glob import glob
from importlib import import_module
from importlib.machinery import PathFinder
from os.path import join as pjoin

# This file is run as a script, and `import wrappers` is not zip-safe, so we
Expand All @@ -40,15 +41,10 @@ def read_json(path):
class BackendUnavailable(Exception):
"""Raised if we cannot import the backend"""

def __init__(self, traceback):
self.traceback = traceback


class BackendInvalid(Exception):
"""Raised if the backend is invalid"""

def __init__(self, message):
def __init__(self, message, traceback=None):
super().__init__(message)
self.message = message
self.traceback = traceback


class HookMissing(Exception):
Expand All @@ -59,38 +55,52 @@ def __init__(self, hook_name=None):
self.hook_name = hook_name


def contained_in(filename, directory):
"""Test if a file is located within the given directory."""
filename = os.path.normcase(os.path.abspath(filename))
directory = os.path.normcase(os.path.abspath(directory))
return os.path.commonprefix([filename, directory]) == directory


def _build_backend():
"""Find and load the build backend"""
# Add in-tree backend directories to the front of sys.path.
backend_path = os.environ.get("_PYPROJECT_HOOKS_BACKEND_PATH")
ep = os.environ["_PYPROJECT_HOOKS_BUILD_BACKEND"]
mod_path, _, obj_path = ep.partition(":")

if backend_path:
# Ensure in-tree backend directories have the highest priority when importing.
extra_pathitems = backend_path.split(os.pathsep)
sys.path[:0] = extra_pathitems
sys.meta_path.insert(0, _BackendPathFinder(extra_pathitems, mod_path))

ep = os.environ["_PYPROJECT_HOOKS_BUILD_BACKEND"]
mod_path, _, obj_path = ep.partition(":")
try:
obj = import_module(mod_path)
except ImportError:
raise BackendUnavailable(traceback.format_exc())

if backend_path:
if not any(contained_in(obj.__file__, path) for path in extra_pathitems):
raise BackendInvalid("Backend was not loaded from backend-path")
msg = f"Cannot import {mod_path!r}"
raise BackendUnavailable(msg, traceback.format_exc())

if obj_path:
for path_part in obj_path.split("."):
obj = getattr(obj, path_part)
return obj


class _BackendPathFinder:
"""Implements the MetaPathFinder interface to locate modules in ``backend-path``.

Since the environment provided by the frontend can contain all sorts of
MetaPathFinders, the only way to ensure the backend is loaded from the
right place is to prepend our own.
"""

def __init__(self, backend_path, backend_module):
self.backend_path = backend_path
self.backend_module = backend_module

def find_spec(self, fullname, _path, _target=None):
# Ignore other items in _path or sys.path and use backend_path instead:
spec = PathFinder.find_spec(fullname, path=self.backend_path)
if spec is None and fullname == self.backend_module:
# According to the spec, the backend MUST be loaded from backend-path.
# Therefore, we can halt the import machinery and raise a clean error.
msg = f"Cannot find module {self.backend_module!r} in {self.backend_path!r}"
raise BackendUnavailable(msg)
return spec


def _supported_features():
"""Return the list of options features supported by the backend.

Expand Down Expand Up @@ -342,8 +352,6 @@ def main():
except BackendUnavailable as e:
json_out["no_backend"] = True
json_out["traceback"] = e.traceback
except BackendInvalid as e:
json_out["backend_invalid"] = True
json_out["backend_error"] = e.message
except GotUnsupportedOperation as e:
json_out["unsupported"] = True
Expand Down
4 changes: 3 additions & 1 deletion tests/test_call_hooks.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,10 @@ def get_hooks(pkg, **kwargs):
def test_missing_backend_gives_exception():
hooks = get_hooks("pkg1")
with modified_env({"PYTHONPATH": ""}):
with pytest.raises(BackendUnavailable):
msg = "Cannot import 'buildsys'"
with pytest.raises(BackendUnavailable, match=msg) as exc:
hooks.get_requires_for_build_wheel({})
assert exc.value.backend_name == "buildsys"


def test_get_requires_for_build_wheel():
Expand Down
50 changes: 48 additions & 2 deletions tests/test_inplace_hooks.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
from inspect import cleandoc
from os.path import abspath, dirname
from os.path import join as pjoin
from pathlib import Path

import pytest
from testpath import modified_env
from testpath.tempdir import TemporaryDirectory

from pyproject_hooks import BackendInvalid, BuildBackendHookCaller
from pyproject_hooks import BackendUnavailable, BuildBackendHookCaller
from tests.compat import tomllib

SAMPLES_DIR = pjoin(dirname(abspath(__file__)), "samples")
Expand Down Expand Up @@ -59,5 +62,48 @@ def test_intree_backend():
def test_intree_backend_not_in_path():
hooks = get_hooks("pkg_intree", backend="buildsys")
with modified_env({"PYTHONPATH": BUILDSYS_PKGS}):
with pytest.raises(BackendInvalid):
msg = "Cannot find module 'buildsys' in .*pkg_intree.*backend"
with pytest.raises(BackendUnavailable, match=msg):
hooks.get_requires_for_build_sdist({})


def test_intree_backend_loaded_from_correct_backend_path():
"""
PEP 517 establishes that the backend code should be loaded from ``backend-path``,
and recognizes that not always the environment isolation is perfect
(e.g. it explicitly mentions ``--system-site-packages``).
Therefore, even in a situation where a third-party ``MetaPathFinder`` has
precedence over ``importlib.machinery.PathFinder``, the backend should
still be loaded from ``backend-path``.
"""
hooks = get_hooks("pkg_intree", backend="intree_backend")
with TemporaryDirectory() as tmp:
invalid = Path(tmp, ".invalid", "intree_backend.py")
invalid.parent.mkdir()
invalid.write_text("raise ImportError('Do not import')", encoding="utf-8")
install_finder_with_sitecustomize(tmp, {"intree_backend": str(invalid)})
with modified_env({"PYTHONPATH": tmp}): # Override `sitecustomize`.
res = hooks.get_requires_for_build_sdist({})
assert res == ["intree_backend_called"]


def install_finder_with_sitecustomize(directory, mapping):
finder = f"""
import sys
from importlib.util import spec_from_file_location

MAPPING = {mapping!r}

class _Finder: # MetaPathFinder
@classmethod
def find_spec(cls, fullname, path=None, target=None):
if fullname in MAPPING:
return spec_from_file_location(fullname, MAPPING[fullname])

def install():
if not any(finder == _Finder for finder in sys.meta_path):
sys.meta_path.insert(0, _Finder)
"""
sitecustomize = "import _test_finder_; _test_finder_.install()"
Path(directory, "_test_finder_.py").write_text(cleandoc(finder), encoding="utf-8")
Path(directory, "sitecustomize.py").write_text(sitecustomize, encoding="utf-8")