Merge pull request #1364 from pre-commit/exec_error_no_shebang

Don't crash out on OSErrors in subprocess calls

pre_commit/error_handler.py

@@ -8,23 +8,15 @@ from typing import Generator
 import pre_commit.constants as C
 from pre_commit import output
 from pre_commit.store import Store
+from pre_commit.util import force_bytes
 
 
 class FatalError(RuntimeError):
     pass
 
 
-def _exception_to_bytes(exc: BaseException) -> bytes:
-    with contextlib.suppress(TypeError):
-        return bytes(exc)  # type: ignore
-    with contextlib.suppress(Exception):
-        return str(exc).encode()
-    return f'<unprintable {type(exc).__name__} object>'.encode()
-
-
 def _log_and_exit(msg: str, exc: BaseException, formatted: str) -> None:
-    error_msg = f'{msg}: {type(exc).__name__}: '.encode()
-    error_msg += _exception_to_bytes(exc)
+    error_msg = f'{msg}: {type(exc).__name__}: '.encode() + force_bytes(exc)
     output.write_line_b(error_msg)
     log_path = os.path.join(Store().directory, 'pre-commit.log')
     output.write_line(f'Check the log at {log_path}')

pre_commit/util.py

@@ -43,6 +43,14 @@ def yaml_dump(o: Any) -> str:
     )
 
 
+def force_bytes(exc: Any) -> bytes:
+    with contextlib.suppress(TypeError):
+        return bytes(exc)
+    with contextlib.suppress(Exception):
+        return str(exc).encode()
+    return f'<unprintable {type(exc).__name__} object>'.encode()
+
+
 @contextlib.contextmanager
 def clean_path_on_failure(path: str) -> Generator[None, None, None]:
     """Cleans up the directory on an exceptional failure."""
@@ -120,6 +128,10 @@ def _setdefault_kwargs(kwargs: Dict[str, Any]) -> None:
         kwargs.setdefault(arg, subprocess.PIPE)
 
 
+def _oserror_to_output(e: OSError) -> Tuple[int, bytes, None]:
+    return 1, force_bytes(e).rstrip(b'\n') + b'\n', None
+
+
 def cmd_output_b(
         *cmd: str,
         retcode: Optional[int] = 0,
@@ -132,9 +144,13 @@ def cmd_output_b(
     except parse_shebang.ExecutableNotFoundError as e:
         returncode, stdout_b, stderr_b = e.to_output()
     else:
-        proc = subprocess.Popen(cmd, **kwargs)
-        stdout_b, stderr_b = proc.communicate()
-        returncode = proc.returncode
+        try:
+            proc = subprocess.Popen(cmd, **kwargs)
+        except OSError as e:
+            returncode, stdout_b, stderr_b = _oserror_to_output(e)
+        else:
+            stdout_b, stderr_b = proc.communicate()
+            returncode = proc.returncode
 
     if retcode is not None and retcode != returncode:
         raise CalledProcessError(returncode, cmd, retcode, stdout_b, stderr_b)
@@ -205,7 +221,11 @@ if os.name != 'nt':  # pragma: win32 no cover
         with open(os.devnull) as devnull, Pty() as pty:
             assert pty.r is not None
             kwargs.update({'stdin': devnull, 'stdout': pty.w, 'stderr': pty.w})
-            proc = subprocess.Popen(cmd, **kwargs)
+            try:
+                proc = subprocess.Popen(cmd, **kwargs)
+            except OSError as e:
+                return _oserror_to_output(e)
+
             pty.close_w()
 
             buf = b''

tests/util_test.py

@@ -9,6 +9,7 @@ from pre_commit.util import clean_path_on_failure
 from pre_commit.util import cmd_output
 from pre_commit.util import cmd_output_b
 from pre_commit.util import cmd_output_p
+from pre_commit.util import make_executable
 from pre_commit.util import parse_version
 from pre_commit.util import rmtree
 from pre_commit.util import tmpdir
@@ -92,6 +93,18 @@ def test_cmd_output_exe_not_found_bytes(fn):
     assert out == b'Executable `dne` not found'
 
 
+@pytest.mark.parametrize('fn', (cmd_output_b, cmd_output_p))
+def test_cmd_output_no_shebang(tmpdir, fn):
+    f = tmpdir.join('f').ensure()
+    make_executable(f)
+
+    # previously this raised `OSError` -- the output is platform specific
+    ret, out, _ = fn(str(f), retcode=None, stderr=subprocess.STDOUT)
+    assert ret == 1
+    assert isinstance(out, bytes)
+    assert out.endswith(b'\n')
+
+
 def test_parse_version():
     assert parse_version('0.0') == parse_version('0.0')
     assert parse_version('0.1') > parse_version('0.0')