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

Test suite compatibility with the latest virtualenv >= 20.0.0 #8441

Closed
wants to merge 14 commits into from

Conversation

frenzymadness
Copy link
Contributor

I'd like to continue with the effort from #7698 and #7718 and make the pip test suite use the latest released virtualenv.

This PR is WIP created to see the CI status.

@frenzymadness
Copy link
Contributor Author

After hours of debugging, I've finaly found the root cause of Python 2.7 and test_build_env_isolation failure.

--------------------------------------------------------- Captured stdout call ---------------------------------------------------------
Script result: python /tmp/pytest-of-lbalhar/pytest-0/test_build_env_isolation0/workspace/scratch/build_env.py /tmp/pytest-of-lbalhar/pytest-0/test_build_env_isolation0/workspace/scratch/test.py
  return code: 1
-- stderr: --------------------
imported `pkg` from `/tmp/pytest-of-lbalhar/pytest-0/test_build_env_isolation0/workspace/venv/lib/python2.7/site-packages/pkg/__init__.pyc`
system sites:
  /tmp/pytest-of-lbalhar/pytest-0/test_build_env_isolation0/workspace/venv/lib/python2.7/site-packages
  /tmp/pytest-of-lbalhar/pytest-0/test_build_env_isolation0/workspace/venv/lib64/python2.7/site-packages
sys.path:
  /tmp/pytest-of-lbalhar/pytest-0/test_build_env_isolation0/workspace/scratch
  /tmp/pytest-of-lbalhar/pytest-0/test_build_env_isolation0/workspace/tmp/pip-build-env-TxuwZV/site
  /usr/lib/python27.zip
  /usr/lib64/python2.7
  /usr/lib64/python2.7/plat-linux2
  /usr/lib64/python2.7/lib-tk
  /usr/lib64/python2.7/lib-old
  /usr/lib64/python2.7/lib-dynload
  /tmp/pytest-of-lbalhar/pytest-0/test_build_env_isolation0/workspace/tmp/pip-build-env-TxuwZV/overlay/lib/python2.7/site-packages
  /tmp/pytest-of-lbalhar/pytest-0/test_build_env_isolation0/workspace/tmp/pip-build-env-TxuwZV/overlay/lib64/python2.7/site-packages
  /tmp/pytest-of-lbalhar/pytest-0/test_build_env_isolation0/workspace/tmp/pip-build-env-TxuwZV/normal/lib/python2.7/site-packages
  /tmp/pytest-of-lbalhar/pytest-0/test_build_env_isolation0/workspace/tmp/pip-build-env-TxuwZV/normal/lib64/python2.7/site-packages
  /tmp/pytest-of-lbalhar/pytest-0/test_build_env_isolation0/workspace/venv/lib64/python2.7/site-packages
  /tmp/pytest-of-lbalhar/pytest-0/test_build_env_isolation0/workspace/venv/lib/python2.7/site-packages
  /tmp/pytest-of-lbalhar/pytest-0/test_build_env_isolation0/workspace/scratch/pth_install
  /tmp/pytest-of-lbalhar/pytest-0/setuptools0/install
  /tmp/pytest-of-lbalhar/pytest-0/pip0/pip/src
  /usr/lib64/python2.7/site-packages
  /usr/lib64/python2.7/site-packages/gtk-2.0
  /usr/lib/python2.7/site-packages
Traceback (most recent call last):
  File "/tmp/pytest-of-lbalhar/pytest-0/test_build_env_isolation0/workspace/scratch/build_env.py", line 33, in <module>
    subprocess.check_call((sys.executable, sys.argv[1]))
  File "/usr/lib64/python2.7/subprocess.py", line 190, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '('/tmp/pytest-of-lbalhar/pytest-0/test_build_env_isolation0/workspace/venv/bin/python', '/tmp/pytest-of-lbalhar/pytest-0/test_build_env_isolation0/workspace/scratch/test.py')' returned non-zero exit status 1

pkg is available but it should not be in the isolated build environment. The problem seems to be that the fixture virtualenv_template has

pip/tests/conftest.py

Lines 337 to 340 in 445711c

# Enable user site packages.
venv.user_site_packages = True

which is then translated to include-system-site-packages = true in

pip/tests/lib/venv.py

Lines 134 to 146 in 201c9fc

@user_site_packages.setter
def user_site_packages(self, value):
self._user_site_packages = value
self._customize_site()
pyvenv_cfg = self.location.joinpath("pyvenv.cfg")
modified_lines = []
for line in pyvenv_cfg.read_text().splitlines():
k, v = line.split("=", 1)
if k.strip() == "include-system-site-packages":
line = "{}= {}".format(k, "true" if value else "false")
modified_lines.append(line)
pyvenv_cfg.write_text("\n".join(modified_lines))

sys.path is adjusted in BuildEnvironment context manager (via sitecustomize.py) but the new virtualenv seems to load site Python module before it adds global packages to sys.path:

https://github.com/pypa/virtualenv/blob/f99353ca3d0ca9e23cfe4b66e54ba653bf99ab4a/src/virtualenv/create/via_global_ref/builtin/python2/site.py#L10-L21

I honestly don't know how to fix that because the whole tests is kinda complex/complicated and the chain of fixtures (test_build_env_isolation → script → virtualenv → virtualenv_factory → virtualenv_template) as well. Any help would be very appreciated. I'll investigate other failures in the meantime.

@frenzymadness
Copy link
Contributor Author

Correction to my last comment about test_build_env_isolation: It's not caused by availability of system-site packages but by virtualenv which alters sys.path after sitecustomize.py is loaded. And because I think of it as about a bug, I've filled: pypa/virtualenv#1861

@frenzymadness
Copy link
Contributor Author

The problem with these two (py38):

FAILED tests/functional/test_install.py::test_install_pep508_with_url - AssertionError: Script result: python -m pip install --no-ind...
FAILED tests/functional/test_install.py::test_install_pep508_with_url_in_install_requires

is that the virtual environment has access to system site-packages and packaging is already installed there.

@frenzymadness frenzymadness force-pushed the virtualenv-20 branch 2 times, most recently from cd18c2c to 329819d Compare June 16, 2020 10:20
@frenzymadness
Copy link
Contributor Author

Well, it seems that we have some progress here 🎉

Is there anybody who can help me with the issues on Windows? I don't have windows so I cannot reproduce them.

I'll take a look on pypy.

@uranusjr
Copy link
Member

Thanks for all the investigation effort, I incerely hope we’ll be able to come up with a solution soon 😃

The sys.path manipulation sounds hairy. Regardless of whether virtualenv doing it after *customize.py is correct or not, I have a feeling it does not really have an alternative due to how Python 2 startup functions, and we’ll likely need to find a workaround somehow.

@pfmoore
Copy link
Member

pfmoore commented Jun 16, 2020

Just a note, but if this is specific Python 2 hackery (which I'm not clear on, because this comment suggests there's something up on Python 3) I'd be fine with patching over the problem, as we'll be dropping Python 2 support fairly soon.

@frenzymadness
Copy link
Contributor Author

Just a note, but if this is specific Python 2 hackery (which I'm not clear on, because this comment suggests there's something up on Python 3) I'd be fine with patching over the problem, as we'll be dropping Python 2 support fairly soon.

It's only about Python 2 and a hacky way test_build_env_isolation is implemented and based on virtualenv because virtualenv has to do some dark magic for Python 2 to guarantee that sys.path contains all paths from virtualenv. See the discussion in pypa/virtualenv#1861

If you are okay with that, I'd prefer to skip that test for Python 2 rather than investing energy to find a solution for soon-to-be-dropped Python 2.

@frenzymadness frenzymadness force-pushed the virtualenv-20 branch 4 times, most recently from a72e4c9 to a8a8fa8 Compare June 17, 2020 10:19
@pradyunsg
Copy link
Member

I'm 100% on board to just straight up skip the test, with a (long) comment describing why we're skipping it and why we're hitting this issue.

@frenzymadness
Copy link
Contributor Author

I'll get back to it in ~2 weeks.

@chrahunt
Copy link
Member

chrahunt commented Jul 14, 2020

I don't have windows so I cannot reproduce them.

FWIW, Microsoft offers Windows VM images here that are good for like 90 days. That's what I use.

@frenzymadness
Copy link
Contributor Author

PR is rebased on top of the latest master and I'll continue.

@pradyunsg pradyunsg changed the title Compatibility with the latest virtualenv >= 20.0.0 Test suite compatibility with the latest virtualenv >= 20.0.0 Jul 28, 2020
@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Aug 17, 2020
uranusjr and others added 8 commits September 1, 2020 08:18
virtualenv 20.0+ completely revamps its API:

* There's no path_locations() anymore. Stdlib sysconfig can replace it
  completely.
* The Python API to create an environment is split into several pieces.
  Use the entry point function for now to provide the functionality.
I *think* virtualenv fixed these? We'll see.
This is what the old virtualenv implementation did, so we match that.
The latest virtualenv does some dark magic for sys.path in
Python 2 so the BuildEnvironment is no longer isolated here.

See: pypa/virtualenv#1861
These tests should not have access to site packages so
they always install the package into an empty environment.
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Sep 1, 2020
@frenzymadness
Copy link
Contributor Author

I'll try to move this forward. One question: If I understand it correctly, the new virtualenv uses the same configuration as the builtin venv module so the code for the legacy virtualenv can be deleted, right? For example, functions checking the availability of system site-packages _no_global_under_regular_virtualenv.

@uranusjr
Copy link
Member

uranusjr commented Sep 1, 2020

The main source src/pip still need to contain code to detect legacy virtualenvs (probably until the end of time), since not everyone has upgraded (will ever upgrade) to virtualenv 20.

@frenzymadness
Copy link
Contributor Author

I'm afraid I won't be able to continue on this effort. The codebase is too complex for me and I don't have enough free time to dive deep enough to move pip to the new virtualenv while still support compatibility with venv and old virtualenv.

Is there anybody who can help me with this or take over it?

@pradyunsg
Copy link
Member

Ahoy! What kind of help would you want with this? I'm happy to help. :)

@frenzymadness
Copy link
Contributor Author

Ahoy! What kind of help would you want with this? I'm happy to help. :)

I'd like an insight into pip codebase because it seems that pip itself has some code to detect different kinds of virtual environments to be able to install packages to them and also its tests depend heavily on venv/virtualenv.

Are virtual environments in tests used to test how pip handles them or just to isolate one test from another? Do we want to test pip with venv, new virtualenv, and also the old virtualenv? It seems to me that the virtual environments created by venv are very similar to those produced by the new virtualenv so is there any reason to distinguish between them?

@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Feb 4, 2021
@pradyunsg
Copy link
Member

Looking at this now, this likely needs a lot of additional work.

I'll pick this up this weekend, and see where I can take it. :)

@pradyunsg
Copy link
Member

pradyunsg commented Apr 2, 2021

I'll close this for now, since this one is significantly bitrotten; and filing a new PR is cheap. We can have a further discussion in #8273. ^>^

@pradyunsg pradyunsg closed this Apr 2, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs rebase or merge PR has conflicts with current master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants