Tests using virtual environments: pass the copy of existing env to the subprocesses

[befeleme]

Summary of changes

In specifically set environment like the build environment in Fedora, we noticed some of the tests running subprocesses were failing. Typically they didn't see the correct paths, leading to multiple ModuleNotFoundError: No module named setuptools and failed meta-tests in test_virtualenv (DID NOT RAISE...).
The changes: passing the copy of the environment minus PYTHONPATH for the subprocesses made the test suite pass during our builds.

Pull Request Checklist

• Changes have tests (I'd say N/A - those are test fixes)
• News fragment added in changelog.d/.
  (See documentation for details)

[abravalheri]

@befeleme thank you very much for working on this.

There are somethings I don't understand: in the tests the venvs are supposed to have their own installation of setuptools, and testing a complete independent installation of setuptools after it is packaged, is also important...

When looking here:

setuptools/setuptools/tests/environment.py

Line 19 in 6193a69

cmd = [self.exe(cmd[0])] + cmd[1:]

and in jaraco.envs, the environment object seems to do the right thing to find the correct executable inside the virtual env...

When looking at:

setuptools/setuptools/tests/fixtures.py

Line 100 in 6193a69

env.req = str(setuptools_wheel)

setuptools/setuptools/tests/fixtures.py

Line 109 in 6193a69

env.create_opts = ['--no-setuptools']

and here, and here, I cannot understand how setuptools is not getting installed when it should (you mentioned ModuleNotFoundError: No module named setuptool), or why setuptools is getting installed when it should not not (you mentioned DID NOT RAISE...)...

Can you elaborate a bit more on how have you reached the conclusion that missing env vars are the root cause of this problem?
Do you know what would be the missing variables, and how did they influence the correct creation of the virtual environment used inside the tests.

[befeleme]

Firstly, thank you for taking time to look at this PR.

Let me elaborate on what happens with the current 60.9.3 when I run the test suite in the build environment for Fedora.
My understanding so far is that it's not about setuptools being or not being installed in venv, but about skewed visibility of modules if PYTHONPATH is set.
If you can point to a better explanation and fix, it would be much appreciated.

We set PYTHONPATH for the test run in the build environment to /builddir/build/BUILD/setuptools-60.9.3. The package was already built and installed to this directory and we want the test suite to run against the installed package.

Problem 1
When test_pip_upgrade_from_source runs the invocation of the meta-test, this happens in venv.run:

cmd is set to [Path('/tmp/pytest-of-mockbuild/pytest-33/test_pip_upgrade_from_source_p3/venv_without_setuptools/.env/bin/python'), '-c', 'import setuptools']
PYTHONPATH is set to /builddir/build/BUILD/setuptools-60.9.3

Test tries to import setuptools and it succeeds, and my understanding is that the installed-by-fedora-build package is indeed on PYTHONPATH and that's where 'DID NOT RAISE...' failure comes from.
If I unset PYTHONPATH before invoking the import, "global" setuptools stops being visible from the inside of the venv and the test passes as the exception is raised.

Problem 2
As for

• test_distutils_local and test_distutils_local_with_setuptools from test_distutils_adoption.py
• test_test_command_install_requirements from test_virtualenv.py

the situation looks a bit different.
The first two tests check whether the .env is present in distutils.__file__.

test_distutils_local doesn't find .env in the path.

>       assert venv.name in find_distutils(venv, env=env).split(os.sep)
E       AssertionError: assert '.env' in **['', 'usr', 'lib64', 'python3.10', 'distutils', '__init__.py\n']**

test_distutils_local_with_setuptools can't import setuptools, test_test_command_install_requirements can't import pkg_resources.

Traceback (most recent call last):
  File "<string>", line 1, in <module>
ModuleNotFoundError: No module named 'setuptools'

But if env.create() is run in the context unsetting PYTHONPATH, the magic happens and the assertions are true as .env is present in both paths:

/tmp/pytest-of-mockbuild/pytest-39/test_distutils_local_with_setu0/venv/.env/lib/python3.10/site-packages/setuptools/_distutils/__init__.py
['', 'tmp', 'pytest-of-mockbuild', 'pytest-40', 'test_distutils_local0', 'venv', '.env', 'lib', 'python3.10', 'site-packages', 'setuptools', '_distutils', '__init__.py\n']

I suspect that copy of the environment that takes place here duplicates "our" PYTHONPATH and causes essentially the same problem with visibility as in the first issue.

I hope this shows where we're being hit and why this approach was taken to fix it (I'm sorry for the wall of text, though!)

[abravalheri]

Thank you very much @befeleme for the explanation. Nice to see that we are starting to get to the bottom of the problems :)

Please correct me here if I am wrong, If I understand it correctly this problem will always happen if someone, instead of running the tests via tox, uses PYTHONPATH to point to a specific setuptools codebase, right? Once PYTHONPATH is set, it will leak to the spawned processed when env is not explicitly passed.

I always think that making tests more isolated is a good thing, so removing items from the environment variables that are not explicitly set should be fine!

However, I would avoid removing items from the env dictionary when it is explicitly set. For whatever reason, tests might need to set PYTHONPATH, so I would like to avoid surprises for the developers.

[abravalheri]

@befeleme there are somethings that are still not obvious to me, could you try to help me understand?

test_distutils_local doesn't find .env in the path
test_distutils_local_with_setuptools can't import setuptools, test_test_command_install_requirements can't import pkg_resources.

Assuming that implicit PYTHONPATH is removed from the environment variables in the subprocess¹, why would these tests fail? The venv is supposed to have an installation of setuptools from the wheel/sdist right? And SETUPTOOLS_USE_DISTUTILS="local" should prevent distutils from being loaded from the standard library...

I suspect that copy of the environment that takes place here duplicates "our" PYTHONPATH and causes essentially the same problem with visibility as in the first issue.

It is also not clear to me how having PYTHONPATH set while venv is being created will affect the way it runs afterwards²

Footnotes

1. In test_distutils_adoption.py, the env dictionary seems to be explicitly set (e.g. env = dict(SETUPTOOLS_USE_DISTUTILS='local')), so in this case PYTHONPATH should not be leaking... (It might only be leaking in test_pip_import and test_distutils_has_origin...)
   In test_test_command_install_requirements there is already a contexts.environment(PYTHONPATH=None, ...) preventing the leak... ↩

2. Maybe if --system-site-packages was passed to virtualenv, but that is not the case, right? ↩

[befeleme]

Please correct me here if I am wrong, If I understand it correctly this problem will always happen if someone, instead of running the tests via tox, uses PYTHONPATH to point to a specific setuptools codebase, right? Once PYTHONPATH is set, it will leak to the spawned processed when env is not explicitly passed.

Yes, this is what happens.

However, I would avoid removing items from the env dictionary when it is explicitly set. For whatever reason, tests might need to set PYTHONPATH, so I would like to avoid surprises for the developers.

Yes, this is possible. I've run the test suite on a modified patch, when I only touch environment which hasn't been explicitly set by test and the tests pass. Which puzzled me at first, but then given the venv is created and installs package in a context unsetting PYTHONPATH, this makes sense, as it's already removed from the way.

Regarding the second problem, I think @hroncok's clarification better explains the point I've missed.

I'll send the updated (and better explained) patch soon. Thank you for the well-pointed questions!

[abravalheri]

Thank you very much @befeleme! Sorry for disturbing you with a lot of questions.

[befeleme]

I have just sent the updated patch. The comments are quite verbose but I hope for a good clarity of the explanation (and I've never managed to keep it short, eh).

Sorry for disturbing you with a lot of questions.

No, it's great! This discussion helped me understand the issue much deeper. :)

[befeleme]

The last force-push invalidated the approved workflow. @abravalheri, could you please rerun it? I'm ready with the updates.

[abravalheri]

Thank you very much @befeleme, that is a very great work, and followed by an in depth-investigation!

[abravalheri]

@Mergifyio rebase

[mergify]

rebase

✅ Branch has been successfully rebased

[abravalheri]

I am sorry @befeleme, I was hopping that 5ae9aa4 would have fixed the problems with the CI but apparently we still have other problems to solve in the main branch.

Maybe this PR will take a bit longer to land (until we fix the problems in the main).

[jaraco]

@Mergifyio rebase

[mergify]

rebase

✅ Branch has been successfully rebased