Merge pull request #5884 from cjerdonek/pip-no-cache-dir-var

Make PIP_NO_CACHE_DIR behave as it reads, and not crash pip

news/5385.bugfix

@@ -0,0 +1 @@
+Setting ``PIP_NO_CACHE_DIR=yes`` no longer causes pip to crash.

news/5735.feature

@@ -0,0 +1,2 @@
+Make ``PIP_NO_CACHE_DIR`` disable the cache also for truthy values like
+``"true"``, ``"yes"``, ``"1"``, etc.

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
 
@@ -537,11 +538,37 @@ def prefer_binary():
     help="Store the cache data in <dir>."
 )  # type: partial[Option]
 
+
+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.",
 )  # type: partial[Option]
 

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'])