Repository: pypa/pip
Pull request: Fix bypassed pip upgrade warning on Windows
Fixes #6841 

Problem statement:
pip upgrade warning is bypassed when pip is a dependency
Helping to debug a contributor setup issue at the PyCon AU sprints, we eventually tracked the problem down to:

* briefcase needed a more recent pip
* "pip install --upgrade briefcase" implicitly tried to upgrade pip, but had the usual Windows issues with trying to replace a running executable
* the warning about this case didn't get displayed

Hints from issue discussion:
Yeah, the heuristics for working out when we're trying to use pip to upgrade itself aren't 100% reliable. I don't think anyone would object to improved detection of this case, but we don't know how to do it. Suggestions (and working code :slightly_smiling_face:) welcome, of course!
Hmm... We're checking if "pip" is in the requirement set, which should be robust to this.

I guess there's something about this assumption when I wrote that, that's not correct. 🙃
I think it's that pip is not in the explicit requirement set, but is pulled in as a dependency when installing one of the requested requirements. I don't think we can do much about that, as we only find out once the install process has started (although I may well be wrong, I'm going from a pretty vague memory of how it works...)
Right. We're both speculating from memory. XD

I've fallen for this before so... let's wait till someone finds the time to investigate what's actually happening.
I've tried to reproduce the issue on AppVeyor and found that adding "pip" to the list below fixes easily the issue:

https://github.com/pypa/pip/blob/f377148f6de201c6f0e23baea340e0be69518325/src/pip/_internal/utils/misc.py#L1189-L1193

Checkout my test case and AppVeyor log: 
https://github.com/atugushev/test-package/blob/master/.appveyor.yml
https://ci.appveyor.com/project/atugushev/test-package/builds/26548167

Tried on https://github.com/atugushev/pip/commit/84625807f10e8639c32c2ef3596bbf8cf7441f0f

Commit message: Address review comments

Rebased to the latest master
Remove unneeded assertions
Use create_basic_wheel_for_package
Couple other nitpicks

Patch to review:
diff --git a/news/6841.bugfix b/news/6841.bugfix
new file mode 100644
index 00000000000..278caa64e54
--- /dev/null
+++ b/news/6841.bugfix
@@ -0,0 +1 @@
+Fix bypassed pip upgrade warning on Windows.
diff --git a/src/pip/_internal/utils/misc.py b/src/pip/_internal/utils/misc.py
index 576a25138ed..2a6735b7f4a 100644
--- a/src/pip/_internal/utils/misc.py
+++ b/src/pip/_internal/utils/misc.py
@@ -1184,11 +1184,11 @@ def protect_pip_from_modification_on_windows(modifying_pip):
     On Windows, any operation modifying pip should be run as:
         python -m pip ...
     """
-    pip_names = [
-        "pip.exe",
-        "pip{}.exe".format(sys.version_info[0]),
-        "pip{}.{}.exe".format(*sys.version_info[:2])
-    ]
+    pip_names = set()
+    for ext in ('', '.exe'):
+        pip_names.add('pip{ext}'.format(ext=ext))
+        pip_names.add('pip{}{ext}'.format(sys.version_info[0], ext=ext))
+        pip_names.add('pip{}.{}{ext}'.format(*sys.version_info[:2], ext=ext))
 
     # See https://github.com/pypa/pip/issues/1299 for more discussion
     should_show_use_python_msg = (
diff --git a/tests/functional/test_install.py b/tests/functional/test_install.py
index 6e1e3dd400b..ac8aad9a2ae 100644
--- a/tests/functional/test_install.py
+++ b/tests/functional/test_install.py
@@ -7,6 +7,7 @@
 
 import pytest
 
+from pip import __version__ as pip_current_version
 from pip._internal import pep425tags
 from pip._internal.cli.status_codes import ERROR, SUCCESS
 from pip._internal.models.index import PyPI, TestPyPI
@@ -1527,3 +1528,87 @@ def test_target_install_ignores_distutils_config_install_prefix(script):
         (target - script.base_path) in result.files_created and
         (prefix - script.base_path) not in result.files_created
     ), str(result)
+
+
+@pytest.mark.network
+@pytest.mark.skipif("sys.platform != 'win32'")
+@pytest.mark.parametrize('pip_name', [
+    'pip',
+    'pip{}'.format(sys.version_info[0]),
+    'pip{}.{}'.format(*sys.version_info[:2]),
+    'pip.exe',
+    'pip{}.exe'.format(sys.version_info[0]),
+    'pip{}.{}.exe'.format(*sys.version_info[:2])
+])
+def test_protect_pip_from_modification_on_windows(script, pip_name):
+    """
+    Test that pip modification command using ``pip install ...``
+    raises an error on Windows.
+    """
+    command = [pip_name, 'install', 'pip != {}'.format(pip_current_version)]
+    result = script.run(*command, expect_error=True)
+    new_command = [sys.executable, '-m', 'pip'] + command[1:]
+    expected_message = (
+        'To modify pip, please run the following command:\n{}'
+        .format(' '.join(new_command))
+    )
+    assert expected_message in result.stderr, str(result)
+
+
+@pytest.mark.network
+@pytest.mark.skipif("sys.platform != 'win32'")
+def test_protect_pip_from_modification_via_deps_on_windows(script):
+    """
+    Test ``pip install pkga`` is raised an error on Windows
+    if `pkga` implicitly tries to upgrade pip.
+    """
+    pkga_wheel_path = create_basic_wheel_for_package(
+        script,
+        'pkga', '0.1',
+        depends=['pip != {}'.format(pip_current_version)],
+    )
+
+    # Make sure pip install pkga is raised an error
+    command = ['pip', 'install', pkga_wheel_path]
+    result = script.run(*command, expect_error=True)
+    new_command = [sys.executable, "-m"] + command
+    expected_message = (
+        'To modify pip, please run the following command:\n{}'
+        .format(' '.join(new_command))
+    )
+    assert expected_message in result.stderr, str(result)
+
+
+@pytest.mark.network
+@pytest.mark.skipif("sys.platform != 'win32'")
+def test_protect_pip_from_modification_via_sub_deps_on_windows(script):
+    """
+    Test ``pip install pkga`` is raised an error on Windows
+    if sub-dependencies of `pkga` implicitly tries to upgrade pip.
+    """
+    # Make a wheel for pkga which requires pkgb
+    pkga_wheel_path = create_basic_wheel_for_package(
+        script,
+        'pkga', '0.1',
+        depends=['pkgb'],
+    )
+
+    # Make a wheel for pkgb which requires pip
+    pkgb_wheel_path = create_basic_wheel_for_package(
+        script,
+        'pkgb', '0.1',
+        depends=['pip != {}'.format(pip_current_version)],
+    )
+
+    # Make sure pip install pkga is raised an error
+    command = [
+        'pip', 'install', pkga_wheel_path,
+        '--find-links', pkgb_wheel_path.parent,
+    ]
+    result = script.run(*command, expect_error=True)
+    new_command = [sys.executable, '-m'] + command
+    expected_message = (
+        'To modify pip, please run the following command:\n{}'
+        .format(' '.join(new_command))
+    )
+    assert expected_message in result.stderr, str(result)


Code review comments:
diff_hunk: @@ -1541,86 +1542,73 @@ def test_target_install_ignores_distutils_config_install_prefix(script):
 ])
 def test_protect_pip_from_modification_on_windows(script, pip_name):
     """
-    Test ``pip install --upgrade pip`` is raised an error on Windows.
+    Test that pip modification command using ``pip install ...``
+    raises an error on Windows.
     """
-    command = [pip_name, 'install', '--upgrade', 'pip']
+    command = [pip_name, 'install', 'pip != {}'.format(pip_current_version)]
     result = script.run(*command, expect_error=True)
-    assert result.returncode != 0
     new_command = [sys.executable, '-m', 'pip'] + command[1:]
-    assert 'To modify pip, please run the following command:\n{}'.format(
-        ' '.join(new_command)) in result.stderr, str(result)
+    expected_message = (
+        'To modify pip, please run the following command:\n{}'
+        .format(' '.join(new_command))
+    )
+    assert expected_message in result.stderr, str(result)
 
 
+@pytest.mark.network
 @pytest.mark.skipif("sys.platform != 'win32'")
-def test_protect_pip_from_modification_via_deps_on_windows(script, with_wheel):
+def test_protect_pip_from_modification_via_deps_on_windows(script):
     """
     Test ``pip install pkga`` is raised an error on Windows
     if `pkga` implicitly tries to upgrade pip.
     """
-    # Make a wheel for pkga which requires pip
-    script.scratch_path.joinpath('pkga').mkdir()
-    pkga_path = script.scratch_path / 'pkga'
-    pkga_path.joinpath('setup.py').write_text(textwrap.dedent("""
-        from setuptools import setup
-        setup(name='pkga',
-              version='0.1',
-              install_requires = ["pip<{}"])
-    """.format(pip.__version__)))
-    script.run(
-        'python', 'setup.py', 'bdist_wheel', '--universal', cwd=pkga_path
+    pkga_wheel_path = create_basic_wheel_for_package(
+        script,
+        'pkga', '0.1',
+        depends=['pip != {}'.format(pip_current_version)],
     )
 
-    # Make sure pip install pkga is raised error
-    pkga_wheel_path = './pkga/dist/pkga-0.1-py2.py3-none-any.whl'
+    # Make sure pip install pkga is raised an error
path: tests/functional/test_install.py
text: @user1:
minor typo: "is raised" -> "raises"