Can this be applied to 2.11.x? That would require it to still support Python 2.7.

the importlib machinery is new in python 3 (as are pep 451 loaders) so I'm not sure it would help python2.7?

The current implementation does work in 2.7, it avoids the Python 3 specific stuff. It would be nice to fix this in 2.11.x if possible.

More radical would be reverting the removal of pkg_resources and making it part of 3.0 instead.

it could be backported with version guards probably

Here's a small repro:

https://github.com/gaborbernat/jinja-pytest

running tox shows:

ERROR:   bug: commands failed
  old: commands succeeded
ERROR:   fix: commands failed

So forgot to package the templates, which throw the same error. Fixed that and now my report prooves that this PR solves the issue, but latest released is still broken.

@davidism can we review/land this? 😊

I think this needs to be in 2.11.x. Maybe something like the following?

try:
    from importlib.util import find_spec

    spec = importlib.util.find_spec(package_name)
    loader = spec.loader
except ImportError:
    # Python 2
    spec = None
    loader = pkgutil.get_loader(package_name)

Fine by me 👍

Soft ping on what needs to happen for this 👍

Currently in the middle of some work on Werkzeug, then will get to this. If you or Anthony can move this to 2.11.x, that would let it happen sooner.

Soft ping on this 👍

Working on backporting it.

Backporting this makes it super complicated, and doesn't address the issue on Python 2. Debating just requiring importlib_resources for < 3.9.

is this a problem in python2? the new style loaders weren't introduced until python3

pkgutil.get_loader is used for both versions, but pytest doesn't define get_filename in either.

the 4.6 edition of pytest still uses a PEP 302 loader: https://github.com/pytest-dev/pytest/blob/020831d868319dbfa54046fdcce5fd853e20d281/src/_pytest/assertion/rewrite.py#L321-L325

That's not enough for what PackageLoader needs. That just allows opening files, not zip files, and doesn't allow listing the files.

@gaborbernat I think regardless of what the fix in Jinja is, Pytest needs to support get_filename, just like it added support for PEP 302 get_data. It's only rewriting SourceFileLoader, so it should be providing the same API, otherwise it may be breaking compatibility with other libraries that assume they're installed as source files.

How do we feel about using importlib_resources for Python < 3.9? Its files API is exactly what's needed here and is compatible with all supported Python versions. (I'm assuming @jaraco will get the new API into 3.9, it's not in the docs yet though.)

Pytest needs to support get_filename, just like it added support for PEP 302 get_data

the 4.6 maintenance branch is frozen (where the PEP 302 importer is implemented) and get_filename is not part of PEP 451 (pytest 5+) -- the import hook on the 4.6 branch is basically what it has been since creation and hasn't caused problems elsewhere -- the dependence on get_filename is the bug here as it's specified optional in the PEP

I don't really think importlib-resources solves anything here that the vanilla apis (origin / submodule_search_locations) already give us

get_data is still present in the modern loader. get_filename is optional, but for the only loader that Pytest is rewriting, it is implemented, so I don't see why Pytest shouldn't mirror that.

importlib_resources appeals to me because it provides one API for inspecting, opening, and listing folders and zip files, and is compatbile with all versions without a bunch of extra checks. The only thing it might not support is namespace packages, but since the files API just returns a path-like object, we can return a path-like object when detecting a namespace.

I should just revert the removal of pkg_resources in 2.11. The main source of problems is trying to remain compatible with Python 2. Dealing with folder/zip/namespace is minor compared to dealing with not having PEP 451 in Python 2.

I don't see why Pytest shouldn't mirror that.

because (1) it's difficult, pytest changes the module layout entirely (2) it's optional, a consumer can derive the path that they want (and often a better answer than pytest can provide you) from the information present

(not my project but) my 2c: I would maybe consider reverting whatever broke this on the 2.11.x branch and then moving forward with this patch in 3.x

We're thinking the same thing. :-)

OK, reverted in #1182, now I just have to merge that forward to master and rebase this.

How do we feel about using importlib_resources for Python < 3.9? Its files API is exactly what's needed here and is compatible with all supported Python versions. (I'm assuming @jaraco will get the new API into 3.9, it's not in the docs yet though.)

I'd recommend this approach. In fact, just rely on importlib_resources unconditionally until it lands upstream. It's very likely to land in 3.9, though there's a non-trivial chance some later reviews on the design might push out the adoption into Python 3.9.