diff --git a/news/4553.feature b/news/4553.feature new file mode 100644 index 00000000000..ca5732bcc6b --- /dev/null +++ b/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. diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index 55322d57626..cf6c69870e5 100644 --- a/src/pip/_internal/commands/install.py +++ b/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( diff --git a/src/pip/_internal/req/req_install.py b/src/pip/_internal/req/req_install.py index c53c8255c9e..948cea5832d 100644 --- a/src/pip/_internal/req/req_install.py +++ b/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): diff --git a/src/pip/_internal/wheel.py b/src/pip/_internal/wheel.py index a38e16eb949..cb9ad999c23 100644 --- a/src/pip/_internal/wheel.py +++ b/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( diff --git a/tests/functional/test_install.py b/tests/functional/test_install.py index dee56c7d7ce..2935fb2e48c 100644 --- a/tests/functional/test_install.py +++ b/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 diff --git a/tests/unit/test_wheel.py b/tests/unit/test_wheel.py index 6a35941352a..dcc092e7345 100644 --- a/tests/unit/test_wheel.py +++ b/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