Rewrite hook attempts to rewrite namespace package

[jaraco]

As discovered in pypa/setuptools#1038, when the following is true:

• a plugin (such as backports.unittest_mock) is a namespace package,
• that namespace package is defined using the recommended pkg_util technique,
• that plugin is used by the test environment, and
• something in the test suite attempts to import another package in that namespace,

py.test will attempt to rewrite the namespace package module, which will cause errors in sandboxed environments where writing outside of the prescribed directory is disallowed.

It's not obvious to me that this is a bug in pytest, and I admit, the factors are highly specialized.

Still, since namespace packages using the pkg_resources technique don't cause pytest to rewrite that package, I'd like to consider if this issue can be addressed by pytest detecting and honoring these namespace packages and excluding them from needing rewriting.

I have a minimal test project I will be pushing shortly demonstrating the issue.

[RonnyPfannschmidt]

@jaraco i believe the actual issue is that the rewriter also tries to write to the __pycache__ folders

is there a cheap way to detect the setuptools sandbox so we can opt out of writing in such cases

[jaraco]

Yes, indeed. The rewriter is attempting to create the __pycache__ folder for the namespace package. And while I've filed an issue with pypa/python-packaging-user-guide regarding the expectation of namespace packages, it seems interesting if not pertinent that this issue doesn't occur when the package(s) in question are managed by pkg_resources.

I want to be cautious (if possible) not to overfit the solution to this symptom. That is, it's not obvious to me that the rewrite hook should silently fail if it finds itself unable to write to the pycache folder... that is, unless it makes sense to always silently fail if creating the cache files is optional.

More importantly, I'd like to answer the question - why is pytest marking these packages as needing rewrite and should it be more precise about what needs rewritten? For example, I can see how the inclusion of backports.unittest_mock (the package) might signal to pytest to rewrite backports.unittest_mock (the module as indicated in the pytest11 entry point), but should it also rewrite every package and module contained in the distribution package containing that plugin? Should that also extend to other distribution packages sharing a namespace with the plugin? That is, what's the value of rewriting backports for backports.ssl_match_hostname?

[jaraco]

I disabled the functionality for marking plugin files for rewrite, after which the test suite quickly revealed that #1920 captures why plugins need their imports rewritten.

I experimented with the idea of only marking for rewrite those modules specified by the plugins, so if the pytest11: ['myplugin = foo.bar'], only foo.bar would be marked for rewrite and not every file or package in the distribution package for that plugin. However, it seems the tests (specifically TestImportHookInstallation.test_installed_plugin_rewrite) are asserting that functionality not referenced by the entry point explicitly is implicitly rewritten. In that test, spamplugin is the module that's referenced in the entry point, but because that module also imports hampkg, that causes other pytest fixtures to be loaded.

This behavior strikes me as problemmatic, as it's relying on the bundling of behavior into a distribution package to determine when to rewrite, which would fail in a scenario where a plugin imports fixtures from another package:

• myplugin has a dependency on other_pkg.
• myplugin defines a pytest11 entry point but other_pkg does not: ['myplugin = myplugin.fixtures'].
• in the module myplugin.fixtures, it imports other_pkg.fixtures which defines additional fixtures.

In this scenario, when myplugin (and thus other_pkg) are installed, the fixtures from both packages will be made available, but only those in myplugin will be assert rewritten (because those in other_pkg aren't part of the myplugin distribution, the current heuristic for determining candidacy for rewrite).

I propose that pytest should instead change the spec such that:

• only those modules indicated in the pytest11 entry point will be rewritten.
• plugin authors should either specify using entry points all of the modules that should be rewritten or there should be another mechanism by which a package may explicitly declare other modules as being plugin-relevant modules that should be rewritten.

Such a change would prevent pytest from over-eagerly reaching into some packages (as it does with namespace packages such as backports.*) but also prevent pytest from failing to match relevant modules such as other_pkg in the hypothetical scenario above. It would also correct the secondary issue reported downstream.

Thoughts?

[jaraco]

Also to note, such an approach would make the code for rewrite candidacy dramatically simpler, like:

for ep in pkg_resources.iter_entry_points('pytest11'):
    hook.mark_rewrite(ep.module_name)) 

[jaraco]

is there a cheap way to detect the setuptools sandbox so we can opt out of writing in such cases

The short answer is no. The sandboxing happens in a setuptools.sandbox.DirectorySandbox class that is never exposed by name. One could keep a reference to os.mkdir early, and then when detecting for sandbox, compare os.mkdir to the saved reference and if they're different, bypass the writing of the pyc.

As I mentioned before, I feel uncomfortable with the idea of pytest specifically avoiding writing pycache files specifically when in a sandbox. Can the pytest assert rewrites work without writing pycache files? It's conceivable that pytest could catch and suppress a sandbox.SandboxViolation in that code and treat it like one of the other cases where cache files aren't written.

It seems there are a few things that pytest could do here:

• change rewrite behavior to be more selective and explicit about what is rewritten (proposed above).
• detect and ignore namespace packages when doing rewrites.
• provide a hook or ask setuptools to monkeypatch pytest to avoid the error.
• suppress these errors when encountered (adds complication between pytest and setuptools mainly for the purpose of setuptools' own tests).

Those are in my order of preference, although the first option is by far the most invasive. I'm open to other ideas also.

I've worked around the urgency of the issue by pinning to the old backports module, but that solution is only temporary.

I'll wait until others have time to absorb the considerations and recommend the preferred way forward for pytest.

[RonnyPfannschmidt]

Given your very detailed description, I believe we should catch the sandbox error

[asottile]

@jaraco do you happen to have a minimal reproduction? I'm curious if #5468 resolves this

[jaraco]

See the aforementioned repository pytest-dev/pytest-issue-2419. It has two branches, master which I created a couple of years ago, and alt-repro, which I created recently not realizing I already had a reproducer. I think the master is a more accurate capturing of the issue. Run it with tox -e py27 and you'll see the rewrite warning using the latest released pytest. You'll also see a failure message which I think has changed over time.

You can't use pytest master due to Python 2 incompatibility (and your patch excludes the possibility of a backport), so I'm not sure your patch is relevant.

[asottile]

ok just confirmed that the changes in 5.0 do fix this, they can't be backported however because importlib doesn't exist (and a change of that scale likely shouldn't be backported anyway since it borders on "feature")

$ tox -e py37 -r
py37 recreate: /tmp/pytest-issue-2419/.tox/py37
py37 installdeps: pytest>=5, backports.unittest_mock==1.3
py37 develop-inst: /tmp/pytest-issue-2419
py37 installed: atomicwrites==1.3.0,attrs==19.1.0,backports.unittest-mock==1.3,importlib-metadata==0.18,more-itertools==7.1.0,packaging==19.0,pluggy==0.12.0,py==1.8.0,pyparsing==2.4.0,pytest==5.0.0,six==1.12.0,-e git+git@github.com:pytest-dev/pytest-issue-2419@1eab7a9c7a4bc579fcaa0f40f646e5136feec03a#egg=testproj,wcwidth==0.1.7,zipp==0.5.1
py37 run-test-pre: PYTHONHASHSEED='2338040538'
py37 run-test: commands[0] | py.test
============================= test session starts ==============================
platform linux -- Python 3.7.3, pytest-5.0.0, py-1.8.0, pluggy-0.12.0
cachedir: .tox/py37/.pytest_cache
rootdir: /tmp/pytest-issue-2419
plugins: backports.unittest-mock-1.3
collected 1 item                                                               

test_issue.py .                                                          [100%]

=========================== 1 passed in 0.01 seconds ===========================
___________________________________ summary ____________________________________
  py37: commands succeeded
  congratulations :)

2.7 produces a warning but at least does not outright fail:

$ tox -r -e py27
py27 create: /tmp/pytest-issue-2419/.tox/py27
py27 installdeps: pytest, backports.unittest_mock==1.3
py27 develop-inst: /tmp/pytest-issue-2419
py27 installed: DEPRECATION: Python 2.7 will reach the end of its life on January 1st, 2020. Please upgrade your Python as Python 2.7 won't be maintained after that date. A future version of pip will drop support for Python 2.7.,atomicwrites==1.3.0,attrs==19.1.0,backports.unittest-mock==1.3,configparser==3.7.4,contextlib2==0.5.5,funcsigs==1.0.2,importlib-metadata==0.18,mock==3.0.5,more-itertools==5.0.0,packaging==19.0,pathlib2==2.3.4,pluggy==0.12.0,py==1.8.0,pyparsing==2.4.0,pytest==4.6.4,scandir==1.10.0,six==1.12.0,-e git+git@github.com:pytest-dev/pytest-issue-2419@1eab7a9c7a4bc579fcaa0f40f646e5136feec03a#egg=testproj,wcwidth==0.1.7,zipp==0.5.1
py27 run-test-pre: PYTHONHASHSEED='3636244050'
py27 run-test: commands[0] | py.test
============================= test session starts ==============================
platform linux2 -- Python 2.7.15+, pytest-4.6.4, py-1.8.0, pluggy-0.12.0
cachedir: .tox/py27/.pytest_cache
rootdir: /tmp/pytest-issue-2419
plugins: backports.unittest-mock-1.3
collected 1 item                                                               

test_issue.py .                                                          [100%]

=============================== warnings summary ===============================
.tox/py27/local/lib/python2.7/site-packages/_pytest/config/__init__.py:784
.tox/py27/local/lib/python2.7/site-packages/_pytest/config/__init__.py:784
  /tmp/pytest-issue-2419/.tox/py27/local/lib/python2.7/site-packages/_pytest/config/__init__.py:784: PytestAssertRewriteWarning: Module already imported so cannot be rewritten: backports
    self._mark_plugins_for_rewrite(hook)

-- Docs: https://docs.pytest.org/en/latest/warnings.html
===================== 1 passed, 2 warnings in 0.01 seconds =====================
___________________________________ summary ____________________________________
  py27: commands succeeded
  congratulations :)

and that's probably the best we can do there?

[RonnyPfannschmidt]

we should simply not war for namespace package roots that get added by pth files

[asottile]

@RonnyPfannschmidt that's what's happening in pytest 5 -- as far as I can tell we can't detect that case to prevent it in python2 at all

[jaraco]

I'm seeing a similar warning in the pmxbot project:

.tox/python/lib/python3.9/site-packages/_pytest/config/__init__.py:1114
  /Users/jaraco/code/public/pmxbot/pmxbot/.tox/python/lib/python3.9/site-packages/_pytest/config/__init__.py:1114: PytestAssertRewriteWarning: Module already imported so cannot be rewritten: pmxbot
    self._mark_plugins_for_rewrite(hook)

This example is Python 3 only, and is a case where the package under test is also a plugin and exposes a namespace package (pmxbot).