Improve backend loading from `backend-path` #165

abravalheri · 2023-02-22T23:20:02Z

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¹ seems more appropriate than the description for BackendInvalid² 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...

docstring: Will be raised if the backend cannot be imported in the hook process. ↩
docstring: Will be raised if the backend is invalid. ↩

FFY00 · 2023-02-23T22:39:35Z

src/pyproject_hooks/_in_process/_in_process.py

+        spec = _find_spec_in_path(module_name, pathitems)
+        module = module_from_spec(spec)
+        sys.modules[module_name] = module
+        spec.loader.exec_module(module)


This will execute the module with sys.meta_path unmodified. Even if you fixed loading this particular module from the right place, modules it imports during execution will be found by the loaders in sys.meta_path, which is even more dangerous than loading the specified backend module from the wrong place IMO.

# Parent packages need to be imported to ensure everything comes from pathitems.

Exactly, but children and packages with a different top-level name too!

IMO the correct way to handle this is to simply prepend your PathFinder to sys.meta_path while we perform the build, and just import as normal, instead of this custom import hack 😅

Thank you very much for the review, Filipe.

In f14e5df I tried to implement the suggested approach. Please let me know if I misunderstood your comment (I can always revert and try something different).

FFY00

Looks good, thanks @abravalheri!

abravalheri · 2023-03-08T00:18:57Z

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...).

Previously `_in_process` would only modify `sys.path`, use `importlib.import_module` and hope the backend is loaded from the right path. This would lead to misleading error messages in some cases when the environment isolation is not perfect. This change modifies the behavior by using `importlib.machinery.PathFinder.find_spec` to locate the backend module within a set of path entries.

… when error regards import problems.

This should address review comments and increase the robustness of the solution.

abravalheri · 2023-04-21T14:36:01Z

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

abravalheri · 2023-08-02T16:58:08Z

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 · 2023-08-05T20:15:05Z

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 😉 )

pradyunsg · 2023-07-15T12:20:09Z

src/pyproject_hooks/_impl.py

        self.traceback = traceback
+        super().__init__(message or "Error while importing backend")


 class BackendInvalid(Exception):


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

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?

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.

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.

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. 😅

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

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

pradyunsg · 2023-08-06T14:44:10Z

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 · 2023-08-06T15:22:44Z

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 · 2023-08-18T13:05:01Z

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 · 2023-08-19T00:26:04Z

Thanks @abravalheri! ^.^

See: pypa/pyproject-hooks#165

abravalheri force-pushed the improve-backend-path branch from caa16c6 to 2219922 Compare February 22, 2023 23:26

abravalheri marked this pull request as ready for review February 22, 2023 23:35

abravalheri mentioned this pull request Feb 23, 2023

pip fails to build project from backend-path when MetaPathFinder is present and installing from VCS url pypa/pip#11812

Open

1 task

abravalheri changed the title ~~Improve backend loading from backend-path~~ Improve/fix? backend loading from backend-path Feb 23, 2023

FFY00 suggested changes Feb 23, 2023

View reviewed changes

FFY00 approved these changes Mar 3, 2023

View reviewed changes

abravalheri added 9 commits April 21, 2023 15:19

Test backend is loaded from backend-path

a2f302a

Unify usages of BackedInvalid with BackendUnavailable

7a01590

… when error regards import problems.

Fix path sep error when testing on windows

bfcdac1

Improve explanation in code comment

148b2aa

Ensure backend info is available in exception

5726c39

Replace custom import sequence with MetaPathFinder

4790b2b

This should address review comments and increase the robustness of the solution.

Improve test docstring for intree backend

dcdedc9

Replace comment with a more appropriate description

0177e3d

abravalheri force-pushed the improve-backend-path branch from 4224f9b to 0177e3d Compare April 21, 2023 14:27

pradyunsg reviewed Aug 6, 2023

View reviewed changes

Mark BackendInvalid as no longer used/deprecated

7bb74e2

pradyunsg changed the title ~~Improve/fix? backend loading from backend-path~~ Improve backend loading from backend-path Aug 8, 2023

abravalheri added 4 commits August 8, 2023 16:06

Remove unused BackendInvalid

a318f8e

Test nested intree backend

32c6af3

Add negative tests for nested intree backends

19023eb

Avoid passing nested modules to PathFinder

89722cb

pradyunsg approved these changes Aug 19, 2023

View reviewed changes

pradyunsg merged commit 084b02e into pypa:main Aug 19, 2023
18 checks passed

abravalheri deleted the improve-backend-path branch August 19, 2023 00:29

takluyver mentioned this pull request Apr 24, 2024

Prepare v1.1 #188

Merged

This was referenced Apr 29, 2024

Release 1.1: Regression breaks setuptools builds #192

Open

Fix path pollution again #193

Merged

jsirois added a commit to jsirois/pex that referenced this pull request May 10, 2024

Work around issue caused by pyproject-hooks 1.1.0.

b10c96d

See: pypa/pyproject-hooks#165

abravalheri mentioned this pull request Jul 11, 2024

Directory with an empty pyproject.toml files is built into an sdist+wheel by setuptools 64 pypa/setuptools#3511

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve backend loading from `backend-path` #165

Improve backend loading from `backend-path` #165

abravalheri commented Feb 22, 2023 •

edited

Loading

FFY00 Feb 23, 2023

abravalheri Feb 24, 2023

FFY00 left a comment

abravalheri commented Mar 8, 2023

abravalheri commented Apr 21, 2023

abravalheri commented Aug 2, 2023

takluyver commented Aug 5, 2023

pradyunsg Jul 15, 2023

abravalheri Aug 6, 2023 •

edited

Loading

abravalheri Aug 7, 2023

pradyunsg Aug 8, 2023 •

edited

Loading

pradyunsg Aug 8, 2023

abravalheri Aug 8, 2023

pradyunsg commented Aug 6, 2023

abravalheri commented Aug 6, 2023

abravalheri commented Aug 18, 2023 •

edited

Loading

pradyunsg commented Aug 19, 2023

Improve backend loading from backend-path #165

Improve backend loading from backend-path #165

Conversation

abravalheri commented Feb 22, 2023 • edited Loading

Approach

Other details

Footnotes

FFY00 Feb 23, 2023

Choose a reason for hiding this comment

abravalheri Feb 24, 2023

Choose a reason for hiding this comment

FFY00 left a comment

Choose a reason for hiding this comment

abravalheri commented Mar 8, 2023

abravalheri commented Apr 21, 2023

abravalheri commented Aug 2, 2023

takluyver commented Aug 5, 2023

pradyunsg Jul 15, 2023

Choose a reason for hiding this comment

abravalheri Aug 6, 2023 • edited Loading

Choose a reason for hiding this comment

abravalheri Aug 7, 2023

Choose a reason for hiding this comment

pradyunsg Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

pradyunsg Aug 8, 2023

Choose a reason for hiding this comment

abravalheri Aug 8, 2023

Choose a reason for hiding this comment

pradyunsg commented Aug 6, 2023

abravalheri commented Aug 6, 2023

abravalheri commented Aug 18, 2023 • edited Loading

pradyunsg commented Aug 19, 2023

Improve backend loading from `backend-path` #165

Improve backend loading from `backend-path` #165

abravalheri commented Feb 22, 2023 •

edited

Loading

abravalheri Aug 6, 2023 •

edited

Loading

pradyunsg Aug 8, 2023 •

edited

Loading

abravalheri commented Aug 18, 2023 •

edited

Loading