Supply Distribution.locate_file, honoring the abstract method.

[jaraco]

Fixes #11684.

[jaraco]

I supply this proposed change as an illustration of what might address the issue. I'd prefer if someone from pip would contribute the rest of it (or replace it with their own).

[uranusjr]

How would this function make sense for a zip file? The documentation is sparse on what exactly is expected, and we obviously can’t return a pathlib.Path.

[pfmoore]

We still need some indication as to how we're supposed to implement the required method. From a brief look at the documentation, it seems that it would be quite a lot of work given that we clearly don't call anything that doesn't use the newly-required method...

Simply raising an error on the missing methods is a possibility, I guess, and that's what this PR does, but

1. This comment suggests it's not intended as the correct solution, and
2. It feels like we'd be subverting the intention of the change to make the methods required.

Advice on how we should proceed, and ideally a PR that you do consider complete and correct, would be appreciated.

[jaraco]

1. This comment suggests it's not intended as the correct solution, and

I'm unsure if it's the correct solution or not. I'm unaware of the internals of pip so am unsure why these Distribution objects exist or what users expect from them.

I'll explore in the bug.

[pfmoore]

I'm unaware of the internals of pip so am unsure why these Distribution objects exist or what users expect from them.

But isn't the point here that the stdlib is enforcing the implementation of the abstract methods? So what pip, or pip's users, expect from the objects is irrelevant - the stdlib is now requiring that we implement the method(s), so we have no choice. What we're asking is what constitutes (in the stdlib's eyes) a valid implementation. Ideally, that should be in the stdlib documentation, but to be blunt, the documentation is fairly unhelpful - all it says is "Given a path to a file in this distribution, return a SimplePath to it." That suggests that we have to implement some form of SimplePath subclass, in spite of the fact that currently we demonstrably execute no code paths that care about, or even call, locate_file.

If it's acceptable to simply raise NotImplementedError from the method, then I'd say that should be documented in the API documentation, along with a description of the implications of doing so (what importlib.metadata functions will fail if that method raises an error, for example). With that information, we could reasonably judge whether it's OK for pip to do that. And as I say, I'm pretty certain we'd say that it is OK, simply because it's worked until now - although this change in CPython is evidence that we'd have been wrong in that assumption, hence my concern about simply making the same assumption again.

To put this briefly: CPython changed the rules, I'd like them¹ to tell us what the new rules are, so we don't have to just guess again...

Footnotes

1. More precisely, you, as the CPython core dev expert on importlib.metadata 🙂 ↩

[jaraco]

CPython changed the rules

As the implementer, I can state with authority that it was always the intention that these methods were abstract and were expected by a subclass to implement them. It was probably an artifact of the Python 2.7 compatibility that the class itself wasn't an ABCMeta so lacked the enforcement in the API. Still, the method was always declared as an abstractmethod (IIRC). That said, you're right that the implementation is shifting to now honor the original intention that was missed and closing the gaps that have emerged since.

[wimglenn]

Is there any blocker on merging this for now to mitigate the problem? I like to use PYTHONWARNINGS=error sometimes in CI, to escalate warnings to exceptions, but can't install anything with pip in such an environment due to this issue.

[ichard26]

Raising an error in locate_file() is a valid implementation as per the importlib.metadata documentation.

[sbidoul]

@jaraco can you add type annotations on the new method to fix the mypy error?

[ichard26]

To be clear, there is an example Distribution subclass that defines a locate_file method which does nothing but error out.

class DatabaseDistribution(importlib.metadata.Distribution):
    def __init__(self, record):
        self.record = record

    def read_text(self, filename):
        """
        Read a file like "METADATA" for the current distribution.
        """
        if filename == "METADATA":
            return f"""Name: {self.record.name}
Version: {self.record.version}
"""
        if filename == "entry_points.txt":
            return "\n".join(
              f"""[{ep.group}]\n{ep.name}={ep.value}"""
              for ep in self.record.entry_points)

    def locate_file(self, path):
        raise RuntimeError("This distribution has no file system")