-
Notifications
You must be signed in to change notification settings - Fork 3k
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 distlib to emulate pkg_resources #909
Conversation
Test results are same as in develop branch: One failure (test_https_opener_director_handlers).
I'd be much happier with this if vendor/distlib/pkg_resources.txt was somewhere else - it's not part of upstream distlib, so shouldn't be under distlib here. Actually it's not a "vendor" file at all as it isn't an unchanged copy of an upstream file. Also, the vendor/distlib changes are an issue, as they will presumably be broken if anyone re-vendors distlib. They seem to be mostly around the metadata 1.3 vs metadata 2.0 issue, so maybe it's a temporary problem.But of course, this is why wheel support was waiting on acceptance of the standards in the first place, and why the fact that metadata 2.0 is still in flux is hurting us :-( |
Fair point about the location of the |
I can't think of a better place to move the shim to than directly in the |
I couldn't think of an ideal location either. pip/pkg_resources sounds fine. Or maybe pip/compat/pkg_resources. But the extra level doesn't really add much other than complexity... |
I think I will move the shim to |
@@ -0,0 +1,262 @@ | |||
# This module is a shim to help migrate from the real pkg_resources |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to make sure I understand why you think this pkg_resources adapter help's the migration.
There's 4 things here to distinguish:
- not changing very much core pip code, for fear we might want to easily revert out of this later
- changing less code, because it decreases chances for bugs
- pip having classes/functions on top of distlib so as not to repeat routines over and over.
- pip continuing to use the pkg_resources interface, because that's perceived as a good thing for some reason?
what is your rationale for doing it this way in terms of 1-4?
unless we're really motivated by 1, not sure why we'd do this.
we're creating an adapter that we'll want to replace later with a more direct distlib layer.
we don't have that much code that consumes pkg_resources interfaces to replace.
in any case, the risk is mostly about switching to distlib, not so much having more code changes.
I want to make sure I understand why you think the pkg_resources adapter help's the migration. There's 4 things here to distinguish:
what is your rationale for doing it this way in terms of 1-4? unless we're really motivated by 1, not sure why we'd do this. |
I don't know yet to what extent it can help, but it certainly demonstrates that for a tool like
The advantage of minimising code changes means that the changes are easier to reason about and review. It's also important to be able to easily revert these changes if we do find problems.
Not sure how you're distinguishing between point 1) and point 2) here. I could make the changes even more minimal by e.g. having all the other modules import
That's part of a wider design consideration about
I don't see the
A larger change would take longer to review, and entail more risks (because any change could introduce a bug). It could also involve a fair amount of changes to the test code, which I've managed to avoid by using the strategy I did.
True, but the adapter should show that
Well, there has been some talk (I think by Daniel) of vendoring
I'm not opposed to wider code changes calling more directly into |
a "direct" non-adapter migration would demonstrate that too.
so that is part of your rationale, i.e. that we might end up doing some post-release revert of all this.
I was distinguishing:
whether a direct approach would be that much larger and harder, is what I'm not sure about. |
@@ -387,6 +387,17 @@ def __init__(self, environ=None, sitecustomize=None): | |||
self.run('python', '-c', | |||
'"import sys; sys.path.insert(0, %r); import pip; sys.exit(pip.main());"' % os.path.dirname(here), | |||
'uninstall', '-vvv', '-y', 'pip') | |||
# For some reason, the above command can return with a zero exit code, no |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look at this. regardless of this pull, I'm curious what might be going on here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed - there wasn't even a non-zero returncode
to indicate something might be wrong.
A non-adapter migration would entail more work and more risk, but I agree that longer term a different approach would be better. However, this approach allows that to be done module by module, rather than a "big bang" which from my experience would likely take longer to achieve, because "everything would stop working" while bugs were ironed out.
The likelihood of having to revert is dependent on the confidence level in the coverage of the pip tests. I don't see a show-stopper problem which would require a revert (any bugs that show up can be fixed just as I've been doing during this development) but I want to make it easy to revert, should it be necessary.
Making fewer changes makes it easier to make decisions about reverting, because it's easier to see what's going on.
Well, the adapter is done now, so there's no work there apart from reviewing it (easier than reviewing a direct-to- |
my point is that the "big bang" is moving over to distlib, which is where most of the risk is at.
this argument has the most power practically : ) |
just included this pull as an option to fast track getting pip PEP439 compliant (see #863) |
…Test parity with develop achieved.
I'm closing this as we've vendored pkg_resources now, and any move to replace pkg_resources with distlib will likely be done without a shim layer. |
I've merged the latest changes. I got some test failures, but all of these appear to be because the test virtualenv created in
tests/tests_cache/test_ws/setuptools/.virtualenv
had both pip-1.3.1 and pip-1.4.dev1 in itssite-packages
. I can't see any reason why the uninstallation of pip during the test environment creation fails, and there's no check for failure in the env creation process.I fixed the failures by adding code to ensure that any old
pip
directory is removed after the uninstallation.Note: There are some occurrences of
pkg_resources
in string literals, which I haven't changed. These might need looking at.