-
Notifications
You must be signed in to change notification settings - Fork 405
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
Use importlib.metadata #344
Conversation
Between #348 and the fact that the metadata inspector swallows all exceptions, I'm struggling to determine where the error is in the code. |
Patching the venv inspector and manually invoking it, I see the error:
|
I've fixed two issues, but another still remains - when attempting to resolve the dependency of |
Correction - the issue isn't the naming convention. |
I'm stumped. I don't know what's causing the changes to fail. The venv inspector script runs on my local machine. The fact that it's apparently emitting nothing suggests it's failing to run to |
It appears you need to install
I believe this reuses the pipx 'shared' virtual environment which - incidentally - is not isolated from the user installation during testing. My |
The tests on Travis will pass making the following change: diff --git a/src/pipx/shared_libs.py b/src/pipx/shared_libs.py
index 9a375c2..b2efcbd 100644
--- a/src/pipx/shared_libs.py
+++ b/src/pipx/shared_libs.py
@@ -79,6 +79,7 @@ class _SharedLibs:
"pip",
"setuptools",
"wheel",
+ "packaging",
]
)
self.has_been_updated_this_run = True The shared library would also have to be updated when upgrading pipx to install |
Whoops, for 3.7 and lower you will also need to add |
I still don't understand why we need to eliminate pkg_resources. Can somebody explain that? I've googled a lot and can't find anything saying it or setuptools is deprecated. Also, the use of pkg_resource in I'm not in favor of this PR without a really good reason. |
I'm the maintainer of setuptools and author of importlib.metadata. In coordination with Barry Warsaw, we created importlib.metadata to compliment importlib.resources in order to replace pkg_resources. I can say with authority that pkg_resources is deprecated. It has intractable bugs and undesirable behaviors that were easier to sidestep than fix. I haven't yet gotten around to documenting the deprecation, and it has no deadline, but the intention is to move use-cases like this one to those tools. The welcome paragraph alludes to the replacement, implying deprecation. I submit this change here with the expectation that it would be a preferable change. And at first, this change does simplify the code, but then it became more complicated when adding support for environment markers (which pkg_resources has integrated). Probably there should be some code that draws these behaviors together with clean interfaces. If the adoption of this change is objectionable for reasons other than YAGNI, I want to address those objections in packaging, importlib_metadata, etc. If the sole objection is YAGNI, I'd like to encourage you to work with me to make the shift as a demonstration of what moving off of pkg_resources (and possibly setuptools) looks like. |
@jaraco, thanks for the explanation how it is deprecated. I would hear you and @gaborbernat referring to it, but I could not find anything publicly available that would say the same thing. Going forward it might be nice if there was a reference online that talked about deprecating pkg_resources so more people know about it (besides the maintainer of setuptools himself! 😄). I agree there's a lot of goofiness in pkg_resources. My caution is not YAGNI as far as I understand it, I just didn't want us to rush into something that has a pervasive influence on how pipx operates and could cause bugs--when what we have works, and there was no hint anywhere online that pkg_resources is going away or deprecated. It's nice now to see the rationale, from your above comment. If @cs01 is fine with this I'm not going to object. But we should be careful that we cover existing users, have a scheme to force an update of the shared libs, and basically don't require a |
I also like treading carefully when modifying |
One of your failing tests seems to be installing a package in python3.5, which we currently support in venvs. Is |
One question I have about
Historically, editable packages from a local filepath have been the edge cases for pipx's install system, and have been the source of errors when other installs work. I don't think we currently have a local editable install in our tests. |
@itsayellow It does. An editable install puts an |
Yes.
Yes. Out of the box, importlib_metadata supports packages on sys.path with |
Even after getting tests passing on master, I'm struggling to replicate the test failures for this branch as observed on Travis in my local environment. Instead, the tests fail early in |
Indeed, it seems the monkeypatch in the pipx_temp_env fixture is insufficient to isolate the test runs from the user's environment because the
|
Yes, like I said above:
|
…d packages without requirements metadata.
I'm seeing this issue too. I think the problem is that 'importlib_metadata' is being installed in 'shared_libs', but for Python 3.8 (e.g. $shared_libs/lib/python3.8/site-packages/importlib_metadata), which isn't relevant for Python 3.5. I don't understand how 'shared_libs' as a single venv makes sense in a world of multiple Python versions. My guess is that the only reason the tests aren't failing now is because the existing dependencies (setuptools) are installed implicitly in the main, non-shared environment. |
Actually I always wondered how the same shared libs dir was servicing possibly multiple versions of python too... Do we need a separate shared libs dir for each python major.minor? |
The tests aren't failing because pipx creates a |
…necessarily run on all targeted environments.
All of the tests are passing except for the Windows tests. I ran the tests in my Windows environment and they were passing on Python 3.8, but failing on Python 3.7 (perhaps similar to the Travis failures; I'm still awaiting the output). |
I've spent too many hours of my life on this. I'm giving up. |
This is a shot in the dark but it looks like Windows might be truncating the path to 180 characters which so happens to result in:
... which also might be why it can't find a |
The default |
I asked this when @pfmoore added it. It was designed assuming the shared libs work across all python versions. |
(Or at least, all versions that pipx supports) It's worth noting that setuptools has now dropped support for Python < 3.5. As pipx is Python >= 3.6 only, this isn't a problem yet, but it bears watching. When I designed the feature, I was assuming that the various core packaging tools would be more cautious than average over dropping older Python versions. While that's true for pip so far, it hasn't turned out that way for the other tools. |
Working on #339, I discovered this use of the deprecated
pkg_resources
module. This change adds a dependency onimportlib_metadata
for python 3.7 and earlier and uses the stdlib version on Python 3.8 and later.I haven't tested this behavior, so I could use some help validating that it does what's expected.