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

[BUG] DistutilsMetaFinder breaks distutils imports when old setuptools is higher on path #2957

Closed
1 task done
nitzmahone opened this issue Dec 23, 2021 · 8 comments · Fixed by #2962
Closed
1 task done
Labels

Comments

@nitzmahone
Copy link
Contributor

nitzmahone commented Dec 23, 2021

setuptools version

setuptools==60.0.4

Python version

python3.8

OS

RHEL8, FreeBSD12/13, Ubuntu 20.04

Additional environment information

Any environment where an old setuptools (eg OS package) is higher in sys.path, but a newer setuptools is installed anywhere on sys.path (with its distutils shimming .pth file active)

Description

Any import of distutils will bomb when a new setuptools is present, but an older setuptools is higher on the path (since the new setuptools' path shim blindly tries to import from setuptools._distutils, which doesn't exist in old versions of setuptools). This has probably been the case ever since the distutils shim was created, but now that it's active by default, it's a bigger problem.

The place we hit this was running pip install from a venv with --system-site-packages- pip's build isolation installs latest setuptools, but then shuffles sys.path around such that the ancient OS-packaged setuptools is higher on the path. The first thing that tries to import distutils invokes the metapath finder, which has no validation or recovery and just lets the ModuleNotFoundError fly.

At first blush, it seems like the metapath shim's spec_for_distutils should try to at least verify the co-location of setuptools with the shim impl (ie, that they're under the same path prefix) and/or that the _distutils subpackage is available via find_spec before it actually imports it. Mixing and matching the shim version with random setuptools versions seems like a recipe for trouble down the road...

I could probably throw together a PR for this if there's a consensus that this approach is sane.

Expected behavior

The presence of a new-but-not-loadable setuptools on sys.path doesn't break old setuptools/distutils.

How to Reproduce

(NB: this is a much simpler repro than the OS installed setuptools that's actually failing, but illustrates the problem more easily on any OS)

python3 -m venv /tmp/bang && source /tmp/bang/bin/activate  # give us a clean venv to work in
python -m pip install 'setuptools>60'  # install new setuptools to venv site-packages
python -m pip install 'setuptools<45' -t .  # install old setuptools to cwd- higher than site-packages on path for most systems
python -c 'import distutils'

Output

...
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/tmp/bang/lib64/python3.9/site-packages/_distutils_hack/__init__.py", line 92, in create_module
    return importlib.import_module('setuptools._distutils')
  File "/usr/lib64/python3.9/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "/home/mdavis/setuptools/__init__.py", line 6, in <module>
    import distutils.core
  File "/tmp/bang/lib64/python3.9/site-packages/_distutils_hack/__init__.py", line 92, in create_module
    return importlib.import_module('setuptools._distutils')
  File "/usr/lib64/python3.9/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
ModuleNotFoundError: No module named 'setuptools._distutils'

Code of Conduct

  • I agree to follow the PSF Code of Conduct
@nitzmahone nitzmahone added bug Needs Triage Issues that need to be evaluated for severity and status. labels Dec 23, 2021
@jaraco
Copy link
Member

jaraco commented Dec 23, 2021

Nice report. I do agree the behavior as described is undesirable. The presumption was that exactly one setuptools instance is installed in a given environment. Perhaps that assumption needs to be revisited.

@jaraco jaraco removed the Needs Triage Issues that need to be evaluated for severity and status. label Dec 23, 2021
@nitzmahone
Copy link
Contributor Author

Yeah, even without our questionably-constructed "leaky" virtualenvs in play, even pip install --user can potentially cause problems here with >1 setuptools install visible to ordinary users.

@nitzmahone
Copy link
Contributor Author

I couldn't help myself throwing something together- doing it all with direct module loads in the .pth ended up being a lot tighter change than I thought it would be (thanks importlib.util!)... #2964 worked as expected with every wacky combo of older/newer versions I tried.

@jaraco
Copy link
Member

jaraco commented Dec 29, 2021

I'm beginning to wonder if this behavior is actually revealing a design defect in the Python site handling. In particular:

But if there are two site dirs, each with pth files that 'import' something, the paths will take precedence in the order encountered, so a pth file can expect for other paths to supersede it. A better expectation would be for a .pth file to be able to count on the current site dir having precedence.

@jaraco
Copy link
Member

jaraco commented Dec 29, 2021

A better expectation would be for a .pth file to be able to count on the current site dir having precedence.

On further consideration, as I started writing up a description of why a pth file should expect its modules to take precedence, I realized that expectation isn't reasonable. Once one module is imported by any .pth file, there's no reasonable way for a subsequent .pth file to import that same module from another location.

So instead, I believe the best thing to do is honor the path ordering as Python expects. First come, first served.

@bhilbert4
Copy link

We are seeing an error in what appears to be the opposite situation. Our readthedocs builds are failing because RTD is initially installing the latest version of setuptools. But later in the build, there is a requirement for setuptools<58.3.0. So RTD is uninstalling version 60.1.1 and installing 58.2.0. The build is then failing with a very similar traceback to what is shown above:

    import distutils.core
  File "/tmp/pip-build-env-q6clmc8t/overlay/lib/python3.7/site-packages/_distutils_hack/__init__.py", line 99, in create_module
    return importlib.import_module('setuptools._distutils')
  File "/home/docs/checkouts/readthedocs.org/user_builds/jwql/envs/829/lib/python3.7/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
ModuleNotFoundError: No module named 'setuptools._distutils'

I don't know enough to say for sure that this is the same issue as above, but it seems very similar, so I thought I would bring it up. Here is the link to our full RTD build log, in case that provides any clarity: https://readthedocs.org/api/v2/build/15642941.txt

@bhilbert4
Copy link

Just a quick note to say we solved our issue by not allowing the virtual environment on RTD to access the global site-packages directory.

@nitzmahone
Copy link
Contributor Author

Thanks @jaraco for merging a version of the short-term fix- that should at least keep the wheels on the wagon for awhile. Sorry for going dark for a couple weeks- I was trying really hard to stay unplugged over the holidays.

So instead, I believe the best thing to do is honor the path ordering as Python expects. First come, first served.

That works for most happy path situations, but falls down quickly if there's any reordering of sys.path before the user import of setuptools/distutils that causes a different setuptools to be found. It's not a problem today, but it will be as soon as some behavior of the shim needs to change. The most immediate problem case I can see there is the "am I running under pip?" short-circuit exit- that will break on any Python-sans-distutils. So let's say that gets accommodated in a hypothetical setuptools 61 shim- it's not going to work right if the setuptools 60 shim gets installed first, even if by the time the import occurs, setuptools 61 is at the front of sys.path. That's why I tried out the "pedantically correct and lazy as possible" version in #2964 .

Anyway, it seems like it might be worth tightening that up sooner than later, or there'll be all sorts of variations on these issues that will likely recur. Happy to kick around ideas and/or help out with impl and testing...

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants