diff --git a/news/5385.bugfix b/news/5385.bugfix new file mode 100644 index 00000000000..b880318cf9a --- /dev/null +++ b/news/5385.bugfix @@ -0,0 +1 @@ +Setting ``PIP_NO_CACHE_DIR=yes`` no longer causes pip to crash. diff --git a/news/5735.feature b/news/5735.feature new file mode 100644 index 00000000000..823bdcb6dce --- /dev/null +++ b/news/5735.feature @@ -0,0 +1,2 @@ +Make ``PIP_NO_CACHE_DIR`` disable the cache also for truthy values like +``"true"``, ``"yes"``, ``"1"``, etc. diff --git a/src/pip/_internal/cli/cmdoptions.py b/src/pip/_internal/cli/cmdoptions.py index 3033cd4b5e6..f5578ad20bb 100644 --- a/src/pip/_internal/cli/cmdoptions.py +++ b/src/pip/_internal/cli/cmdoptions.py @@ -10,6 +10,7 @@ from __future__ import absolute_import import warnings +from distutils.util import strtobool from functools import partial from optparse import SUPPRESS_HELP, Option, OptionGroup @@ -520,11 +521,37 @@ def prefer_binary(): help="Store the cache data in ." ) + +def no_cache_dir_callback(option, opt, value, parser): + """ + Process a value provided for the --no-cache-dir option. + + This is an optparse.Option callback for the --no-cache-dir option. + """ + # The value argument will be None if --no-cache-dir is passed via the + # command-line, since the option doesn't accept arguments. However, + # the value can be non-None if the option is triggered e.g. by an + # 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) + + # Originally, setting PIP_NO_CACHE_DIR to a value that strtobool() + # converted to 0 (like "false" or "no") caused cache_dir to be disabled + # rather than enabled (logic would say the latter). Thus, we disable + # the cache directory not just on values that parse to True, but (for + # backwards compatibility reasons) also on values that parse to False. + # In other words, always set it to False if the option is provided in + # some (valid) form. + parser.values.cache_dir = False + + no_cache = partial( Option, "--no-cache-dir", dest="cache_dir", - action="store_false", + action="callback", + callback=no_cache_dir_callback, help="Disable the cache.", ) diff --git a/tests/unit/test_options.py b/tests/unit/test_options.py index b6a1bf85453..e0a0885b3b9 100644 --- a/tests/unit/test_options.py +++ b/tests/unit/test_options.py @@ -1,4 +1,5 @@ import os +from contextlib import contextmanager import pytest @@ -7,6 +8,23 @@ from tests.lib.options_helpers import AddFakeCommandMixin +@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 + + +def assert_is_default_cache_dir(value): + # This path looks different on different platforms, but the path always + # has the substring "pip". + assert 'pip' in value + + class TestOptionPrecedence(AddFakeCommandMixin): """ Tests for confirming our option precedence: @@ -81,6 +99,63 @@ def test_cli_override_environment(self): options, args = main(['fake', '--timeout', '-2']) assert options.timeout == -2 + @pytest.mark.parametrize('pip_no_cache_dir', [ + # Enabling --no-cache-dir means no cache directory. + '1', + 'true', + 'on', + 'yes', + # For historical / backwards compatibility reasons, we also disable + # the cache directory if provided a value that translates to 0. + '0', + 'false', + 'off', + 'no', + ]) + def test_cache_dir__PIP_NO_CACHE_DIR(self, pip_no_cache_dir): + """ + Test setting the PIP_NO_CACHE_DIR environment variable without + passing any command-line flags. + """ + os.environ['PIP_NO_CACHE_DIR'] = pip_no_cache_dir + options, args = main(['fake']) + assert options.cache_dir is False + + @pytest.mark.parametrize('pip_no_cache_dir', ['yes', 'no']) + def test_cache_dir__PIP_NO_CACHE_DIR__with_cache_dir( + self, pip_no_cache_dir + ): + """ + Test setting PIP_NO_CACHE_DIR while also passing an explicit + --cache-dir value. + """ + os.environ['PIP_NO_CACHE_DIR'] = pip_no_cache_dir + options, args = main(['--cache-dir', '/cache/dir', 'fake']) + # The command-line flag takes precedence. + assert options.cache_dir == '/cache/dir' + + @pytest.mark.parametrize('pip_no_cache_dir', ['yes', 'no']) + def test_cache_dir__PIP_NO_CACHE_DIR__with_no_cache_dir( + self, pip_no_cache_dir + ): + """ + Test setting PIP_NO_CACHE_DIR while also passing --no-cache-dir. + """ + os.environ['PIP_NO_CACHE_DIR'] = pip_no_cache_dir + options, args = main(['--no-cache-dir', 'fake']) + # The command-line flag should take precedence (which has the same + # value in this case). + assert options.cache_dir is False + + def test_cache_dir__PIP_NO_CACHE_DIR_invalid__with_no_cache_dir(self): + """ + 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'"): + main(['--no-cache-dir', 'fake']) + class TestOptionsInterspersed(AddFakeCommandMixin): @@ -106,6 +181,19 @@ class TestGeneralOptions(AddFakeCommandMixin): # the reason to specifically test general options is due to the # extra processing they receive, and the number of bugs we've had + def test_cache_dir__default(self): + options, args = main(['fake']) + # With no options the default cache dir should be used. + assert_is_default_cache_dir(options.cache_dir) + + def test_cache_dir__provided(self): + options, args = main(['--cache-dir', '/cache/dir', 'fake']) + assert options.cache_dir == '/cache/dir' + + def test_no_cache_dir__provided(self): + options, args = main(['--no-cache-dir', 'fake']) + assert options.cache_dir is False + def test_require_virtualenv(self): options1, args1 = main(['--require-virtualenv', 'fake']) options2, args2 = main(['fake', '--require-virtualenv'])