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

distutils shim should ignore setuptools on another path #2962

Merged
merged 5 commits into from
Dec 29, 2021

Conversation

nitzmahone
Copy link
Contributor

@nitzmahone nitzmahone commented Dec 23, 2021

Summary of changes

Prevents _distutils_hack finder shim from interacting with a possibly incompatible setuptools higher on sys.path

Closes #2957

Pull Request Checklist

@nitzmahone
Copy link
Contributor Author

I actually found a bunch of other potential problems while trying to get this working. Bigger picture- a lot of the problem cases are due to the .pth shim hack being in a separate top-level package than setuptools itself. Is there a reason the .pth can't be under the main setuptools top-level package? (I got nailed with similar problems on pyyaml when we moved the optional _yaml native extension top-level package into a subpackage, making the old top-level package visible from lower entries on sys.path)

@jaraco
Copy link
Member

jaraco commented Dec 23, 2021

Is there a reason the .pth can't be under the main setuptools top-level package?

Yes. .pth files are only processed if they appear directly in site directories (as registered by site.addsitedir), usually a small subset of paths on sys.path.

@jaraco
Copy link
Member

jaraco commented Dec 23, 2021

Maybe a better approach would be to update the DistutilsLoader to always ensure it imports setuptools._distutils from the intended path (same as the .pth file).

@nitzmahone
Copy link
Contributor Author

nitzmahone commented Dec 23, 2021

Yes. .pth files are only processed if they appear directly in site directories (as registered by site.addsitedir), usually a small subset of paths on sys.path.

Ah of course- I forgot how the actual mechanism functioned under the covers...

update the DistutilsLoader to always ensure it imports setuptools._distutils from the intended path (same as the .pth file).

I'm not sure that'd cover all the problem cases- unless I'm misunderstanding your approach, that'd open up some strange mismatch possibilities where you're directly importing content from packages that might end up being shadowed on sys.path (since IIRC, each .pth from a registered site dir is going to run as sys.path is being built up, but we don't know until the user actually imports setuptools or distutils which site dir is going to "win").

I hate to even suggest this, but it seems like the only way to account for not knowing which setuptools will "win" in the face of early startup sys.path shenanigans by users/tools is to have each .pth install a dynamically-typed shim- ie, each .pth directly defines and installs its own shim type that will only service its associated setuptools instance. So if there were 10 different installs of setuptools visible (and thus 10 .pth's that ran at startup), there'd actually be 10 shims of different (runtime) types (that didn't import anything from setuptools/_distutils_hack packages to avoid chicken/egg shadowing problems), and only one should ever actually "work". The one shim that actually does its work on the first user import of setuptools/distutils should theoretically be able to clean the others up off the metapath. I think that model would also get rid of the need for the separate _distutils_hack package altogether (and problems from its cross-path visibility), at the expense of potentially needing to duplicate some of the shim code that the setuptools package init uses directly. IIUC the reason for the separate _distutils_hack package is to avoid the .pth invoking any of the setuptools package init too early- this would preserve that as well.

I can probably throw together a prototype of this if you want a strawman to poke at- especially with the shimming being on by default now, it seems like the scope of this problem and/or potential future incompatibilities with mismatched shim versions + setuptools versions is only going to get bigger.

assert '_distutils' in core.__file__, core.__file__
# FIXME: this assertion blows up if the MetaFinder below has no-opped on a
# setuptools from another path
# assert '_distutils' in core.__file__, core.__file__
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to understand why this check is a problem. ensure_local_distutils is only called by do_override, which is only called by importing setuptools. But if an old version of setuptools is earlier on sys.path, it will not invoke the override.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 41f61f3, I've removed the commented code and restored the expectation to see what breaks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha. I see now (in the failed RTD build). Because the check for "matching setuptools" was very particular, and because another copy of Setuptools was taking precedence, and because local checkouts don't get the .pth treatment, the shim wasn't getting loaded for any invocation (I think).

In da8c68c, I'm exploring allowing any setuptools to satisfy the distutils requirement.

@jaraco
Copy link
Member

jaraco commented Dec 29, 2021

since IIRC, each .pth from a registered site dir is going to run as sys.path is being built up, but we don't know until the user actually imports setuptools or distutils which site dir is going to "win"

In my investigation, I found that site paths are always appended, so the first .pth file to run should take precedence... and any site dirs added later should be shadowed by the first. The only concern then is if an older setuptools without the shim appears earlier on sys.path, and in that case, it should be safe to disable the shim, since that version of setuptools doesn't expect a local distutils.

@jaraco
Copy link
Member

jaraco commented Dec 29, 2021

I'm liking this implementation better, for its simplicity and reliance on import semantics. Let's see if it solves the primary issue(s) and we can evaluate if the other identified issues justify further accommodation.

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.

[BUG] DistutilsMetaFinder breaks distutils imports when old setuptools is higher on path
2 participants