Fix regression with sys.path filter

[cdce8p]

Steps

• Add yourself to CONTRIBUTORS if you are a new contributor.
• Add a ChangeLog entry describing what your PR does.
• If it's a new feature or an important bug fix, add a What's New entry in doc/whatsnew/<current release.rst>.
• Write a good description on what the PR does.

Description

This MR fixes the regression with sys.path filter by checking PYTHONPATH before removing any additional entries.
This should fix the new issue (#4252) without undoing the previous one.

This should ideally be included in 2.7.3

Type of Changes

	Type
✓	🐛 Bug fix

Related Issue

Closes #4252

Followup to
#4153
#4164

--
CC: @sbraz, @Pierre-Sassoulas

[coveralls]

Coverage increased (+0.004%) to 91.433% when pulling 2991b4b on cdce8p:fix-sys-path into 95daeca on PyCQA:master.

[cdce8p]

cc: @stanislavlevin

[Pierre-Sassoulas]

This looks reasonable. I don't see where the launching of the plugin is tested, could we even test that ? Maybe an integration test patching sys.argv ?

[sbraz]

Tests don't pass for me with

FROM python:3.9
RUN git clone --depth 1 https://github.com/PyCQA/pylint
WORKDIR pylint
RUN git fetch origin pull/4258/head:syspath && git checkout syspath
RUN pip install astroid isort pytest mccabe pytest-benchmark
RUN bash -c "PYTHONPATH=:$(pwd) pytest -vv"

>           assert sys.path == paths[1:]
E           AssertionError: assert ['/usr/local/lib/python3.9',\n '/usr/local/lib/python3.9/lib-dynload',\n '/usr/local/lib/python3.9/site-packages'] == ['/usr/local/lib/python39.zip',\n '/usr/local/lib/python3.9',\n '/usr/local/lib/python3.9/lib-dynload',\n '/usr/local/lib/python3.9/site-packages']
E             At index 0 diff: '/usr/local/lib/python3.9' != '/usr/local/lib/python39.zip'
E             Right contains one more item: '/usr/local/lib/python3.9/site-packages'
E             Full diff:
E               [
E             -  '/usr/local/lib/python39.zip',
E                '/usr/local/lib/python3.9',
E                '/usr/local/lib/python3.9/lib-dynload',
E                '/usr/local/lib/python3.9/site-packages',
E               ]

tests/test_self.py:753: AssertionError

[cdce8p]

@Pierre-Sassoulas I'm currently working on it 😅
@sbraz Thanks for testing. Thought I did test it, but I guess I'll have another look.

[Pierre-Sassoulas]

Oups, sorry ! Do you mind if I create an integration test (I think I can create a new file that should not conflict) ?

[cdce8p]

Go for it, but I think I already have one.

    @staticmethod
    def test_import_plugin_from_local_directory_if_pythonpath_cwd(tmpdir):
        p_plugin = tmpdir / "plugin.py"
        p_plugin.write("# Some plugin content")

        with tmpdir.as_cwd():
            orig_pythonpath = os.environ.get("PYTHONPATH")
            os.environ["PYTHONPATH"] = "."
            process = subprocess.run(
                [
                    sys.executable,
                    "-m",
                    "pylint",
                    "--load-plugins",
                    "plugin",
                ],
                cwd=str(tmpdir),
                stderr=subprocess.PIPE,
            )
            assert "AttributeError: module 'plugin' has no attribute 'register'" \
                in process.stderr.decode()
            if orig_pythonpath:
                os.environ["PYTHONPATH"] = orig_pythonpath
            else:
                del os.environ["PYTHONPATH"]

Added in test_self.py

[Pierre-Sassoulas]

Ho, my bad I did not notice !

[cdce8p]

I haven't pushed it yet. So you couldn't have known.

[Pierre-Sassoulas]

I called mine pylint_plugin.py based on the issue, instead of plugin.py but we had pretty much the same code :D

[cdce8p]

@Pierre-Sassoulas If thats ok with you, I would like to push my changes instead of yours (I would take care of that). That way I could also address the PYTHONPATH issue directly.

[cdce8p]

As far as I can tell it works now. @sbraz You need to add RUN pip install -e . before running the test.

[cdce8p]

@sbraz If that doesn't work for you, we would have to take another look. We just merged it now, since we are planning a new release soon (later today / tomorrow) and it should (hopefully) be a working state already.

[sbraz]

Yeah that won't work for me because in the Gentoo ebuild, we need to load stuff from PYTHONPATH, which you explicitly set to ..
I'll probably just skip that new test_import_plugin_from_local_directory_if_pythonpath_cwd test.

[Pierre-Sassoulas]

Do you have an idea on how we could fix this for you @sbraz ? We have a lot of chores to do before being able to release, if there is a simple solution we can wait.

[cdce8p]

I have one last idea. I wanted to use

os.environ["PYTHONPATH"] = f"{(orig_pythonpath or '').strip(':')}:."

originally, but that caused the windows tests to fail.
We could do something like that:

            orig_pythonpath = os.environ.get("PYTHONPATH")
            if sys.platform == 'win32':
                os.environ["PYTHONPATH"] = "."
            else:
                os.environ["PYTHONPATH"] = f"{(orig_pythonpath or '').strip(':')}:."

Not pretty, but it might just work.

[sbraz]

I don't think it's worth the hassle to be honest. Thanks a lot for your help.

[cdce8p]

It's just a test, so if it would help maybe someone else uses it too.