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

Remove module importing hack #1890

Merged
merged 3 commits into from Mar 7, 2020
Merged

Remove module importing hack #1890

merged 3 commits into from Mar 7, 2020

Conversation

rotu
Copy link
Contributor

@rotu rotu commented Oct 27, 2019

Summary of changes

Remove import hack that prevents objects from pickling. This was to support relative imports between vendored dependencies, but such relative imports do not (and should not) exist anyway.

Closes #1888

Pull Request Checklist

  • Changes have tests
  • News fragment added in changelog.d. See documentation for details

@rotu rotu force-pushed the patch-1 branch 4 times, most recently from 2aae88c to c9a828d Compare Oct 27, 2019
@rotu rotu changed the title Remove sys.modules hack Remove module importing hack Oct 27, 2019
Fix pypa#1888 (metadata accidentally not picklable), and removes a case where reimporting a vendored module results in a second copy of the same module.
@jaraco
Copy link
Member

@jaraco jaraco commented Oct 29, 2019

The comment around this hack is clearly there to protect this code - to indicate its purpose so it's not capriciously removed. I don't see in this PR or in the associated bug an analysis of why this code is no longer necessary. I do agree it's undesirable that the Distribution object isn't pickleable due to this hack, but we'll want to assess and weigh the costs of removing this hack with keeping it. Could you try tracing this code to its inception and see if there's any hint there as to what problem this code solves?

@rotu
Copy link
Contributor Author

@rotu rotu commented Oct 29, 2019

Could you try tracing this code to its inception and see if there's any hint there as to what problem this code solves?

This seems to be its origin, from when extern and _vendor both had their own VendorImporter: a76f5c0#diff-c6950cefad8b244938b76f24a0db9a6a
commit message: "Pop the module off the stack, preventing the 'Version' class from having a different manifestation in packaging than in pkg_resources."

Below is my understanding and may be wrong:

It would have been a problem at that time, since, depending which importer it goes through, you would have a different module object and a different class object, causing isinstance checks to fail.

def _coerce_version(self, version):
if not isinstance(version, (LegacyVersion, Version)):
version = parse(version)
return version

There still is some weirdness which I didn't address and exists both before and after removing this hack. Namely that, while immediate descendants of _vendor (e.g. setuptools.extern.packaging) report (via __name__) as being in the _vendor package, their children (e.g. setuptools.extern.packaging.version) report as being in the extern package instead.

@rotu
Copy link
Contributor Author

@rotu rotu commented Oct 29, 2019

If my understanding is wrong, could you please contribute a test that demonstrates the continued need for this code? I'm obviously wading into unfamiliar waters here, and it would help to have your insight as the originator of the hack.

@jaraco
Copy link
Member

@jaraco jaraco commented Mar 7, 2020

Here are my thoughts around when that hack originated.

@jaraco
Copy link
Member

@jaraco jaraco commented Mar 7, 2020

Could you please contribute a test that demonstrates the continued need for this code?

A fair and reasonable request. Reading my thoughts from four years ago, it appears to me as if tests were failing without this change. The fact that they're not now suggests that maybe the hack is unnecessary. I tried drafting a test, but I'm not able to ascertain what behavior was faulty without that effect.

I think the goal was that when using this importer, there should be exactly one instance of the imported modules in place... that there shouldn't be both setuptools._vendor.packaging and setuptools.extern.packaging in sys.modules.

I observe that in #1296, I copied the VendorImporter code. I definitely want to avoid allowing those implementations to deviate (and perhaps I should revert d0f7a33 unless revisiting that issue). Ugh. Those two have already deviated (949bd16), and on this line of code that's being removed.

@jaraco
Copy link
Member

@jaraco jaraco commented Mar 7, 2020

In this latest commit, I'm applying the same change to both implementations. Tests are passing for me locally. If they pass CI, then my plan is to move forward with this change. If it causes problems, we can capture those issues and add tests around those use-cases, and thanks to the tests you've added here, that expectation should be able to be retained as well.

@jaraco jaraco merged commit 640b283 into pypa:master Mar 7, 2020
20 of 22 checks passed
rotu added a commit to rotu/colcon-python-setup-py that referenced this issue Apr 22, 2020
Upstream change removes the need for much of our workaround: pypa/setuptools#1890
I do not remove turning the object into a dict, as it would change the public signature of `get_setup_information`.
rotu added a commit to rotu/colcon-python-setup-py that referenced this issue Apr 22, 2020
Upstream change removes the need for much of our workaround: pypa/setuptools#1890
I do not remove turning the object into a dict, as it would change the public signature of `get_setup_information`.
rotu added a commit to rotu/colcon-python-setup-py that referenced this issue Apr 24, 2020
Upstream change removes the need for much of our workaround: pypa/setuptools#1890
I do not remove turning the object into a dict, as it would change the public signature of `get_setup_information`.
rotu added a commit to rotu/colcon-python-setup-py that referenced this issue Apr 24, 2020
Upstream change removes the need for much of our workaround: pypa/setuptools#1890
I do not remove turning the object into a dict, as it would change the public signature of `get_setup_information`.
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.

2 participants