Fix python path discovery if not called python

- Begin a refactor of `delegator.run` invocation to ensure we capture
  and handle failures with our own exception wrappers
- Additoinally capture output and error logging and command information
  when running in verbose mode (should avoid significant repitition in
  the codebase)
- Refactor `which` and `system_which` to fallback to pythonfinder's
implementation
- Abstract `is_python_command` to identify whether we are looking for
  python, this enables us to rely on `pythonfinder.Finder.find_all_python_versions()`
  to ensure we aren't skipping python versions
- Fixes #2783

Signed-off-by: Dan Ryan <[email protected]>

news/2783.bugfix.rst

@@ -0,0 +1,2 @@
+Fixed an issue which caused errors due to reliance on the system utilities ``which`` and ``where`` which may not always exist on some systems.
+- Fixed a bug which caused periodic failures in python discovery when executables named ``python`` were not present on the target ``$PATH``.

pipenv/cmdparse.py

@@ -10,7 +10,7 @@ class ScriptEmptyError(ValueError):
 
 
 def _quote_if_contains(value, pattern):
-    if next(re.finditer(pattern, value), None):
+    if next(iter(re.finditer(pattern, value)), None):
         return '"{0}"'.format(re.sub(r'(\\*)"', r'\1\1\\"', value))
     return value
 

pipenv/core.py

@@ -39,9 +39,12 @@
     parse_indexes,
     escape_cmd,
     create_spinner,
-    get_canonical_names
+    get_canonical_names,
+    run_command,
+    is_python_command,
+    find_python
 )
-from . import environments, pep508checker, progress
+from . import environments, pep508checker, progress, exceptions
 from .environments import (
     PIPENV_COLORBLIND,
     PIPENV_SHELL_FANCY,
@@ -55,8 +58,7 @@
     PIPENV_CACHE_DIR,
     PIPENV_PYUP_API_KEY,
 )
-from ._compat import fix_utf8
-from . import exceptions
+from ._compat import fix_utf8, decode_output
 
 # Packages that should be ignored later.
 BAD_PACKAGES = (
@@ -106,16 +108,19 @@ def which(command, location=None, allow_global=False):
             location = os.environ.get("VIRTUAL_ENV", None)
     if not (location and os.path.exists(location)) and not allow_global:
         raise RuntimeError("location not created nor specified")
+
+    version_str = "python{0}".format(".".join([str(v) for v in sys.version_info[:2]]))
+    is_python = command in ("python", os.path.basename(sys.executable), version_str)
     if not allow_global:
         if os.name == "nt":
             p = find_windows_executable(os.path.join(location, "Scripts"), command)
         else:
             p = os.path.join(location, "bin", command)
     else:
-        if command == "python":
+        if is_python:
             p = sys.executable
     if not os.path.exists(p):
-        if command == "python":
+        if is_python:
             p = sys.executable or system_which("python")
         else:
             p = system_which(command)
@@ -343,26 +348,18 @@ def find_a_system_python(line):
     * Search for "python" and "pythonX.Y" executables in PATH to find a match.
     * Nothing fits, return None.
     """
-    if not line:
-        return None
+
     if os.path.isabs(line):
         return line
     from .vendor.pythonfinder import Finder
-
     finder = Finder(system=False, global_search=True)
+    if not line:
+        return next(iter(finder.find_all_python_versions()), None)
+    # Use the windows finder executable
     if (line.startswith("py ") or line.startswith("py.exe ")) and os.name == "nt":
         line = line.split(" ", 1)[1].lstrip("-")
-    elif line.startswith("py"):
-        python_entry = finder.which(line)
-        if python_entry:
-            return python_entry.path.as_posix()
-        return None
-    python_entry = finder.find_python_version(line)
-    if not python_entry:
-        python_entry = finder.which("python{0}".format(line))
-    if python_entry:
-        return python_entry.path.as_posix()
-    return None
+    python_entry = find_python(finder, line)
+    return python_entry
 
 
 def ensure_python(three=None, python=None):
@@ -1445,18 +1442,59 @@ def pip_download(package_name):
     return c
 
 
+def fallback_which(command, location=None, allow_global=False, system=False):
+    """
+    A fallback implementation of the `which` utility command that relies exclusively on
+    searching the path for commands.
+
+    :param str command: The command to search for, optional
+    :param str location: The search location to prioritize (prepend to path), defaults to None
+    :param bool allow_global: Whether to search the global path, defaults to False
+    :param bool system: Whether to use the system python instead of pipenv's python, defaults to False
+    :raises ValueError: Raised if no command is provided
+    :raises TypeError: Raised if the command provided is not a string
+    :return: A path to the discovered command location
+    :rtype: str
+    """
+
+    from .vendor.pythonfinder import Finder
+    if not command:
+        raise ValueError("fallback_which: Must provide a command to search for...")
+    if not isinstance(command, six.string_types):
+        raise TypeError("Provided command must be a string, received {0!r}".format(command))
+    global_search = system or allow_global
+    finder = Finder(system=False, global_search=global_search, path=location)
+    if is_python_command(command):
+        result = find_python(finder, command)
+        if result:
+            return result
+    result = finder.which(command)
+    if result:
+        return result.path.as_posix()
+    return ""
+
+
 def which_pip(allow_global=False):
     """Returns the location of virtualenv-installed pip."""
+
+    location = None
+    if "VIRTUAL_ENV" in os.environ:
+        location = os.environ["VIRTUAL_ENV"]
     if allow_global:
-        if "VIRTUAL_ENV" in os.environ:
-            return which("pip", location=os.environ["VIRTUAL_ENV"])
+        if location:
+            pip = which("pip", location=location)
+            if pip:
+                return pip
 
         for p in ("pip", "pip3", "pip2"):
             where = system_which(p)
             if where:
                 return where
 
-    return which("pip")
+    pip = which("pip")
+    if not pip:
+        pip = fallback_which("pip", allow_global=allow_global, location=location)
+    return pip
 
 
 def system_which(command, mult=False):
@@ -1466,6 +1504,7 @@ def system_which(command, mult=False):
         vistir.compat.fs_str(k): vistir.compat.fs_str(val)
         for k, val in os.environ.items()
     }
+    result = None
     try:
         c = delegator.run("{0} {1}".format(_which, command))
         try:
@@ -1480,21 +1519,20 @@ def system_which(command, mult=False):
                 )
             assert c.return_code == 0
         except AssertionError:
-            return None if not mult else []
+            result = fallback_which(command, allow_global=True)
     except TypeError:
-        from .vendor.pythonfinder import Finder
-        finder = Finder()
-        result = finder.which(command)
-        if result:
-            return result.path.as_posix()
-        return
+        if not result:
+            result = fallback_which(command, allow_global=True)
     else:
-        result = c.out.strip() or c.err.strip()
-    if mult:
-        return result.split("\n")
+        if not result:
+            result = next(iter([c.out, c.err]), "").split("\n")
+            result = next(iter(result)) if not mult else result
+            return result
+        if not result:
+            result = fallback_which(command, allow_global=True)
+    result = [result] if mult else result
+    return result
 
-    else:
-        return result.split("\n")[0]
 
 
 def format_help(help):
@@ -2094,6 +2132,7 @@ def do_uninstall(
         p for normalized, p in selected_pkg_map.items()
         if normalized in (used_packages - bad_pkgs)
     ]
+    pip_path = None
     for normalized, package_name in selected_pkg_map.items():
         click.echo(
             crayons.white(
@@ -2103,12 +2142,10 @@ def do_uninstall(
         # Uninstall the package.
         if package_name in packages_to_remove:
             with project.environment.activated():
-                cmd = "{0} uninstall {1} -y".format(
-                    escape_grouped_arguments(which_pip(allow_global=system)), package_name,
-                )
-                if environments.is_verbose():
-                    click.echo("$ {0}".format(cmd))
-                c = delegator.run(cmd)
+                if pip_path is None:
+                    pip_path = which_pip(allow_global=system)
+                cmd = [pip_path, "uninstall", package_name, "-y"]
+                c = run_command(cmd)
                 click.echo(crayons.blue(c.out))
                 if c.return_code != 0:
                     failure = True
@@ -2340,6 +2377,7 @@ def do_check(
     args=None,
     pypi_mirror=None,
 ):
+    from pipenv.vendor.vistir.compat import JSONDecodeError
     if not system:
         # Ensure that virtualenv is available.
         ensure_project(
@@ -2370,18 +2408,27 @@ def do_check(
             sys.exit(1)
         else:
             sys.exit(0)
-    click.echo(crayons.normal(fix_utf8("Checking PEP 508 requirements…"), bold=True))
-    if system:
-        python = system_which("python")
-    else:
+    click.echo(crayons.normal(decode_output("Checking PEP 508 requirements…"), bold=True))
+    pep508checker_path = pep508checker.__file__.rstrip("cdo")
+    safety_path = os.path.join(
+        os.path.dirname(os.path.abspath(__file__)), "patched", "safety.zip"
+    )
+    if not system:
         python = which("python")
+    else:
+        python = system_which("python")
+    _cmd = [python,]
     # Run the PEP 508 checker in the virtualenv.
-    c = delegator.run(
-        '"{0}" {1}'.format(
-            python, escape_grouped_arguments(pep508checker.__file__.rstrip("cdo"))
-        )
-    )
-    results = simplejson.loads(c.out)
+    cmd = _cmd + [pep508checker_path]
+    c = run_command(cmd)
+    try:
+        results = simplejson.loads(c.out.strip())
+    except JSONDecodeError:
+        click.echo("{0}\n{1}".format(
+            crayons.white(decode_output("Failed parsing pep508 results: "), bold=True),
+            c.out.strip()
+        ))
+        sys.exit(1)
     # Load the pipfile.
     p = pipfile.Pipfile.load(project.pipfile_location)
     failed = False
@@ -2406,13 +2453,9 @@ def do_check(
         sys.exit(1)
     else:
         click.echo(crayons.green("Passed!"))
-    click.echo(crayons.normal(fix_utf8("Checking installed package safety…"), bold=True))
-    path = pep508checker.__file__.rstrip("cdo")
-    path = os.sep.join(__file__.split(os.sep)[:-1] + ["patched", "safety.zip"])
-    if not system:
-        python = which("python")
-    else:
-        python = system_which("python")
+    click.echo(crayons.normal(
+        decode_output("Checking installed package safety…"), bold=True)
+    )
     if ignore:
         ignored = "--ignore {0}".format(" --ignore ".join(ignore))
         click.echo(
@@ -2423,17 +2466,13 @@ def do_check(
         )
     else:
         ignored = ""
-    c = delegator.run(
-        '"{0}" {1} check --json --key={2} {3}'.format(
-            python, escape_grouped_arguments(path), PIPENV_PYUP_API_KEY, ignored
-        )
-    )
+    key = "--key={0}".format(PIPENV_PYUP_API_KEY)
+    cmd = _cmd + [safety_path, "check", "--json", key, ignored]
+    c = run_command(cmd)
     try:
         results = simplejson.loads(c.out)
-    except ValueError:
-        click.echo("An error occurred:", err=True)
-        click.echo(c.err if len(c.err) > 0 else c.out, err=True)
-        sys.exit(1)
+    except (ValueError, JSONDecodeError):
+        raise exceptions.JSONParseError(c.out, c.err)
     for (package, resolved, installed, description, vuln) in results:
         click.echo(
             "{0}: {1} {2} resolved ({3} installed)!".format(
@@ -2452,7 +2491,9 @@ def do_check(
 
 
 def do_graph(bare=False, json=False, json_tree=False, reverse=False):
+    from pipenv.vendor.vistir.compat import JSONDecodeError
     import pipdeptree
+    pipdeptree_path = pipdeptree.__file__.rstrip("cdo")
     try:
         python_path = which("python")
     except AttributeError:
@@ -2517,11 +2558,9 @@ def do_graph(bare=False, json=False, json_tree=False, reverse=False):
             err=True,
         )
         sys.exit(1)
-    cmd = '"{0}" {1} {2} -l'.format(
-        python_path, escape_grouped_arguments(pipdeptree.__file__.rstrip("cdo")), flag
-    )
+    cmd_args = [python_path, pipdeptree_path, flag, "-l"]
+    c = run_command(cmd_args)
     # Run dep-tree.
-    c = delegator.run(cmd)
     if not bare:
         if json:
             data = []
@@ -2543,9 +2582,14 @@ def traverse(obj):
                     obj["dependencies"] = traverse(obj["dependencies"])
                     return obj
 
-            data = traverse(simplejson.loads(c.out))
-            click.echo(simplejson.dumps(data, indent=4))
-            sys.exit(0)
+            try:
+                parsed = simplejson.loads(c.out.strip())
+            except JSONDecodeError:
+                raise exceptions.JSONParseError(c.out, c.err)
+            else:
+                data = traverse(parsed)
+                click.echo(simplejson.dumps(data, indent=4))
+                sys.exit(0)
         else:
             for line in c.out.strip().split("\n"):
                 # Ignore bad packages as top level.
@@ -2656,9 +2700,8 @@ def do_clean(ctx, three=None, python=None, dry_run=False, bare=False, pypi_mirro
                     )
                 )
             # Uninstall the package.
-            c = delegator.run(
-                "{0} uninstall {1} -y".format(which_pip(), apparent_bad_package)
-            )
+            cmd = [which_pip(), "uninstall", apparent_bad_package, "-y"]
+            c = run_command(cmd)
             if c.return_code != 0:
                 failure = True
     sys.exit(int(failure))