Fixed #35308 -- Handled OSError when launching code formatters.

Co-authored-by: Natalia <[email protected]>

django/core/management/commands/makemigrations.py

@@ -392,7 +392,7 @@ def write_migration_files(self, changes, update_previous_migration_paths=None):
                         )
                     )
                     self.log(writer.as_string())
-        run_formatters(self.written_files)
+        run_formatters(self.written_files, stderr=self.stderr)
 
     @staticmethod
     def get_relative_path(path):
@@ -499,7 +499,7 @@ def all_items_equal(seq):
                     # Write the merge migrations file to the disk
                     with open(writer.path, "w", encoding="utf-8") as fh:
                         fh.write(writer.as_string())
-                    run_formatters([writer.path])
+                    run_formatters([writer.path], stderr=self.stderr)
                     if self.verbosity > 0:
                         self.log("\nCreated new merge migration %s" % writer.path)
                         if self.scriptable:

django/core/management/commands/optimizemigration.py

@@ -121,7 +121,7 @@ def handle(self, *args, **options):
                     )
         with open(writer.path, "w", encoding="utf-8") as fh:
             fh.write(migration_file_string)
-        run_formatters([writer.path])
+        run_formatters([writer.path], stderr=self.stderr)
 
         if verbosity > 0:
             self.stdout.write(

django/core/management/commands/squashmigrations.py

@@ -221,7 +221,7 @@ def handle(self, **options):
             )
         with open(writer.path, "w", encoding="utf-8") as fh:
             fh.write(writer.as_string())
-        run_formatters([writer.path])
+        run_formatters([writer.path], stderr=self.stderr)
 
         if self.verbosity > 0:
             self.stdout.write(

django/core/management/templates.py

@@ -229,7 +229,7 @@ def handle(self, app_or_project, name, target=None, **options):
                 else:
                     shutil.rmtree(path_to_remove)
 
-        run_formatters([top_dir], **formatter_paths)
+        run_formatters([top_dir], **formatter_paths, stderr=self.stderr)
 
     def handle_template(self, template, subdir):
         """

django/core/management/utils.py

@@ -2,6 +2,8 @@
 import os
 import shutil
 import subprocess
+import sys
+import traceback
 from pathlib import Path
 from subprocess import run
 
@@ -161,15 +163,19 @@ def find_formatters():
     return {"black_path": shutil.which("black")}
 
 
-def run_formatters(written_files, black_path=(sentinel := object())):
+def run_formatters(written_files, black_path=(sentinel := object()), stderr=sys.stderr):
     """
     Run the black formatter on the specified files.
     """
     # Use a sentinel rather than None, as which() returns None when not found.
     if black_path is sentinel:
         black_path = shutil.which("black")
     if black_path:
-        subprocess.run(
-            [black_path, "--fast", "--", *written_files],
-            capture_output=True,
-        )
+        try:
+            subprocess.run(
+                [black_path, "--fast", "--", *written_files],
+                capture_output=True,
+            )
+        except OSError:
+            stderr.write("Formatters failed to launch:")
+            traceback.print_exc(file=stderr)

tests/admin_scripts/tests.py

@@ -16,6 +16,8 @@
 from io import StringIO
 from unittest import mock
 
+from user_commands.utils import AssertFormatterFailureCaughtContext
+
 from django import conf, get_version
 from django.conf import settings
 from django.core.management import (
@@ -2943,6 +2945,16 @@ def test_honor_umask(self):
                     expected_mode,
                 )
 
+    def test_failure_to_format_code(self):
+        with AssertFormatterFailureCaughtContext(self) as ctx:
+            call_command(
+                "startapp",
+                "mynewapp",
+                directory=self.test_dir,
+                stdout=ctx.stdout,
+                stderr=ctx.stderr,
+            )
+
 
 class StartApp(AdminScriptTestCase):
     def test_invalid_name(self):

tests/migrations/test_base.py

@@ -4,7 +4,10 @@
 from contextlib import contextmanager
 from importlib import import_module
 
+from user_commands.utils import AssertFormatterFailureCaughtContext
+
 from django.apps import apps
+from django.core.management import call_command
 from django.db import connection, connections, migrations, models
 from django.db.migrations.migration import Migration
 from django.db.migrations.optimizer import MigrationOptimizer
@@ -168,6 +171,15 @@ def assertFKExists(self, table, columns, to, value=True, using="default"):
     def assertFKNotExists(self, table, columns, to):
         return self.assertFKExists(table, columns, to, False)
 
+    def assertFormatterFailureCaught(
+        self, *args, module="migrations.test_migrations", **kwargs
+    ):
+        with (
+            self.temporary_migration_module(module=module),
+            AssertFormatterFailureCaughtContext(self) as ctx,
+        ):
+            call_command(*args, stdout=ctx.stdout, stderr=ctx.stderr, **kwargs)
+
     @contextmanager
     def temporary_migration_module(self, app_label="migrations", module=None):
         """

tests/migrations/test_commands.py

@@ -2265,6 +2265,19 @@ def test_makemigrations_scriptable_merge(self, mock_input):
         self.assertEqual(out.getvalue(), f"{merge_file}\n")
         self.assertIn(f"Created new merge migration {merge_file}", err.getvalue())
 
+    def test_makemigrations_failure_to_format_code(self):
+        self.assertFormatterFailureCaught("makemigrations", "migrations")
+
+    def test_merge_makemigrations_failure_to_format_code(self):
+        self.assertFormatterFailureCaught("makemigrations", "migrations", empty=True)
+        self.assertFormatterFailureCaught(
+            "makemigrations",
+            "migrations",
+            merge=True,
+            interactive=False,
+            module="migrations.test_migrations_conflict",
+        )
+
     def test_makemigrations_migrations_modules_path_not_exist(self):
         """
         makemigrations creates migrations when specifying a custom location
@@ -3069,6 +3082,11 @@ def test_squashmigrations_manual_porting(self):
             + black_warning,
         )
 
+    def test_failure_to_format_code(self):
+        self.assertFormatterFailureCaught(
+            "squashmigrations", "migrations", "0002", interactive=False
+        )
+
 
 class AppLabelErrorTests(TestCase):
     """
@@ -3302,6 +3320,9 @@ def test_unknown_prefix(self):
         with self.assertRaisesMessage(CommandError, msg):
             call_command("optimizemigration", "migrations", "nonexistent")
 
+    def test_failure_to_format_code(self):
+        self.assertFormatterFailureCaught("optimizemigration", "migrations", "0001")
+
 
 class CustomMigrationCommandTests(MigrationTestBase):
     @override_settings(

tests/user_commands/test_files/black

@@ -0,0 +1 @@
+# This file is not executable, to simulate a broken `black` install.

tests/user_commands/tests.py

@@ -1,6 +1,8 @@
 import os
+import sys
 from argparse import ArgumentDefaultsHelpFormatter
 from io import StringIO
+from pathlib import Path
 from unittest import mock
 
 from admin_scripts.tests import AdminScriptTestCase
@@ -15,13 +17,15 @@
     is_ignored_path,
     normalize_path_patterns,
     popen_wrapper,
+    run_formatters,
 )
 from django.db import connection
 from django.test import SimpleTestCase, override_settings
 from django.test.utils import captured_stderr, extend_sys_path
 from django.utils import translation
 
 from .management.commands import dance
+from .utils import AssertFormatterFailureCaughtContext
 
 
 # A minimal set of apps to avoid system checks running on all apps.
@@ -535,3 +539,24 @@ def test_is_ignored_path_false(self):
     def test_normalize_path_patterns_truncates_wildcard_base(self):
         expected = [os.path.normcase(p) for p in ["foo/bar", "bar/*/"]]
         self.assertEqual(normalize_path_patterns(["foo/bar/*", "bar/*/"]), expected)
+
+    def test_run_formatters_handles_oserror_for_black_path(self):
+        cases = [
+            (FileNotFoundError, "nonexistent"),
+            (
+                OSError if sys.platform == "win32" else PermissionError,
+                str(Path(__file__).parent / "test_files" / "black"),
+            ),
+        ]
+        for exception, location in cases:
+            with (
+                self.subTest(exception.__qualname__),
+                AssertFormatterFailureCaughtContext(
+                    self, shutil_which_result=location
+                ) as ctx,
+            ):
+                run_formatters([], stderr=ctx.stderr)
+                parsed_error = ctx.stderr.getvalue()
+                self.assertIn(exception.__qualname__, parsed_error)
+                if sys.platform != "win32":
+                    self.assertIn(location, parsed_error)

tests/user_commands/utils.py

@@ -0,0 +1,23 @@
+from io import StringIO
+from unittest import mock
+
+
+class AssertFormatterFailureCaughtContext:
+
+    def __init__(self, test, shutil_which_result="nonexistent"):
+        self.stdout = StringIO()
+        self.stderr = StringIO()
+        self.test = test
+        self.shutil_which_result = shutil_which_result
+
+    def __enter__(self):
+        self.mocker = mock.patch(
+            "django.core.management.utils.shutil.which",
+            return_value=self.shutil_which_result,
+        )
+        self.mocker.start()
+        return self
+
+    def __exit__(self, exc_type, exc_value, traceback):
+        self.mocker.stop()
+        self.test.assertIn("Formatters failed to launch", self.stderr.getvalue())