From c0cc004ca82f14a19282f6e742ff2ac976febcba Mon Sep 17 00:00:00 2001 From: Chris Jerdonek Date: Sun, 13 Jan 2019 07:59:17 -0800 Subject: [PATCH 1/2] Raise an error if the user tries to use PIP_NO_USE_PEP517. --- src/pip/_internal/cli/cmdoptions.py | 42 +++++++++- tests/unit/test_options.py | 117 ++++++++++++++++++++++++++++ 2 files changed, 158 insertions(+), 1 deletion(-) diff --git a/src/pip/_internal/cli/cmdoptions.py b/src/pip/_internal/cli/cmdoptions.py index 0aebf75e834..3000b078d31 100644 --- a/src/pip/_internal/cli/cmdoptions.py +++ b/src/pip/_internal/cli/cmdoptions.py @@ -9,6 +9,7 @@ """ from __future__ import absolute_import +import textwrap import warnings from distutils.util import strtobool from functools import partial @@ -28,6 +29,20 @@ from pip._internal.cli.parser import ConfigOptionParser # noqa: F401 +def raise_option_error(parser, option, msg): + """ + Raise an option parsing error using parser.error(). + + Args: + parser: an OptionParser instance. + option: an Option instance. + msg: the error text. + """ + msg = '{} error: {}'.format(option, msg) + msg = textwrap.fill(' '.join(msg.split())) + parser.error(msg) + + def make_option_group(group, parser): # type: (Dict[str, Any], ConfigOptionParser) -> OptionGroup """ @@ -601,6 +616,30 @@ def no_cache_dir_callback(option, opt, value, parser): 'if this option is used.' ) # type: Callable[..., Option] + +def no_use_pep517_callback(option, opt, value, parser): + """ + Process a value provided for the --no-use-pep517 option. + + This is an optparse.Option callback for the no_use_pep517 option. + """ + # Since --no-use-pep517 doesn't accept arguments, the value argument + # will be None if --no-use-pep517 is passed via the command-line. + # However, the value can be non-None if the option is triggered e.g. + # by an environment variable, for example "PIP_NO_USE_PEP517=true". + if value is not None: + msg = """A value was passed for --no-use-pep517, + probably using either the PIP_NO_USE_PEP517 environment variable + or the "no-use-pep517" config file option. Use an appropriate value + of the PIP_USE_PEP517 environment variable or the "use-pep517" + config file option instead. + """ + raise_option_error(parser, option=option, msg=msg) + + # Otherwise, --no-use-pep517 was passed via the command-line. + parser.values.use_pep517 = False + + use_pep517 = partial( Option, '--use-pep517', @@ -615,7 +654,8 @@ def no_cache_dir_callback(option, opt, value, parser): Option, '--no-use-pep517', dest='use_pep517', - action='store_false', + action='callback', + callback=no_use_pep517_callback, default=None, help=SUPPRESS_HELP ) # type: Any diff --git a/tests/unit/test_options.py b/tests/unit/test_options.py index e0a0885b3b9..b8da7b073d3 100644 --- a/tests/unit/test_options.py +++ b/tests/unit/test_options.py @@ -5,9 +5,27 @@ import pip._internal.configuration from pip._internal import main +from pip._internal.commands import DownloadCommand from tests.lib.options_helpers import AddFakeCommandMixin +@contextmanager +def temp_environment_variable(name, value): + not_set = object() + original = os.environ[name] if name in os.environ else not_set + os.environ[name] = value + + try: + yield + finally: + # Return the environment variable to its original state. + if original is not_set: + if name in os.environ: + del os.environ[name] + else: + os.environ[name] = original + + @contextmanager def assert_raises_message(exc_class, expected): """ @@ -19,6 +37,22 @@ def assert_raises_message(exc_class, expected): assert str(excinfo.value) == expected +@contextmanager +def assert_option_error(capsys, expected): + """ + Assert that a SystemExit occurred because of a parsing error. + + Args: + expected: an expected substring of stderr. + """ + with pytest.raises(SystemExit) as excinfo: + yield + + assert excinfo.value.code == 2 + stderr = capsys.readouterr().err + assert expected in stderr + + def assert_is_default_cache_dir(value): # This path looks different on different platforms, but the path always # has the substring "pip". @@ -157,6 +191,89 @@ def test_cache_dir__PIP_NO_CACHE_DIR_invalid__with_no_cache_dir(self): main(['--no-cache-dir', 'fake']) +class TestUsePEP517Options(object): + + """ + Test options related to using --use-pep517. + """ + + def parse_args(self, args): + # We use DownloadCommand since that is one of the few Command + # classes with the use_pep517 options. + command = DownloadCommand() + options, args = command.parse_args(args) + + return options + + def test_no_option(self): + """ + Test passing no option. + """ + options = self.parse_args([]) + assert options.use_pep517 is None + + def test_use_pep517(self): + """ + Test passing --use-pep517. + """ + options = self.parse_args(['--use-pep517']) + assert options.use_pep517 is True + + def test_no_use_pep517(self): + """ + Test passing --no-use-pep517. + """ + options = self.parse_args(['--no-use-pep517']) + assert options.use_pep517 is False + + def test_PIP_USE_PEP517_true(self): + """ + Test setting PIP_USE_PEP517 to "true". + """ + with temp_environment_variable('PIP_USE_PEP517', 'true'): + options = self.parse_args([]) + # This is an int rather than a boolean because strtobool() in pip's + # configuration code returns an int. + assert options.use_pep517 == 1 + + def test_PIP_USE_PEP517_false(self): + """ + Test setting PIP_USE_PEP517 to "false". + """ + with temp_environment_variable('PIP_USE_PEP517', 'false'): + options = self.parse_args([]) + # This is an int rather than a boolean because strtobool() in pip's + # configuration code returns an int. + assert options.use_pep517 == 0 + + def test_use_pep517_and_PIP_USE_PEP517_false(self): + """ + Test passing --use-pep517 and setting PIP_USE_PEP517 to "false". + """ + with temp_environment_variable('PIP_USE_PEP517', 'false'): + options = self.parse_args(['--use-pep517']) + assert options.use_pep517 is True + + def test_no_use_pep517_and_PIP_USE_PEP517_true(self): + """ + Test passing --no-use-pep517 and setting PIP_USE_PEP517 to "true". + """ + with temp_environment_variable('PIP_USE_PEP517', 'true'): + options = self.parse_args(['--no-use-pep517']) + assert options.use_pep517 is False + + def test_PIP_NO_USE_PEP517(self, capsys): + """ + Test setting PIP_NO_USE_PEP517, which isn't allowed. + """ + expected_err = ( + '--no-use-pep517 error: A value was passed for --no-use-pep517,\n' + ) + with temp_environment_variable('PIP_NO_USE_PEP517', 'true'): + with assert_option_error(capsys, expected=expected_err): + self.parse_args([]) + + class TestOptionsInterspersed(AddFakeCommandMixin): def test_general_option_after_subcommand(self): From 5fe31579177dd3d221c41e1e6002cd831bb66a9e Mon Sep 17 00:00:00 2001 From: Chris Jerdonek Date: Sun, 13 Jan 2019 02:06:34 -0800 Subject: [PATCH 2/2] Change the --no-cache-dir error to use raise_option_error(). --- src/pip/_internal/cli/cmdoptions.py | 5 ++++- tests/unit/test_options.py | 18 +++++------------- 2 files changed, 9 insertions(+), 14 deletions(-) diff --git a/src/pip/_internal/cli/cmdoptions.py b/src/pip/_internal/cli/cmdoptions.py index 3000b078d31..5cf5ee970dd 100644 --- a/src/pip/_internal/cli/cmdoptions.py +++ b/src/pip/_internal/cli/cmdoptions.py @@ -555,7 +555,10 @@ def no_cache_dir_callback(option, opt, value, parser): # environment variable, like PIP_NO_CACHE_DIR=true. if value is not None: # Then parse the string value to get argument error-checking. - strtobool(value) + try: + strtobool(value) + except ValueError as exc: + raise_option_error(parser, option=option, msg=str(exc)) # Originally, setting PIP_NO_CACHE_DIR to a value that strtobool() # converted to 0 (like "false" or "no") caused cache_dir to be disabled diff --git a/tests/unit/test_options.py b/tests/unit/test_options.py index b8da7b073d3..3215a954038 100644 --- a/tests/unit/test_options.py +++ b/tests/unit/test_options.py @@ -26,17 +26,6 @@ def temp_environment_variable(name, value): os.environ[name] = original -@contextmanager -def assert_raises_message(exc_class, expected): - """ - Assert that an exception with the given type and message is raised. - """ - with pytest.raises(exc_class) as excinfo: - yield - - assert str(excinfo.value) == expected - - @contextmanager def assert_option_error(capsys, expected): """ @@ -181,13 +170,16 @@ def test_cache_dir__PIP_NO_CACHE_DIR__with_no_cache_dir( # value in this case). assert options.cache_dir is False - def test_cache_dir__PIP_NO_CACHE_DIR_invalid__with_no_cache_dir(self): + def test_cache_dir__PIP_NO_CACHE_DIR_invalid__with_no_cache_dir( + self, capsys, + ): """ Test setting PIP_NO_CACHE_DIR to an invalid value while also passing --no-cache-dir. """ os.environ['PIP_NO_CACHE_DIR'] = 'maybe' - with assert_raises_message(ValueError, "invalid truth value 'maybe'"): + expected_err = "--no-cache-dir error: invalid truth value 'maybe'" + with assert_option_error(capsys, expected=expected_err): main(['--no-cache-dir', 'fake'])