Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to more recent 'files' API from importlib.resources #147

Closed

Conversation

jaraco
Copy link

@jaraco jaraco commented Jan 24, 2021

As found on Python 3.9 and importlib_metadata 1.1. Add explicit temporary file cleanup behavior using atexit. Fixes #131.

…n 3.9 and importlib_metadata 1.1. Add explicit temporary file cleanup behavior using atexit. Fixes certifi#131.
@jaraco
Copy link
Author

jaraco commented May 23, 2021

Anything else to consider here?

@jaraco
Copy link
Author

jaraco commented Oct 17, 2021

Also fixes #170.

@Lukasa
Copy link
Member

Lukasa commented Oct 18, 2021

It seems that this will regress the behaviour on older versions of Python 3 which previously used the resources API but now fall back to the Python 2 behaviour. Is there any reason not to preserve that behaviour?

@jaraco
Copy link
Author

jaraco commented Oct 18, 2021

It seems that this will regress the behaviour on older versions of Python 3 which previously used the resources API but now fall back to the Python 2 behaviour.

Good point. Thanks for raising it. I can't remember if I considered it when I authored the change, but since I didn't mention it, it's likely I missed it.

Actually, importlib.resources was only introduced in Python 3.7, so the fallback behavior was pertinent to Python < 3.7.

So this regression would apply to Python 3.7 and 3.8 in environments without importlib_resources > 1.4 and where certifi is not installed on the file system.

Is there any reason not to preserve that behaviour?

My hope was to avoid relying on the legacy behavior at all and environments requiring/desiring robust support on older Pythons would ensure that importlib_resources was present. In other words, I expect that importlib_resources could supply the fallback behavior. Ideally, I'd just add importlib_resources>1.4; python_version<"3.9" to the install dependencies of certifi, but I know this project is reluctant to adopt dependencies.

Would this project consider adding that dependency, perhaps as an "optional" dependency (installed by default, but with fallback behavior if it's not present)?

I'd like to avoid adding fallback to legacy behaviors if possible, but I'm open to it if it's deemed necessary.

@Lukasa
Copy link
Member

Lukasa commented Oct 26, 2021

I'm disinclined to add dependencies to this project. It's astonishing how much complexity this project contains to do the simple job of providing the contents of a file, and adding a dependency seems to be stepping even further.

@sigmavirus24
Copy link
Member

Further, https://discuss.python.org/t/deprecating-importlib-resources-legacy-api/11386/21 indicates that the importlib.resources and importlib.metadata APIs may never stabilize

Unfortunately, I don’t have a crystal ball, so I can’t be certain. When it was written and published, it was presumed to be stable and dependable. It was known not to cover all known use cases, but was expected to cover most relating to resources. As it turns out, that assumption was wrong. But even then, it’s not obvious that an incomplete solution couldn’t be extended in a compatible way. It wasn’t until users reported shortcomings and we delved into the problem that we found the existing design to be inadequate for the use-case.

On top of that this pull request is likely inadequate for the variety of versions of importlib_resources available as backports when certifi is installed with another project as pointed out https://discuss.python.org/t/deprecating-importlib-resources-legacy-api/11386/15

I'm also disinclined to accept this. With 2 of us being against this, I'm going to close it

@jaraco
Copy link
Author

jaraco commented Nov 25, 2021

On top of that this pull request is likely inadequate for the variety of versions of importlib_resources available as backports when certifi is installed with another project as pointed out https://discuss.python.org/t/deprecating-importlib-resources-legacy-api/11386/15

Anthony was mistaken. In this post, I clarify that importlib_resources>=1.3 should have widespread adoption and is unlikely to conflict with other environments (it's unlikely an environment has pinned to <1.3 or lower for any specific reason). I believe Anthony was projecting experiences from another project.

Further, https://discuss.python.org/t/deprecating-importlib-resources-legacy-api/11386/21 indicates that the importlib.resources and importlib.metadata APIs may never stabilize

That's not a fair characterization of those statements. Those projects are believed to be stable, but may evolve as any project might. Regardless of the stability, this project's current implementation is reliant on a "janky" (abusive) use of deprecated functionality in importlib.resources.

I'd offered to devise a solution with an additional fallback that doesn't introduce any new dependencies if that was this project's preferences. Instead, the PR was closed summarily with the implication that no patch that relies on importlib.resources will be accepted. Yet the current implementation relies on importlib.resources.

I came here to help fix the issues this project has, avoid the kinds of issues that stem from mis-use of the API, and illustrate a transition from the legacy API.

Let me know if there's more I can do to help.

@tacaswell
Copy link

I request that this get reconsidered sooner rather than later given that as of python/cpython#29036 (which looks like it will be on py311a3) the current usage is warning (which is in turn breaking down-stream test suites).

@sigmavirus24
Copy link
Member

I believe Anthony was projecting experiences from another project.

As a maintainer of requests, the whole reason it used to vendor dependencies was problems like this. Luckily it has enough sway that the lower bounds it has are fairly well respected at this point. Other projects are rarely so lucky and are often harassed into removing lower bounds because some other project has pinned the same dependency. It's not rare.

but may evolve as any project might.

That's a fine way around saying "People won't have a supported API across multiple versions of Python they might need to support."

Instead, the PR was closed summarily with the implication that no patch that relies on importlib.resources will be accepted. Yet the current implementation relies on importlib.resources.

I must not have been clear. And somehow Cory's very direct "I'm disinclined to add dependencies" was also not clear. We're not adding backport dependencies and for what little we need from importlib.resources the current APIs are satisfactory.

I also don't know why we'd want to suddenly be maintaining lots more code to paper over a deprecation decision that could have been more gracefully executed especially given the many months of feedback on the plan that have been thrown out without a reason other than "My opinion is the new APIs are superior, despite your opinion being that the old APIs were already sufficient".

The-Compiler added a commit to qutebrowser/qutebrowser that referenced this pull request Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot import requests/certifi from embedded zipfile since 2020.4.5.2 release
4 participants