Warn user when installing scripts outside PATH (#4553)

news/4553.feature

@@ -0,0 +1 @@
+pip now displays a warning when it installs scripts from a wheel outside the PATH. These warnings can be suppressed using a new --no-warn-script-location option.

src/pip/_internal/commands/install.py

@@ -158,6 +158,14 @@ def __init__(self, *args, **kw):
             help="Do not compile Python source files to bytecode",
         )
 
+        cmd_opts.add_option(
+            "--no-warn-script-location",
+            action="store_false",
+            dest="warn_script_location",
+            default=True,
+            help="Do not warn when installing scripts outside PATH",
+        )
+
         cmd_opts.add_option(cmdoptions.no_binary())
         cmd_opts.add_option(cmdoptions.only_binary())
         cmd_opts.add_option(cmdoptions.no_clean())
@@ -290,6 +298,7 @@ def run(self, options, args):
                         global_options,
                         root=options.root_path,
                         prefix=options.prefix_path,
+                        warn_script_location=options.warn_script_location,
                     )
 
                     possible_lib_locations = get_lib_location_guesses(

src/pip/_internal/req/req_install.py

@@ -725,7 +725,7 @@ def match_markers(self, extras_requested=None):
             return True
 
     def install(self, install_options, global_options=None, root=None,
-                prefix=None):
+                prefix=None, warn_script_location=True):
         global_options = global_options if global_options is not None else []
         if self.editable:
             self.install_editable(
@@ -735,7 +735,10 @@ def install(self, install_options, global_options=None, root=None,
             version = wheel.wheel_version(self.source_dir)
             wheel.check_compatibility(version, self.name)
 
-            self.move_wheel_files(self.source_dir, root=root, prefix=prefix)
+            self.move_wheel_files(
+                self.source_dir, root=root, prefix=prefix,
+                warn_script_location=warn_script_location,
+            )
             self.install_succeeded = True
             return
 
@@ -927,7 +930,8 @@ def check_if_exists(self):
     def is_wheel(self):
         return self.link and self.link.is_wheel
 
-    def move_wheel_files(self, wheeldir, root=None, prefix=None):
+    def move_wheel_files(self, wheeldir, root=None, prefix=None,
+                         warn_script_location=True):
         move_wheel_files(
             self.name, self.req, wheeldir,
             user=self.use_user_site,
@@ -936,6 +940,7 @@ def move_wheel_files(self, wheeldir, root=None, prefix=None):
             prefix=prefix,
             pycompile=self.pycompile,
             isolated=self.isolated,
+            warn_script_location=warn_script_location,
         )
 
     def get_dist(self):

src/pip/_internal/wheel.py

@@ -3,6 +3,7 @@
 """
 from __future__ import absolute_import
 
+import collections
 import compileall
 import copy
 import csv
@@ -37,8 +38,12 @@
 )
 from pip._internal.utils.setuptools_build import SETUPTOOLS_SHIM
 from pip._internal.utils.temp_dir import TempDirectory
+from pip._internal.utils.typing import MYPY_CHECK_RUNNING
 from pip._internal.utils.ui import open_spinner
 
+if MYPY_CHECK_RUNNING:
+    from typing import Dict, List, Optional
+
 wheel_ext = '.whl'
 
 VERSION_COMPATIBLE = (1, 0)
@@ -140,8 +145,64 @@ def _split_ep(s):
     return console, gui
 
 
+def message_about_scripts_not_on_PATH(scripts):
+    # type: (List[str]) -> Optional[str]
+    """Determine if any scripts are not on PATH and format a warning.
+
+    Returns a warning message if one or more scripts are not on PATH,
+    otherwise None.
+    """
+    if not scripts:
+        return None
+
+    # Group scripts by the path they were installed in
+    grouped_by_dir = collections.defaultdict(set)  # type: Dict[str, set]
+    for destfile in scripts:
+        parent_dir = os.path.dirname(destfile)
+        script_name = os.path.basename(destfile)
+        grouped_by_dir[parent_dir].add(script_name)
+
+    path_env_var_parts = os.environ["PATH"].split(os.pathsep)
+    # Warn only for directories that are not on PATH
+    warn_for = {
+        parent_dir: scripts for parent_dir, scripts in grouped_by_dir.items()
+        if parent_dir not in path_env_var_parts
+    }
+    if not warn_for:
+        return None
+
+    # Format a message
+    msg_lines = []
+    for parent_dir, scripts in warn_for.items():
+        scripts = sorted(scripts)
+        if len(scripts) == 1:
+            start_text = "script {} is".format(scripts[0])
+        else:
+            start_text = "scripts {} are".format(
+                ", ".join(scripts[:-1]) + " and " + scripts[-1]
+            )
+
+        msg_lines.append(
+            "The {} installed in '{}' which is not on PATH."
+            .format(start_text, parent_dir)
+        )
+
+    last_line_fmt = (
+        "Consider adding {} to PATH or, if you prefer "
+        "to suppress this warning, use --no-warn-script-location."
+    )
+    if len(msg_lines) == 1:
+        msg_lines.append(last_line_fmt.format("this directory"))
+    else:
+        msg_lines.append(last_line_fmt.format("these directories"))
+
+    # Returns the formatted multiline message
+    return "\n".join(msg_lines)
+
+
 def move_wheel_files(name, req, wheeldir, user=False, home=None, root=None,
-                     pycompile=True, scheme=None, isolated=False, prefix=None):
+                     pycompile=True, scheme=None, isolated=False, prefix=None,
+                     warn_script_location=True):
     """Install a wheel"""
 
     if not scheme:
@@ -391,9 +452,16 @@ def _get_script_text(entry):
 
     # Generate the console and GUI entry points specified in the wheel
     if len(console) > 0:
-        generated.extend(
-            maker.make_multiple(['%s = %s' % kv for kv in console.items()])
+        generated_console_scripts = maker.make_multiple(
+            ['%s = %s' % kv for kv in console.items()]
         )
+        generated.extend(generated_console_scripts)
+
+        if warn_script_location:
+            msg = message_about_scripts_not_on_PATH(generated_console_scripts)
+            if msg is not None:
+                logger.warn(msg)
+
     if len(gui) > 0:
         generated.extend(
             maker.make_multiple(

tests/functional/test_install.py

@@ -1224,3 +1224,26 @@ def test_install_pep508_with_url_in_install_requires(script):
     res = script.pip('install', pkga_path, expect_error=True)
     assert "Direct url requirement " in res.stderr, str(res)
     assert "are not allowed for dependencies" in res.stderr, str(res)
+
+
+def test_installing_scripts_outside_path_prints_warning(script):
+    result = script.pip_install_local(
+        "--prefix", script.scratch_path, "script_wheel1", expect_error=True
+    )
+    assert "Successfully installed script-wheel1" in result.stdout, str(result)
+    assert "--no-warn-script-location" in result.stderr
+
+
+def test_installing_scripts_outside_path_can_suppress_warning(script):
+    result = script.pip_install_local(
+        "--prefix", script.scratch_path, "--no-warn-script-location",
+        "script_wheel1"
+    )
+    assert "Successfully installed script-wheel1" in result.stdout, str(result)
+    assert "--no-warn-script-location" not in result.stderr
+
+
+def test_installing_scripts_on_path_does_not_print_warning(script):
+    result = script.pip_install_local("script_wheel1")
+    assert "Successfully installed script-wheel1" in result.stdout, str(result)
+    assert "--no-warn-script-location" not in result.stderr

tests/unit/test_wheel.py

@@ -9,7 +9,6 @@
 from pip._internal.compat import WINDOWS
 from pip._internal.exceptions import InvalidWheelFilename, UnsupportedWheel
 from pip._internal.utils.misc import unpack_file
-
 from tests.lib import DATA_DIR
 
 
@@ -371,3 +370,86 @@ def test_skip_building_wheels(self, caplog):
             wb.build(Mock())
             assert "due to already being wheel" in caplog.text
             assert mock_build_one.mock_calls == []
+
+
+class TestMessageAboutScriptsNotOnPATH(object):
+
+    def _template(self, paths, scripts):
+        with patch.dict('os.environ', {'PATH': os.pathsep.join(paths)}):
+            return wheel.message_about_scripts_not_on_PATH(scripts)
+
+    def test_no_script(self):
+        retval = self._template(
+            paths=['/a/b', '/c/d/bin'],
+            scripts=[]
+        )
+        assert retval is None
+
+    def test_single_script__single_dir_not_on_PATH(self):
+        retval = self._template(
+            paths=['/a/b', '/c/d/bin'],
+            scripts=['/c/d/foo']
+        )
+        assert retval is not None
+        assert "--no-warn-script-location" in retval
+        assert "foo is installed in '/c/d'" in retval
+
+    def test_two_script__single_dir_not_on_PATH(self):
+        retval = self._template(
+            paths=['/a/b', '/c/d/bin'],
+            scripts=['/c/d/foo', '/c/d/baz']
+        )
+        assert retval is not None
+        assert "--no-warn-script-location" in retval
+        assert "baz and foo are installed in '/c/d'" in retval
+
+    def test_multi_script__multi_dir_not_on_PATH(self):
+        retval = self._template(
+            paths=['/a/b', '/c/d/bin'],
+            scripts=['/c/d/foo', '/c/d/bar', '/c/d/baz', '/a/b/c/spam']
+        )
+        assert retval is not None
+        assert "--no-warn-script-location" in retval
+        assert "bar, baz and foo are installed in '/c/d'" in retval
+        assert "spam is installed in '/a/b/c'" in retval
+
+    def test_multi_script_all__multi_dir_not_on_PATH(self):
+        retval = self._template(
+            paths=['/a/b', '/c/d/bin'],
+            scripts=[
+                '/c/d/foo', '/c/d/bar', '/c/d/baz',
+                '/a/b/c/spam', '/a/b/c/eggs'
+            ]
+        )
+        assert retval is not None
+        assert "--no-warn-script-location" in retval
+        assert "bar, baz and foo are installed in '/c/d'" in retval
+        assert "eggs and spam are installed in '/a/b/c'" in retval
+
+    def test_two_script__single_dir_on_PATH(self):
+        retval = self._template(
+            paths=['/a/b', '/c/d/bin'],
+            scripts=['/a/b/foo', '/a/b/baz']
+        )
+        assert retval is None
+
+    def test_multi_script__multi_dir_on_PATH(self):
+        retval = self._template(
+            paths=['/a/b', '/c/d/bin'],
+            scripts=['/a/b/foo', '/a/b/bar', '/a/b/baz', '/c/d/bin/spam']
+        )
+        assert retval is None
+
+    def test_multi_script__single_dir_on_PATH(self):
+        retval = self._template(
+            paths=['/a/b', '/c/d/bin'],
+            scripts=['/a/b/foo', '/a/b/bar', '/a/b/baz']
+        )
+        assert retval is None
+
+    def test_single_script__single_dir_on_PATH(self):
+        retval = self._template(
+            paths=['/a/b', '/c/d/bin'],
+            scripts=['/a/b/foo']
+        )
+        assert retval is None