Merge pull request #1694 from rmartin16/subprocess-fsdecode

Remove `fsdecode()` calls for `subprocess` commands

changes/1694.misc.rst

@@ -0,0 +1 @@
+Since ``subprocess`` already calls ``fsdecode()`` for command arguments, it was removed for calls to ``subprocess`` in Briefcase.

src/briefcase/integrations/android_sdk.py

@@ -451,7 +451,7 @@ def upgrade(self):
             # Using subprocess.run() with no I/O redirection so the user sees
             # the full output and can send input.
             self.tools.subprocess.run(
-                [os.fsdecode(self.sdkmanager_path), "--update"],
+                [self.sdkmanager_path, "--update"],
                 env=self.env,
                 check=True,
                 stream_output=False,
@@ -557,7 +557,7 @@ def list_packages(self):
         try:
             # check_output always writes its output to debug
             self.tools.subprocess.check_output(
-                [os.fsdecode(self.sdkmanager_path), "--list_installed"],
+                [self.sdkmanager_path, "--list_installed"],
                 env=self.env,
             )
         except subprocess.CalledProcessError as e:
@@ -593,7 +593,7 @@ def verify_license(self):
             # Using subprocess.run() with no I/O redirection so the user sees
             # the full output and can send input.
             self.tools.subprocess.run(
-                [os.fsdecode(self.sdkmanager_path), "--licenses"],
+                [self.sdkmanager_path, "--licenses"],
                 env=self.env,
                 check=True,
                 stream_output=False,
@@ -641,11 +641,7 @@ def verify_emulator(self):
         self.tools.logger.info("Downloading the Android emulator...")
         try:
             self.tools.subprocess.run(
-                [
-                    os.fsdecode(self.sdkmanager_path),
-                    "platform-tools",
-                    "emulator",
-                ],
+                [self.sdkmanager_path, "platform-tools", "emulator"],
                 env=self.env,
                 check=True,
                 stream_output=False,
@@ -771,10 +767,7 @@ def verify_system_image(self, system_image: str):
         )
         try:
             self.tools.subprocess.run(
-                [
-                    os.fsdecode(self.sdkmanager_path),
-                    system_image,
-                ],
+                [self.sdkmanager_path, system_image],
                 env=self.env,
                 check=True,
                 stream_output=False,
@@ -835,7 +828,7 @@ def emulators(self) -> list[str]:
             # Capture `stderr` so that if the process exits with failure, the
             # stderr data is in `e.output`.
             output = self.tools.subprocess.check_output(
-                [os.fsdecode(self.emulator_path), "-list-avds"]
+                [self.emulator_path, "-list-avds"]
             ).strip()
             # AVD names are returned one per line.
             if len(output) == 0:
@@ -848,7 +841,7 @@ def devices(self) -> dict[str, dict[str, str | bool]]:
         """Find the devices that are attached and available to ADB."""
         try:
             output = self.tools.subprocess.check_output(
-                [os.fsdecode(self.adb_path), "devices", "-l"]
+                [self.adb_path, "devices", "-l"]
             ).strip()
             # Process the output of `adb devices -l`.
             # The first line is header information.
@@ -1197,7 +1190,7 @@ def _create_emulator(
             try:
                 self.tools.subprocess.check_output(
                     [
-                        os.fsdecode(self.avdmanager_path),
+                        self.avdmanager_path,
                         "--verbose",
                         "create",
                         "avd",
@@ -1289,13 +1282,7 @@ def start_emulator(
             extra_args = []
 
         emulator_popen = self.tools.subprocess.Popen(
-            [
-                os.fsdecode(self.emulator_path),
-                f"@{avd}",
-                "-dns-server",
-                "8.8.8.8",
-            ]
-            + extra_args,
+            [self.emulator_path, f"@{avd}", "-dns-server", "8.8.8.8"] + extra_args,
             env=self.env,
             stdout=subprocess.PIPE,
             stderr=subprocess.STDOUT,
@@ -1479,15 +1466,7 @@ def run(self, *arguments: SubprocessArgT, quiet: bool = False) -> str:
         # This keeps performance good in the success case.
         try:
             output = self.tools.subprocess.check_output(
-                [
-                    os.fsdecode(self.tools.android_sdk.adb_path),
-                    "-s",
-                    self.device,
-                ]
-                + [
-                    (os.fsdecode(arg) if isinstance(arg, Path) else arg)
-                    for arg in arguments
-                ],
+                [self.tools.android_sdk.adb_path, "-s", self.device] + list(arguments),
                 quiet=quiet,
             )
             # add returns status code 0 in the case of failure. The only tangible evidence
@@ -1594,7 +1573,7 @@ def logcat(self, pid: str) -> subprocess.Popen:
         # See #1425 for details.
         return self.tools.subprocess.Popen(
             [
-                os.fsdecode(self.tools.android_sdk.adb_path),
+                self.tools.android_sdk.adb_path,
                 "-s",
                 self.device,
                 "logcat",
@@ -1623,7 +1602,7 @@ def logcat_tail(self, since: datetime):
             # See #1425 for details.
             self.tools.subprocess.run(
                 [
-                    os.fsdecode(self.tools.android_sdk.adb_path),
+                    self.tools.android_sdk.adb_path,
                     "-s",
                     self.device,
                     "logcat",

src/briefcase/integrations/java.py

@@ -77,10 +77,7 @@ def version_from_path(cls, tools: ToolCache, java_path: str | Path) -> str:
         :return: JDK release version; e.g. "17.0.X"
         """
         output = tools.subprocess.check_output(
-            [
-                os.fsdecode(Path(java_path) / "bin/javac"),
-                "-version",
-            ],
+            [Path(java_path) / "bin/javac", "-version"]
         )
         # javac's output should look like "javac 17.0.X\n"
         return output.strip("\n").split(" ")[1]

src/briefcase/platforms/linux/appimage.py

@@ -1,6 +1,7 @@
+from __future__ import annotations
+
 import os
 import subprocess
-from typing import List
 
 from briefcase.commands import (
     BuildCommand,
@@ -338,11 +339,9 @@ def build_app(self, app: AppConfig, **kwargs):  # pragma: no-cover-if-is-windows
                         self.tools.linuxdeploy.file_path
                         / self.tools.linuxdeploy.file_name,
                         "--appdir",
-                        os.fsdecode(self.appdir_path(app)),
+                        self.appdir_path(app),
                         "--desktop-file",
-                        os.fsdecode(
-                            self.appdir_path(app) / f"{app.bundle_identifier}.desktop"
-                        ),
+                        self.appdir_path(app) / f"{app.bundle_identifier}.desktop",
                         "--output",
                         "appimage",
                         "-v0" if self.logger.is_deep_debug else "-v1",
@@ -370,7 +369,7 @@ def run_app(
         self,
         app: AppConfig,
         test_mode: bool,
-        passthrough: List[str],
+        passthrough: list[str],
         **kwargs,
     ):
         """Start the application.
@@ -384,7 +383,7 @@ def run_app(
 
         # Start the app in a way that lets us stream the logs
         app_popen = self.tools.subprocess.Popen(
-            [os.fsdecode(self.binary_path(app))] + passthrough,
+            [self.binary_path(app)] + passthrough,
             cwd=self.tools.home_path,
             **kwargs,
             stdout=subprocess.PIPE,

src/briefcase/platforms/linux/system.py

@@ -1,7 +1,6 @@
 from __future__ import annotations
 
 import gzip
-import os
 import re
 import subprocess
 import sys
@@ -807,7 +806,7 @@ def run_app(
         with self.tools[app].app_context.run_app_context(kwargs) as kwargs:
             # Start the app in a way that lets us stream the logs
             app_popen = self.tools[app].app_context.Popen(
-                [os.fsdecode(self.binary_path(app))] + passthrough,
+                [self.binary_path(app)] + passthrough,
                 cwd=self.tools.home_path,
                 stdout=subprocess.PIPE,
                 stderr=subprocess.STDOUT,

src/briefcase/platforms/macOS/__init__.py

@@ -251,11 +251,8 @@ def run_app(
 
             # Start the app in a way that lets us stream the logs
             self.tools.subprocess.run(
-                [
-                    "open",
-                    "-n",  # Force a new app to be launched
-                    os.fsdecode(self.binary_path(app)),
-                ]
+                # Force a new app to be launched
+                ["open", "-n", self.binary_path(app)]
                 + ((["--args"] + passthrough) if passthrough else []),
                 cwd=self.tools.home_path,
                 check=True,
@@ -394,16 +391,11 @@ def sign_file(self, path, identity, entitlements=None):
         :param entitlements: The path to the entitlements file to use.
         """
         options = "runtime" if identity != "-" else None
-        process_command = [
-            "codesign",
-            os.fsdecode(path),
-            "--sign",
-            identity,
-            "--force",
-        ]
+        process_command = ["codesign", path, "--sign", identity, "--force"]
+
         if entitlements:
             process_command.append("--entitlements")
-            process_command.append(os.fsdecode(entitlements))
+            process_command.append(entitlements)
         if options:
             process_command.append("--options")
             process_command.append(options)
@@ -676,7 +668,7 @@ def notarize(self, filename, team_id):
                             "xcrun",
                             "notarytool",
                             "submit",
-                            os.fsdecode(archive_filename),
+                            archive_filename,
                             "--keychain-profile",
                             profile,
                             "--wait",
@@ -707,12 +699,7 @@ def notarize(self, filename, team_id):
                 f"Stapling notarization onto {filename.relative_to(self.base_path)}..."
             )
             self.tools.subprocess.run(
-                [
-                    "xcrun",
-                    "stapler",
-                    "staple",
-                    os.fsdecode(filename),
-                ],
+                ["xcrun", "stapler", "staple", filename],
                 check=True,
             )
         except subprocess.CalledProcessError:

src/briefcase/platforms/windows/__init__.py

@@ -1,4 +1,3 @@
-import os
 import re
 import subprocess
 import uuid
@@ -141,7 +140,7 @@ def run_app(
 
         # Start the app in a way that lets us stream the logs
         app_popen = self.tools.subprocess.Popen(
-            [os.fsdecode(self.binary_path(app))] + passthrough,
+            [self.binary_path(app)] + passthrough,
             cwd=self.tools.home_path,
             encoding="UTF-8",
             stdout=subprocess.PIPE,
@@ -367,7 +366,7 @@ def _package_msi(self, app):
                     [
                         self.tools.wix.heat_exe,
                         "dir",
-                        os.fsdecode(self.packaging_root),
+                        self.packaging_root,
                         "-nologo",  # Don't display startup text
                         "-gg",  # Generate GUIDs
                         "-sfrag",  # Suppress fragment generation for directories

tests/commands/build/conftest.py

@@ -4,7 +4,6 @@
 from briefcase.commands.base import full_options
 from briefcase.config import AppConfig
 from briefcase.console import Console, Log
-
 from ...utils import create_file
 
 

tests/integrations/android_sdk/ADB/test_kill.py

@@ -1,4 +1,3 @@
-import os
 import subprocess
 from unittest.mock import MagicMock
 
@@ -15,7 +14,7 @@ def test_kill(mock_tools, adb):
     # Validate call parameters.
     mock_tools.subprocess.check_output.assert_called_once_with(
         [
-            os.fsdecode(mock_tools.android_sdk.adb_path),
+            mock_tools.android_sdk.adb_path,
             "-s",
             "exampleDevice",
             "emu",

tests/integrations/android_sdk/ADB/test_logcat.py

@@ -1,4 +1,3 @@
-import os
 import subprocess
 from unittest import mock
 
@@ -25,7 +24,7 @@ def test_logcat(mock_tools, adb, is_color_enabled, monkeypatch):
     # Validate call parameters.
     mock_tools.subprocess.Popen.assert_called_once_with(
         [
-            os.fsdecode(mock_tools.android_sdk.adb_path),
+            mock_tools.android_sdk.adb_path,
             "-s",
             "exampleDevice",
             "logcat",

tests/integrations/android_sdk/ADB/test_logcat_tail.py

@@ -1,4 +1,3 @@
-import os
 import subprocess
 from datetime import datetime
 from unittest.mock import MagicMock, PropertyMock
@@ -24,7 +23,7 @@ def test_logcat_tail(mock_tools, adb, is_color_enabled, monkeypatch):
     # Validate call parameters.
     mock_tools.subprocess.run.assert_called_once_with(
         [
-            os.fsdecode(mock_tools.android_sdk.adb_path),
+            mock_tools.android_sdk.adb_path,
             "-s",
             "exampleDevice",
             "logcat",

tests/integrations/android_sdk/ADB/test_run.py

@@ -1,4 +1,3 @@
-import os
 import subprocess
 import sys
 from pathlib import Path
@@ -15,12 +14,8 @@ def test_simple_command(mock_tools, adb, tmp_path):
     # Check that adb was invoked with the expected commands
     mock_tools.subprocess.check_output.assert_called_once_with(
         [
-            os.fsdecode(
-                tmp_path
-                / "sdk"
-                / "platform-tools"
-                / f"adb{'.exe' if sys.platform == 'win32' else ''}"
-            ),
+            tmp_path
+            / f"sdk/platform-tools/adb{'.exe' if sys.platform == 'win32' else ''}",
             "-s",
             "exampleDevice",
             "example",
@@ -37,12 +32,8 @@ def test_quiet_command(mock_tools, adb, tmp_path):
     # Check that adb was invoked with the expected commands
     mock_tools.subprocess.check_output.assert_called_once_with(
         [
-            os.fsdecode(
-                tmp_path
-                / "sdk"
-                / "platform-tools"
-                / f"adb{'.exe' if sys.platform == 'win32' else ''}"
-            ),
+            tmp_path
+            / f"sdk/platform-tools/adb{'.exe' if sys.platform == 'win32' else ''}",
             "-s",
             "exampleDevice",
             "example",
@@ -86,12 +77,8 @@ def test_error_handling(mock_tools, adb, name, exception, tmp_path):
     # Check that adb was invoked as expected
     mock_tools.subprocess.check_output.assert_called_once_with(
         [
-            os.fsdecode(
-                tmp_path
-                / "sdk"
-                / "platform-tools"
-                / f"adb{'.exe' if sys.platform == 'win32' else ''}"
-            ),
+            tmp_path
+            / f"sdk/platform-tools/adb{'.exe' if sys.platform == 'win32' else ''}",
             "-s",
             "exampleDevice",
             "example",