pfmoore / editables

MIT License
39 stars 11 forks source link

Editable installs with redirects masked by natural installs later on sys.path #37

Open jaraco opened 3 months ago

jaraco commented 3 months ago

While I was reviewing the implementation, I noticed that the Redirector always installs itself at the end of sys.meta_path, meaning it defers to all other MetaPathFinders and only has an effect when those finders fail to resolve a name.

But if you consider the scenario where a module/package is naturally installed in one site dir and editable-installed in another site dir, one might expect that the name would resolve based on the order that the site dirs appear in sys.path. After all, that's the expectation for naturally-installed modules/packages.

This situation arises when multiple site dirs are present, such as when using py -m venv --system-site-packages or with tools like pip-run or simply manually creating multiple site dirs on sys.path. Imagine, for example:

 draft @ git clone -q gh://pypa/sampleproject
 draft @ cd sampleproject
 sampleproject main @ py -m pip install -q --no-deps --break-system-packages sampleproject
 sampleproject main @ py -m venv .venv --system-site-packages
 sampleproject main @ .venv/bin/python -c "import sample; print(sample.__file__)"
/opt/homebrew/lib/python3.12/site-packages/sample/__init__.py
 sampleproject main @ .venv/bin/pip install -e .
Obtaining file:///Users/jaraco/draft/sampleproject
  Installing build dependencies ... done
  Checking if build backend supports build_editable ... done
  Getting requirements to build editable ... done
  Installing backend dependencies ... done
  Preparing editable metadata (pyproject.toml) ... done
Collecting peppercorn (from sampleproject==3.0.0)
  Using cached peppercorn-0.6-py3-none-any.whl.metadata (3.4 kB)
Using cached peppercorn-0.6-py3-none-any.whl (4.8 kB)
Building wheels for collected packages: sampleproject
  Building editable for sampleproject (pyproject.toml) ... done
  Created wheel for sampleproject: filename=sampleproject-3.0.0-0.editable-py3-none-any.whl size=4277 sha256=eaf0f18d3dcbeac423913c0318abfd47a8fed2f8d6f075cc77b3ab4dec4d6ecc
  Stored in directory: /private/var/folders/f2/2plv6q2n7l932m2x004jlw340000gn/T/pip-ephem-wheel-cache-qap3v6sb/wheels/a5/93/db/64ecb082ba5bb19a7c78e56450351758b11f7d93d80e1c40cd
Successfully built sampleproject
Installing collected packages: peppercorn, sampleproject
  Attempting uninstall: sampleproject
    Found existing installation: sampleproject 3.0.0
    Not uninstalling sampleproject at /opt/homebrew/lib/python3.12/site-packages, outside environment /Users/jaraco/draft/sampleproject/.venv
    Can't uninstall 'sampleproject'. No files were found to uninstall.
Successfully installed peppercorn-0.6 sampleproject-3.0.0
 sampleproject main @ .venv/bin/python -c "import sample; print(sample.__file__)"
/Users/jaraco/draft/sampleproject/src/sample/__init__.py

In this scenario, the user reasonably expects for the editable-installed version to take precedence, and in the case above, it works as expected because the editable link is a simple .pth file. If, however, the editable install had been made possible by editables.redirector, the system site packages will always take precedence.

This issue is illustrated by two tests added in jaraco/editables@058c85c.

Altering the redirector install to give precedence to the RedirectingFinder:

diff --git a/src/editables/redirector.py b/src/editables/redirector.py
index 7bdef59..3af055a 100644
--- a/src/editables/redirector.py
+++ b/src/editables/redirector.py
@@ -36,7 +36,7 @@ class RedirectingFinder(importlib.abc.MetaPathFinder):
             if f == cls:
                 break
         else:
-            sys.meta_path.append(cls)
+            sys.meta_path.insert(0, cls)

     @classmethod
     def invalidate_caches(cls) -> None:

causes the expectation to flip.

I'm not sure if it's even possible to honor this expectation with a MetaPathFinder, given that the sys.path ordering is honored in the built-in PathFinder.

What RedirectingFinder really wants is a hook into PathFinder to extend the behavior when searching a given path, so its behavior only takes precedence when the considering the path where the package was editable installed.

pfmoore commented 3 months ago

I'm not aware of this hitting any real world users, so I consider this a theoretical problem until it's demonstrated otherwise - at which point, my initial inclination would be to offer the user advice on alternative ways to solve their problem, rather than treating this as an issue with how editable installs work.

Yes, it's a limitation of the metapath approach. But all editable installation methods have limitations - it simply isn't possible to create something that handles every possible use case. What matters to me is effectively handling the majority of typical use cases, providing a small but meaningful level of additional capability compared to the traditional .pth file approach. But I would expect build backends using this library to choose their implementation approach based on the project structure - so the metapath (the .map method) would only be used when simpler methods like .add_to_path and .add_to_subpackage are insufficient.

As a side note, for really advanced or specialised use cases, there's no actual need to use an editable install anyway. It would be perfectly reasonable for someone to write a tool that injected a project directory into the import machinery directly - either ahead of time with something like symlinks, or at runtime via an install_project(project_path) call that sets up path hooks however makes sense. I don't know of any realistic cases where this type of complexity would be needed, but it's available for cases where the "editable install" approach is insufficient.

jaraco commented 3 months ago

I'm not aware of this hitting any real world users, so I consider this a theoretical problem...

I'm pretty sure I'm going to hit this as a real-world user. In the Coherent Software Development System that I'm developing, coherent.test is used to orchestrate the test run, i.e.

python -m coherent.test

The tooling then uses pip-run to install the project under test and its test dependencies before running the tests. Once the system has editable support, I'd like to use the editable install for installing . so that there's just one copy of the files under test.

Under the current implementation of the redirector (the likes of which is needed for the essential layout), the editable install will be masked by any packages in the install of coherent.test making coherent.test and its dependencies untestable in this regime.

As a side note, for really advanced or specialised use cases, there's no actual need to use an editable install anyway.

That's a good point - the tooling could bypass the packaging standards and provide its own machinery. My hope was to re-use the existing and established constructs where possible, but we may deem it not possible, or at least not supported by editables.

jaraco commented 3 months ago

What matters to me is effectively handling the majority of typical use cases, providing a small but meaningful level of additional capability compared to the traditional .pth file approach.

In this case, does it make sense to put the meta path finder ahead of the path finder so that an editable install always has precedence over non-editable installs?

In 4ad975a, I propose that change.

pfmoore commented 3 months ago

I'm pretty sure I'm going to hit this as a real-world user.

To be clear here, the users of editables are build backend developers. I'm not clear from your description whether this Coherent system is intended to be tied to its own build backend, or if it's intended to work with arbitrary build backends (in which case, discussing the issue here is not going to address the problem, as there's no requirement for backends to use editables - and in particular, I know that setuptools doesn't, because they wanted to support use cases that I don't consider a priority).

In this case, does it make sense to put the meta path finder ahead of the path finder so that an editable install always has precedence over non-editable installs?

I'm honestly not sure. The existing precedence was chosen deliberately (I remember thinking about it) but I don't recall the exact reasoning, and unfortunately I don't think I documented it. So I'll need to spend some time working out what the logic was, and how to decide between the trade-offs. In particular, I don't want to break existing users of editables who might rely on the current behaviour, so it might need some sort of transition.

jaraco commented 3 months ago

I'm not clear from your description whether this Coherent system is intended to be tied to its own build backend, or if it's intended to work with arbitrary build backends

It's the former. coherent.build is the build backend for the Coherent System (and also the core tooling for implementing the system).

pfmoore commented 3 months ago

I guess one solution for you is to simply implement your own editable install hook, that makes the trade-offs that suit your requirements. That was always the intention for editable installs, the protocol is simple enough that backends can implement their own (that's one reason I'm so insistent on keeping this project simple - I explicitly don't want backends to think that they have to use it and can't build their own implementation).

jaraco commented 3 months ago

I guess one solution for you is to simply implement your own editable install hook,

That's definitely an option, and something I wanted to consider after making sure there isn't already a solution in the ecosystem on which to rely.