Merge pull request #8556 from chrahunt/maint/fail-on-install-location…

…-options

Disallow explicitly passing install-location-related arguments in --install-options

news/7309.removal

@@ -0,0 +1 @@
+Disallow passing install-location-related arguments in ``--install-options``.

src/pip/_internal/commands/install.py

@@ -21,7 +21,6 @@
 from pip._internal.operations.check import check_install_conflicts
 from pip._internal.req import install_given_reqs
 from pip._internal.req.req_tracker import get_requirement_tracker
-from pip._internal.utils.deprecation import deprecated
 from pip._internal.utils.distutils_args import parse_distutils_args
 from pip._internal.utils.filesystem import test_writable_dir
 from pip._internal.utils.misc import (
@@ -290,7 +289,7 @@ def run(self, options, args):
         try:
             reqs = self.get_requirements(args, options, finder, session)
 
-            warn_deprecated_install_options(
+            reject_location_related_install_options(
                 reqs, options.install_options
             )
 
@@ -606,7 +605,7 @@ def decide_user_install(
     return True
 
 
-def warn_deprecated_install_options(requirements, options):
+def reject_location_related_install_options(requirements, options):
     # type: (List[InstallRequirement], Optional[List[str]]) -> None
     """If any location-changing --install-option arguments were passed for
     requirements or on the command-line, then show a deprecation warning.
@@ -639,20 +638,12 @@ def format_options(option_names):
     if not offenders:
         return
 
-    deprecated(
-        reason=(
-            "Location-changing options found in --install-option: {}. "
-            "This configuration may cause unexpected behavior and is "
-            "unsupported.".format(
-                "; ".join(offenders)
-            )
-        ),
-        replacement=(
-            "using pip-level options like --user, --prefix, --root, and "
-            "--target"
-        ),
-        gone_in="20.2",
-        issue=7309,
+    raise CommandError(
+        "Location-changing options found in --install-option: {}."
+        " This is unsupported, use pip-level options like --user,"
+        " --prefix, --root, and --target instead.".format(
+            "; ".join(offenders)
+        )
     )
 
 

tests/functional/test_install_reqs.py

@@ -1,10 +1,12 @@
+import json
 import os
 import textwrap
 
 import pytest
 
 from tests.lib import (
     _create_test_package_with_subdirectory,
+    create_basic_sdist_for_package,
     create_basic_wheel_for_package,
     need_svn,
     path_to_url,
@@ -15,6 +17,51 @@
 from tests.lib.path import Path
 
 
+class ArgRecordingSdist(object):
+    def __init__(self, sdist_path, args_path):
+        self.sdist_path = sdist_path
+        self._args_path = args_path
+
+    def args(self):
+        return json.loads(self._args_path.read_text())
+
+
+@pytest.fixture()
+def arg_recording_sdist_maker(script):
+    arg_writing_setup_py = textwrap.dedent(
+        """
+        import io
+        import json
+        import os
+        import sys
+
+        from setuptools import setup
+
+        args_path = os.path.join(os.environ["OUTPUT_DIR"], "{name}.json")
+        with open(args_path, 'w') as f:
+            json.dump(sys.argv, f)
+
+        setup(name={name!r}, version="0.1.0")
+        """
+    )
+    output_dir = script.scratch_path.joinpath(
+        "args_recording_sdist_maker_output"
+    )
+    output_dir.mkdir(parents=True)
+    script.environ["OUTPUT_DIR"] = str(output_dir)
+
+    def _arg_recording_sdist_maker(name):
+        # type: (str) -> ArgRecordingSdist
+        extra_files = {"setup.py": arg_writing_setup_py.format(name=name)}
+        sdist_path = create_basic_sdist_for_package(
+            script, name, "0.1.0", extra_files
+        )
+        args_path = output_dir / "{}.json".format(name)
+        return ArgRecordingSdist(sdist_path, args_path)
+
+    return _arg_recording_sdist_maker
+
+
 @pytest.mark.network
 def test_requirements_file(script):
     """
@@ -259,28 +306,21 @@ def test_wheel_user_with_prefix_in_pydistutils_cfg(
     assert 'installed requiresupper' in result.stdout
 
 
-def test_install_option_in_requirements_file(script, data, virtualenv):
-    """
-    Test --install-option in requirements file overrides same option in cli
-    """
-
-    script.scratch_path.joinpath("home1").mkdir()
-    script.scratch_path.joinpath("home2").mkdir()
-
-    script.scratch_path.joinpath("reqs.txt").write_text(
-        textwrap.dedent(
-            """simple --install-option='--home={home}'""".format(
-                home=script.scratch_path.joinpath("home1"))))
+def test_install_option_in_requirements_file_overrides_cli(
+    script, arg_recording_sdist_maker
+):
+    simple_sdist = arg_recording_sdist_maker("simple")
 
-    result = script.pip(
-        'install', '--no-index', '-f', data.find_links, '-r',
-        script.scratch_path / 'reqs.txt',
-        '--install-option=--home={home}'.format(
-            home=script.scratch_path.joinpath("home2")),
-        expect_stderr=True)
+    reqs_file = script.scratch_path.joinpath("reqs.txt")
+    reqs_file.write_text("simple --install-option='-O0'")
 
-    package_dir = script.scratch / 'home1' / 'lib' / 'python' / 'simple'
-    result.did_create(package_dir)
+    script.pip(
+        'install', '--no-index', '-f', str(simple_sdist.sdist_path.parent),
+        '-r', str(reqs_file), '--install-option=-O1',
+    )
+    simple_args = simple_sdist.args()
+    assert 'install' in simple_args
+    assert simple_args.index('-O1') < simple_args.index('-O0')
 
 
 def test_constraints_not_installed_by_default(script, data):
@@ -605,35 +645,49 @@ def test_install_unsupported_wheel_file(script, data):
     assert len(result.files_created) == 0
 
 
-def test_install_options_local_to_package(script, data):
+def test_install_options_local_to_package(script, arg_recording_sdist_maker):
     """Make sure --install-options does not leak across packages.
 
     A requirements.txt file can have per-package --install-options; these
     should be isolated to just the package instead of leaking to subsequent
     packages.  This needs to be a functional test because the bug was around
     cross-contamination at install time.
     """
-    home_simple = script.scratch_path.joinpath("for-simple")
-    test_simple = script.scratch.joinpath("for-simple")
-    home_simple.mkdir()
+
+    simple1_sdist = arg_recording_sdist_maker("simple1")
+    simple2_sdist = arg_recording_sdist_maker("simple2")
+
     reqs_file = script.scratch_path.joinpath("reqs.txt")
     reqs_file.write_text(
-        textwrap.dedent("""
-            simple --install-option='--home={home_simple}'
-            INITools
-            """.format(**locals())))
-    result = script.pip(
+        textwrap.dedent(
+            """
+            simple1 --install-option='-O0'
+            simple2
+            """
+        )
+    )
+    script.pip(
         'install',
-        '--no-index', '-f', data.find_links,
+        '--no-index', '-f', str(simple1_sdist.sdist_path.parent),
         '-r', reqs_file,
-        expect_stderr=True,
     )
 
-    simple = test_simple / 'lib' / 'python' / 'simple'
-    bad = test_simple / 'lib' / 'python' / 'initools'
-    good = script.site_packages / 'initools'
-    result.did_create(simple)
-    assert result.files_created[simple].dir
-    result.did_not_create(bad)
-    result.did_create(good)
-    assert result.files_created[good].dir
+    simple1_args = simple1_sdist.args()
+    assert 'install' in simple1_args
+    assert '-O0' in simple1_args
+    simple2_args = simple2_sdist.args()
+    assert 'install' in simple2_args
+    assert '-O0' not in simple2_args
+
+
+def test_location_related_install_option_fails(script):
+    simple_sdist = create_basic_sdist_for_package(script, "simple", "0.1.0")
+    reqs_file = script.scratch_path.joinpath("reqs.txt")
+    reqs_file.write_text("simple --install-option='--home=/tmp'")
+    result = script.pip(
+        'install',
+        '--no-index', '-f', str(simple_sdist.parent),
+        '-r', reqs_file,
+        expect_error=True
+    )
+    assert "['--home'] from simple" in result.stderr

tests/unit/test_command_install.py

@@ -7,8 +7,9 @@
 from pip._internal.commands.install import (
     create_env_error_message,
     decide_user_install,
-    warn_deprecated_install_options,
+    reject_location_related_install_options,
 )
+from pip._internal.exceptions import CommandError
 from pip._internal.req.req_install import InstallRequirement
 
 
@@ -44,16 +45,15 @@ def test_most_cases(
         assert decide_user_install(use_user_site=None) is result
 
 
-def test_deprecation_notice_for_pip_install_options(recwarn):
+def test_rejection_for_pip_install_options():
     install_options = ["--prefix=/hello"]
-    warn_deprecated_install_options([], install_options)
+    with pytest.raises(CommandError) as e:
+        reject_location_related_install_options([], install_options)
 
-    assert len(recwarn) == 1
-    message = recwarn[0].message.args[0]
-    assert "['--prefix'] from command line" in message
+    assert "['--prefix'] from command line" in str(e.value)
 
 
-def test_deprecation_notice_for_requirement_options(recwarn):
+def test_rejection_for_location_requirement_options():
     install_options = []
 
     bad_named_req_options = ["--home=/wow"]
@@ -67,18 +67,16 @@ def test_deprecation_notice_for_requirement_options(recwarn):
         None, "requirements2.txt", install_options=bad_unnamed_req_options
     )
 
-    warn_deprecated_install_options(
-        [bad_named_req, bad_unnamed_req], install_options
-    )
-
-    assert len(recwarn) == 1
-    message = recwarn[0].message.args[0]
+    with pytest.raises(CommandError) as e:
+        reject_location_related_install_options(
+            [bad_named_req, bad_unnamed_req], install_options
+        )
 
     assert (
         "['--install-lib'] from <InstallRequirement> (from requirements2.txt)"
-        in message
+        in str(e.value)
     )
-    assert "['--home'] from hello (from requirements.txt)" in message
+    assert "['--home'] from hello (from requirements.txt)" in str(e.value)
 
 
 @pytest.mark.parametrize('error, show_traceback, using_user_site, expected', [