Update all subprocess calls to use errors="backslashreplace"

changes/1141.bugfix.rst

@@ -0,0 +1 @@
+When commands produce output that cannot be decoded to Unicode, Briefcase now writes the bytes as hex instead of truncating output or canceling the command altogether.

src/briefcase/integrations/subprocess.py

@@ -221,25 +221,36 @@ def final_kwargs(self, **kwargs):
                 "encoding", os.device_encoding(sys.__stdout__.fileno()) or "UTF-8"
             )
 
+        # When text mode is enabled, subprocess defaults to "strict"
+        # handling for errors arising from decoding output to Unicode.
+        # To avoid Unicode exceptions from misbehaving commands, set
+        # `errors` so output that cannot be decoded for the specified
+        # encoding are replaced with hex of the raw bytes.
+        if any(kwargs.get(kw) for kw in ["text", "encoding", "universal_encoding"]):
+            kwargs.setdefault("errors", "backslashreplace")
+
         # For Windows, convert start_new_session=True to creation flags
         if self.tools.host_os == "Windows":
             try:
                 if kwargs.pop("start_new_session") is True:
                     if "creationflags" in kwargs:
                         raise AssertionError(
-                            "Subprocess called with creationflags set and start_new_session=True.\n"
-                            "This will result in CREATE_NEW_PROCESS_GROUP and CREATE_NO_WINDOW being "
-                            "merged in to the creationflags.\n\n"
-                            "Ensure this is desired configuration or don't set start_new_session=True."
+                            "Subprocess called with creationflags set and "
+                            "start_new_session=True.\nThis will result in "
+                            "CREATE_NEW_PROCESS_GROUP and CREATE_NO_WINDOW "
+                            "being merged in to the creationflags.\n\nEnsure "
+                            "this is desired configuration or don't set "
+                            "start_new_session=True."
                         )
-                    # CREATE_NEW_PROCESS_GROUP: Makes the new process the root process
-                    #     of a new process group. This also disables CTRL+C signal handlers
-                    #     for all processes of the new process group.
-                    # CREATE_NO_WINDOW: Creates a new console for the process but does not
-                    #     open a visible window for that console. This flag is used instead
-                    #     of DETACHED_PROCESS since the new process can spawn a new console
-                    #     itself (in the absence of one being available) but that console
-                    #     creation will also spawn a visible console window.
+                    # CREATE_NEW_PROCESS_GROUP: Promotes the new process to the root
+                    #    process of a new process group. This also disables CTRL+C
+                    #    signal handlers for all processes of the new process group.
+                    # CREATE_NO_WINDOW: Creates a new console for the process but
+                    #    does not open a visible window for that console. This flag
+                    #    is used instead of DETACHED_PROCESS since the new process
+                    #    can spawn a new console itself (in the absence of one being
+                    #    available) but that console creation will also spawn a
+                    #    visible console window.
                     new_session_flags = (
                         self._subprocess.CREATE_NEW_PROCESS_GROUP
                         | self._subprocess.CREATE_NO_WINDOW

tests/conftest.py

@@ -1,5 +1,6 @@
 import inspect
-from unittest import mock
+import subprocess
+from unittest.mock import ANY, MagicMock
 
 import git as git_
 import pytest
@@ -19,9 +20,8 @@ def pytest_sessionfinish(session, exitstatus):
 
 @pytest.fixture
 def mock_git():
-    git = mock.MagicMock(spec_set=git_)
+    git = MagicMock(spec_set=git_)
     git.exc = git_exceptions
-
     return git
 
 
@@ -49,6 +49,27 @@ def no_print(monkeypatch):
     monkeypatch.setattr("builtins.print", monkeypatched_print)
 
 
+@pytest.fixture
+def sub_kw():
+    """Default keyword arguments for all subprocess calls."""
+    return dict(text=True, encoding=ANY, errors="backslashreplace")
+
+
+@pytest.fixture
+def sub_check_output_kw(sub_kw):
+    """Default keyword arguments for all subprocess.check_output calls."""
+    return {**sub_kw, **dict(stderr=subprocess.STDOUT)}
+
+
+@pytest.fixture
+def sub_stream_kw(sub_kw):
+    """Default keyword arguments for all output streaming subprocess calls."""
+    return {
+        **sub_kw,
+        **dict(stdout=subprocess.PIPE, stderr=subprocess.STDOUT, bufsize=1),
+    }
+
+
 @pytest.fixture
 def first_app_config():
     return AppConfig(

tests/integrations/docker/test_DockerAppContext__check_output.py

@@ -1,13 +1,11 @@
 import os
-import subprocess
 import sys
 from pathlib import Path
-from unittest.mock import ANY
 
 import pytest
 
 
-def test_simple_call(mock_docker_app_context, tmp_path, capsys):
+def test_simple_call(mock_docker_app_context, tmp_path, sub_check_output_kw, capsys):
     """A simple call will be invoked."""
     assert mock_docker_app_context.check_output(["hello", "world"]) == "goodbye\n"
 
@@ -24,14 +22,12 @@ def test_simple_call(mock_docker_app_context, tmp_path, capsys):
             "hello",
             "world",
         ],
-        text=True,
-        encoding=ANY,
-        stderr=subprocess.STDOUT,
+        **sub_check_output_kw,
     )
     assert capsys.readouterr().out == ""
 
 
-def test_extra_mounts(mock_docker_app_context, tmp_path, capsys):
+def test_extra_mounts(mock_docker_app_context, tmp_path, sub_check_output_kw, capsys):
     """A call can request additional mounts."""
     assert (
         mock_docker_app_context.check_output(
@@ -61,17 +57,15 @@ def test_extra_mounts(mock_docker_app_context, tmp_path, capsys):
             "hello",
             "world",
         ],
-        text=True,
-        encoding=ANY,
-        stderr=subprocess.STDOUT,
+        **sub_check_output_kw,
     )
     assert capsys.readouterr().out == ""
 
 
 @pytest.mark.skipif(
     sys.platform == "win32", reason="Windows paths aren't converted in Docker context"
 )
-def test_cwd(mock_docker_app_context, tmp_path, capsys):
+def test_cwd(mock_docker_app_context, tmp_path, sub_check_output_kw, capsys):
     """A call can use a working directory relative to the project folder."""
     assert (
         mock_docker_app_context.check_output(
@@ -96,14 +90,17 @@ def test_cwd(mock_docker_app_context, tmp_path, capsys):
             "hello",
             "world",
         ],
-        text=True,
-        encoding=ANY,
-        stderr=subprocess.STDOUT,
+        **sub_check_output_kw,
     )
     assert capsys.readouterr().out == ""
 
 
-def test_call_with_arg_and_env(mock_docker_app_context, tmp_path, capsys):
+def test_call_with_arg_and_env(
+    mock_docker_app_context,
+    tmp_path,
+    sub_check_output_kw,
+    capsys,
+):
     """Extra keyword arguments are passed through as-is; env modifications are
     converted."""
     output = mock_docker_app_context.check_output(
@@ -116,6 +113,7 @@ def test_call_with_arg_and_env(mock_docker_app_context, tmp_path, capsys):
     )
     assert output == "goodbye\n"
 
+    sub_check_output_kw.pop("text")
     mock_docker_app_context.tools.subprocess._subprocess.check_output.assert_called_once_with(
         [
             "docker",
@@ -134,8 +132,7 @@ def test_call_with_arg_and_env(mock_docker_app_context, tmp_path, capsys):
             "world",
         ],
         universal_newlines=True,
-        encoding=ANY,
-        stderr=subprocess.STDOUT,
+        **sub_check_output_kw,
     )
     assert capsys.readouterr().out == ""
 
@@ -144,7 +141,12 @@ def test_call_with_arg_and_env(mock_docker_app_context, tmp_path, capsys):
     sys.platform == "win32",
     reason="Windows paths aren't converted in Docker context",
 )
-def test_call_with_path_arg_and_env(mock_docker_app_context, tmp_path, capsys):
+def test_call_with_path_arg_and_env(
+    mock_docker_app_context,
+    tmp_path,
+    sub_check_output_kw,
+    capsys,
+):
     """Path-based arguments and environment are converted to strings and passed in as-
     is."""
     output = mock_docker_app_context.check_output(
@@ -176,17 +178,20 @@ def test_call_with_path_arg_and_env(mock_docker_app_context, tmp_path, capsys):
             "hello",
             os.fsdecode(tmp_path / "location"),
         ],
-        text=True,
-        encoding=ANY,
-        stderr=subprocess.STDOUT,
+        **sub_check_output_kw,
     )
     assert capsys.readouterr().out == ""
 
 
 @pytest.mark.skipif(
     sys.platform == "win32", reason="Windows paths aren't converted in Docker context"
 )
-def test_simple_verbose_call(mock_docker_app_context, tmp_path, capsys):
+def test_simple_verbose_call(
+    mock_docker_app_context,
+    tmp_path,
+    sub_check_output_kw,
+    capsys,
+):
     """If verbosity is turned out, there is output."""
     mock_docker_app_context.tools.logger.verbosity = 2
 
@@ -205,9 +210,7 @@ def test_simple_verbose_call(mock_docker_app_context, tmp_path, capsys):
             "hello",
             "world",
         ],
-        text=True,
-        encoding=ANY,
-        stderr=subprocess.STDOUT,
+        **sub_check_output_kw,
     )
     assert capsys.readouterr().out == (
         "\n"

tests/integrations/docker/test_DockerAppContext__prepare.py

@@ -1,14 +1,13 @@
 import os
 import subprocess
-from unittest.mock import ANY
 
 import pytest
 
 from briefcase.exceptions import BriefcaseCommandError
 from briefcase.integrations.docker import DockerAppContext
 
 
-def test_prepare(mock_tools, my_app, tmp_path):
+def test_prepare(mock_tools, my_app, tmp_path, sub_stream_kw):
     """The Docker environment can be prepared."""
     mock_docker_app_context = DockerAppContext(mock_tools, my_app)
     mock_docker_app_context.prepare(
@@ -40,11 +39,7 @@ def test_prepare(mock_tools, my_app, tmp_path):
             "HOST_GID=42",
             os.fsdecode(tmp_path / "base" / "path" / "to" / "src"),
         ],
-        stdout=-1,
-        stderr=-2,
-        bufsize=1,
-        text=True,
-        encoding=ANY,
+        **sub_stream_kw,
     )
 
     assert mock_docker_app_context.app_base_path == tmp_path / "base"
@@ -54,7 +49,7 @@ def test_prepare(mock_tools, my_app, tmp_path):
     assert mock_docker_app_context.python_version == "3.X"
 
 
-def test_prepare_failure(mock_docker_app_context, tmp_path):
+def test_prepare_failure(mock_docker_app_context, tmp_path, sub_stream_kw):
     """If the Docker environment can't be prepared, an error is raised."""
     # Mock a failure in docker build.
     mock_docker_app_context.tools.subprocess._subprocess.Popen.side_effect = (
@@ -91,9 +86,5 @@ def test_prepare_failure(mock_docker_app_context, tmp_path):
             "HOST_GID=42",
             os.fsdecode(tmp_path / "base" / "path" / "to" / "src"),
         ],
-        stdout=-1,
-        stderr=-2,
-        bufsize=1,
-        text=True,
-        encoding=ANY,
+        **sub_stream_kw,
     )