search

pypa / pyproject-hooks

A low-level library for calling build-backends in `pyproject.toml`-based project
https://pyproject-hooks.readthedocs.io/
MIT License
122 stars 49 forks source link

Improve backend loading from `backend-path` #165

Closed abravalheri closed 1 year ago

abravalheri commented 1 year ago

Closes #164 Closes #45

My understanding is that these changes are worth implementing (to increase robustness) even if pip improves its environment isolation.

Approach

Instead of changing sys.path and relying on importlib.import_module to do the right thing, this PR proposes to use importlib.machinery.PathFinder.find_spec to actively restrict the paths to be searched when looking for the backend.

Other details

Since the description of BackendUnavailable[^1] seems more appropriate than the description for BackendInvalid[^2] for the circumstances described in the #164, I modified (in a backwards compatible way) how the errors are reported. I think this makes sense with the rest of the alterations...

[^1]: docstring: Will be raised if the backend cannot be imported in the hook process. [^2]: docstring: Will be raised if the backend is invalid.

abravalheri commented 1 year ago

Hi @takluyver, I was wondering if you could review this PR (if you have the time) or share your thoughts about the issue.

I don't know if there would be time for a pyproject-hooks release before pip's next release (very unlikely, but who knows...).

abravalheri commented 1 year ago

@pradyunsg, I rebased the PR on top of the latest changes in main. Please let me know if any other changes are necessary.

abravalheri commented 1 year ago

Hello @pypa/pyproject-hooks-committers, I wonder if this direction for solving the issue is something that the project would be interested, or if it would be better for me to close the PR and assume that the issue would be handled with a different approach.

takluyver commented 1 year ago

I always find it tough to understand the various bits of custom import machinery, so I don't relish trying to understand either the problem or the solution here (and I'm too tired to tackle it right now, when I happen to have a few minutes to look at it).

I see you've added tests, and @FFY00 was happy with it, so I'll give people a few more days to make any comments, and if there are none, I'll trust you and merge this. Feel free to ping me again in a week or so if I forget (which is all too possible :wink: )

pradyunsg commented 1 year ago

Closes (potentially?) #45

This definitely closes that, no? I'll let you edit the description, but my reading is that it definitely does change this behaviour.

abravalheri commented 1 year ago

This definitely closes that, no? I'll let you edit the description, but my reading is that it definitely does change this behaviour.

Perfect, I updated the description. Thank you very much for the confirmation @pradyunsg.

abravalheri commented 1 year ago

I have updated the PR to take into consideration external feedback I received about PathFinder (i.e. to avoid using PathFinder with nested modules).

Luckily, importlib machinery is smart enough to find nested modules based on the parent's path. I copied the if "." in fullname trick from editables.

pradyunsg commented 1 year ago

Thanks @abravalheri! ^.^