pw_presubmit: Refactor GN formatting support
Refactors GN build file formatting support to be more modular.
Bug: b/326309165
Change-Id: I4e20641cf51d65a12ab743ff97e02b106287f747
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/194415
Pigweed-Auto-Submit: Armando Montanez <amontanez@google.com>
Commit-Queue: Auto-Submit <auto-submit@pigweed-service-accounts.iam.gserviceaccount.com>
Reviewed-by: Anthony DiGirolamo <tonymd@google.com>
diff --git a/pw_presubmit/BUILD.gn b/pw_presubmit/BUILD.gn
index cfe8f5b..d700cfe 100644
--- a/pw_presubmit/BUILD.gn
+++ b/pw_presubmit/BUILD.gn
@@ -27,6 +27,7 @@
"py/pw_presubmit/cli.py",
"py/pw_presubmit/format/core.py",
"py/pw_presubmit/format/cpp.py",
+ "py/pw_presubmit/format/gn.py",
"py/pw_presubmit/presubmit.py",
]
}
diff --git a/pw_presubmit/format.rst b/pw_presubmit/format.rst
index 79b295b..07daf96 100644
--- a/pw_presubmit/format.rst
+++ b/pw_presubmit/format.rst
@@ -36,3 +36,7 @@
.. autoclass:: pw_presubmit.format.cpp.ClangFormatFormatter
:members:
:noindex:
+
+.. autoclass:: pw_presubmit.format.gn.GnFormatter
+ :members:
+ :noindex:
diff --git a/pw_presubmit/py/BUILD.gn b/pw_presubmit/py/BUILD.gn
index f38b1de..15c1dd7 100644
--- a/pw_presubmit/py/BUILD.gn
+++ b/pw_presubmit/py/BUILD.gn
@@ -30,6 +30,7 @@
"pw_presubmit/format/__init__.py",
"pw_presubmit/format/core.py",
"pw_presubmit/format/cpp.py",
+ "pw_presubmit/format/gn.py",
"pw_presubmit/format/test_data/__init__.py",
"pw_presubmit/format_code.py",
"pw_presubmit/git_repo.py",
@@ -57,15 +58,20 @@
"pw_presubmit/format/test_data/clang_format_config",
"pw_presubmit/format/test_data/cpp_test_data.cc",
"pw_presubmit/format/test_data/cpp_test_data_golden.cc",
+ "pw_presubmit/format/test_data/gn_test_data.gn",
+ "pw_presubmit/format/test_data/gn_test_data_golden.gn",
+ "pw_presubmit/format/test_data/malformed_file.txt",
]
tests = [
"bazel_parser_test.py",
"context_test.py",
"cpp_checks_test.py",
"cpp_format_test.py",
+ "format_testing_utils.py",
"format_core_test.py",
"git_repo_test.py",
"gitmodules_test.py",
+ "gn_format_test.py",
"keep_sorted_test.py",
"ninja_parser_test.py",
"presubmit_test.py",
diff --git a/pw_presubmit/py/cpp_format_test.py b/pw_presubmit/py/cpp_format_test.py
index 2c4f829..aa59a19 100644
--- a/pw_presubmit/py/cpp_format_test.py
+++ b/pw_presubmit/py/cpp_format_test.py
@@ -15,12 +15,11 @@
import importlib.resources
from pathlib import Path
-import subprocess
from tempfile import TemporaryDirectory
from typing import Final, Sequence
import unittest
-from pw_presubmit.format.core import ToolRunner
+from format_testing_utils import CapturingToolRunner
from pw_presubmit.format.cpp import ClangFormatFormatter
_TEST_DATA_FILES = importlib.resources.files('pw_presubmit.format.test_data')
@@ -34,18 +33,6 @@
)
-class CapturingToolRunner(ToolRunner):
- def __init__(self):
- self.command_history: list[str] = []
-
- def _run_tool(
- self, tool: str, args, **kwargs
- ) -> subprocess.CompletedProcess:
- cmd = [tool] + args
- self.command_history.append(' '.join([str(arg) for arg in cmd]))
- return subprocess.run(cmd, **kwargs)
-
-
class TestClangFormatFormatter(unittest.TestCase):
"""Tests for the ClangFormatFormatter."""
diff --git a/pw_presubmit/py/format_testing_utils.py b/pw_presubmit/py/format_testing_utils.py
new file mode 100644
index 0000000..825f6a3
--- /dev/null
+++ b/pw_presubmit/py/format_testing_utils.py
@@ -0,0 +1,37 @@
+# Copyright 2024 The Pigweed Authors
+#
+# Licensed under the Apache License, Version 2.0 (the "License"); you may not
+# use this file except in compliance with the License. You may obtain a copy of
+# the License at
+#
+# https://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations under
+# the License.
+"""Utilities common to format tests."""
+
+import subprocess
+from typing import List
+
+from pw_presubmit.format.core import ToolRunner
+
+
+class CapturingToolRunner(ToolRunner):
+ """A ToolRunner that captures executed commands and their arguments."""
+
+ def __init__(self):
+ self.command_history: List[str] = []
+
+ def _run_tool(
+ self, tool: str, args, **kwargs
+ ) -> subprocess.CompletedProcess:
+ """Runs the requested tool with the provided arguments.
+
+ The full command is appended to ``command_history``.
+ """
+ cmd = [tool] + args
+ self.command_history.append(' '.join([str(arg) for arg in cmd]))
+ return subprocess.run(cmd, **kwargs)
diff --git a/pw_presubmit/py/gn_format_test.py b/pw_presubmit/py/gn_format_test.py
new file mode 100644
index 0000000..e49ca87
--- /dev/null
+++ b/pw_presubmit/py/gn_format_test.py
@@ -0,0 +1,151 @@
+# Copyright 2024 The Pigweed Authors
+#
+# Licensed under the Apache License, Version 2.0 (the "License"); you may not
+# use this file except in compliance with the License. You may obtain a copy of
+# the License at
+#
+# https://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations under
+# the License.
+"""Tests for GN's built-in formatter."""
+
+import importlib.resources
+from pathlib import Path
+from tempfile import TemporaryDirectory
+import unittest
+
+from format_testing_utils import CapturingToolRunner
+from pw_presubmit.format.gn import GnFormatter
+
+
+_TEST_DATA_FILES = importlib.resources.files('pw_presubmit.format.test_data')
+_TEST_SRC_FILE = _TEST_DATA_FILES / 'gn_test_data.gn'
+_TEST_GOLDEN = _TEST_DATA_FILES / 'gn_test_data_golden.gn'
+_TEST_MALFORMED = _TEST_DATA_FILES / 'malformed_file.txt'
+
+
+class TestGnFormatter(unittest.TestCase):
+ """Tests for the GnFormatter."""
+
+ def test_check_file(self):
+ tool_runner = CapturingToolRunner()
+ formatter = GnFormatter()
+ formatter.run_tool = tool_runner
+
+ result = formatter.format_file_in_memory(
+ _TEST_SRC_FILE, _TEST_SRC_FILE.read_bytes()
+ )
+ self.assertTrue(result.ok)
+ self.assertEqual(result.error_message, None)
+ self.assertMultiLineEqual(
+ result.formatted_file_contents.decode(), _TEST_GOLDEN.read_text()
+ )
+
+ self.assertEqual(
+ tool_runner.command_history.pop(0),
+ ' '.join(
+ (
+ 'gn',
+ 'format',
+ '--stdin',
+ )
+ ),
+ )
+
+ def test_check_file_error(self):
+ tool_runner = CapturingToolRunner()
+ formatter = GnFormatter()
+ formatter.run_tool = tool_runner
+
+ result = formatter.format_file_in_memory(
+ _TEST_MALFORMED, _TEST_MALFORMED.read_bytes()
+ )
+ self.assertFalse(result.ok)
+ self.assertTrue(result.error_message.startswith('ERROR at'))
+ self.assertTrue('Invalid token' in result.error_message)
+ self.assertEqual(result.formatted_file_contents, b'')
+
+ self.assertEqual(
+ tool_runner.command_history.pop(0),
+ ' '.join(
+ (
+ 'gn',
+ 'format',
+ '--stdin',
+ )
+ ),
+ )
+
+ def test_fix_file(self):
+ """Tests that formatting is properly applied to files."""
+
+ tool_runner = CapturingToolRunner()
+ formatter = GnFormatter()
+ formatter.run_tool = tool_runner
+
+ with TemporaryDirectory() as temp_dir:
+ file_to_fix = Path(temp_dir) / _TEST_SRC_FILE.name
+ file_to_fix.write_bytes(_TEST_SRC_FILE.read_bytes())
+
+ malformed_file = Path(temp_dir) / _TEST_MALFORMED.name
+ malformed_file.write_bytes(_TEST_MALFORMED.read_bytes())
+
+ errors = list(formatter.format_files([file_to_fix, malformed_file]))
+
+ # Should see three separate commands, one where we try to format
+ # both files together, and then the fallback logic that formats
+ # them individually to isolate errors.
+ self.assertEqual(
+ tool_runner.command_history.pop(0),
+ ' '.join(
+ (
+ 'gn',
+ 'format',
+ str(file_to_fix),
+ str(malformed_file),
+ )
+ ),
+ )
+
+ self.assertEqual(
+ tool_runner.command_history.pop(0),
+ ' '.join(
+ (
+ 'gn',
+ 'format',
+ str(file_to_fix),
+ )
+ ),
+ )
+
+ self.assertEqual(
+ tool_runner.command_history.pop(0),
+ ' '.join(
+ (
+ 'gn',
+ 'format',
+ str(malformed_file),
+ )
+ ),
+ )
+
+ # Check good build file.
+ self.assertMultiLineEqual(
+ file_to_fix.read_text(), _TEST_GOLDEN.read_text()
+ )
+
+ # Check malformed file.
+ self.assertEqual(len(errors), 1)
+ malformed_files = [malformed_file]
+ for file_path, error in errors:
+ self.assertIn(file_path, malformed_files)
+ self.assertFalse(error.ok)
+ self.assertTrue(error.error_message.startswith('ERROR at'))
+
+
+if __name__ == '__main__':
+ unittest.main()
diff --git a/pw_presubmit/py/pw_presubmit/format/gn.py b/pw_presubmit/py/pw_presubmit/format/gn.py
new file mode 100644
index 0000000..6223b1d
--- /dev/null
+++ b/pw_presubmit/py/pw_presubmit/format/gn.py
@@ -0,0 +1,102 @@
+# Copyright 2024 The Pigweed Authors
+#
+# Licensed under the Apache License, Version 2.0 (the "License"); you may not
+# use this file except in compliance with the License. You may obtain a copy of
+# the License at
+#
+# https://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations under
+# the License.
+"""Code formatter plugin for GN build files."""
+
+from pathlib import Path
+from typing import Final, Iterable, Iterator, Sequence, Tuple
+
+from pw_presubmit.format.core import (
+ FileFormatter,
+ FormattedFileContents,
+ FormatFixStatus,
+)
+
+
+class GnFormatter(FileFormatter):
+ """A formatter that runs `gn format` on files."""
+
+ DEFAULT_FLAGS: Final[Sequence[str]] = ()
+
+ def __init__(self, tool_flags: Sequence[str] = DEFAULT_FLAGS, **kwargs):
+ super().__init__(**kwargs)
+ self.gn_format_flags = list(tool_flags)
+
+ def format_file_in_memory(
+ self, file_path: Path, file_contents: bytes
+ ) -> FormattedFileContents:
+ """Uses ``gn format`` to check the formatting of the requested file.
+
+ The file at ``file_path`` is NOT modified by this check.
+
+ Returns:
+ A populated
+ :py:class:`pw_presubmit.format.core.FormattedFileContents` that
+ contains either the result of formatting the file, or an error
+ message.
+ """
+ # `gn format --dry-run` only signals which files need to be updated,
+ # not what the effects are. To get the formatted result, we pipe the
+ # file in via stdin and then return the result.
+ proc = self.run_tool(
+ 'gn',
+ ['format'] + self.gn_format_flags + ['--stdin'],
+ input=file_contents,
+ )
+ ok = proc.returncode == 0
+ return FormattedFileContents(
+ ok=ok,
+ formatted_file_contents=proc.stdout if ok else b'',
+ # `gn format` emits errors over stdout, not stderr.
+ error_message=None if ok else proc.stdout.decode(),
+ )
+
+ def format_file(self, file_path: Path) -> FormatFixStatus:
+ """Formats the provided file in-place using ``gn format``.
+
+ Returns:
+ A FormatFixStatus that contains relevant errors/warnings.
+ """
+ proc = self.run_tool(
+ 'gn',
+ ['format'] + self.gn_format_flags + [file_path],
+ )
+ ok = proc.returncode == 0
+ return FormatFixStatus(
+ ok=ok,
+ # `gn format` emits errors over stdout, not stderr.
+ error_message=None if ok else proc.stdout.decode(),
+ )
+
+ def format_files(
+ self, paths: Iterable[Path], keep_warnings: bool = True
+ ) -> Iterator[Tuple[Path, FormatFixStatus]]:
+ """Uses ``gn format`` to fix formatting of the specified files in-place.
+
+ Returns:
+ An iterator of ``Path`` and
+ :py:class:`pw_presubmit.format.core.FormatFixStatus` pairs for each
+ file that was not successfully formatted. If ``keep_warnings`` is
+ ``True``, any successful format operations with warnings will also
+ be returned.
+ """
+ # Try to format all files in one `gn` command.
+ proc = self.run_tool(
+ 'gn',
+ ['format'] + self.gn_format_flags + list(paths),
+ )
+
+ # If there's an error, fall back to per-file formatting to figure out
+ # which file has problems.
+ if proc.returncode != 0:
+ yield from super().format_files(paths, keep_warnings)
diff --git a/pw_presubmit/py/pw_presubmit/format/test_data/gn_test_data.gn b/pw_presubmit/py/pw_presubmit/format/test_data/gn_test_data.gn
new file mode 100644
index 0000000..6655eca
--- /dev/null
+++ b/pw_presubmit/py/pw_presubmit/format/test_data/gn_test_data.gn
@@ -0,0 +1,25 @@
+import("//build_overrides/pigweed.gni")
+
+import("$dir_pw_docgen/docs.gni")
+import("$dir_pw_unit_test/test.gni")
+import("$dir_pw_build/target_types.gni")
+import("$dir_pw_unit_test/test.gni")
+
+config("public_include_path") {
+ include_dirs = [ "public" ]
+ visibility = [ ":*" ]
+}
+
+pw_source_set("my_library") {
+ public = [ "public/my_library/foo.h" ]
+ deps = [":an", ":unsorted", ":list"]
+ public_configs = [ ":public_include_path",
+ ]
+}
+
+pw_doc_group("docs") { sources = [ "docs.rst" ] }
+
+pw_test_group("tests") {
+
+
+}
diff --git a/pw_presubmit/py/pw_presubmit/format/test_data/gn_test_data_golden.gn b/pw_presubmit/py/pw_presubmit/format/test_data/gn_test_data_golden.gn
new file mode 100644
index 0000000..cd6c044
--- /dev/null
+++ b/pw_presubmit/py/pw_presubmit/format/test_data/gn_test_data_golden.gn
@@ -0,0 +1,28 @@
+import("//build_overrides/pigweed.gni")
+
+import("$dir_pw_build/target_types.gni")
+import("$dir_pw_docgen/docs.gni")
+import("$dir_pw_unit_test/test.gni")
+import("$dir_pw_unit_test/test.gni")
+
+config("public_include_path") {
+ include_dirs = [ "public" ]
+ visibility = [ ":*" ]
+}
+
+pw_source_set("my_library") {
+ public = [ "public/my_library/foo.h" ]
+ deps = [
+ ":an",
+ ":list",
+ ":unsorted",
+ ]
+ public_configs = [ ":public_include_path" ]
+}
+
+pw_doc_group("docs") {
+ sources = [ "docs.rst" ]
+}
+
+pw_test_group("tests") {
+}
diff --git a/pw_presubmit/py/pw_presubmit/format/test_data/malformed_file.txt b/pw_presubmit/py/pw_presubmit/format/test_data/malformed_file.txt
new file mode 100644
index 0000000..3a08e00
--- /dev/null
+++ b/pw_presubmit/py/pw_presubmit/format/test_data/malformed_file.txt
@@ -0,0 +1,6 @@
+This is just a text file with random contents!
+Some formatters will take this and gracefully try to format it, others will
+flag that this file has syntax errors (which is unsurprising).
+
+Use this file to ensure errors are properly propagated.
+Some characters to potentially cause breakages: [[{\\\\**
diff --git a/pw_presubmit/py/pw_presubmit/format_code.py b/pw_presubmit/py/pw_presubmit/format_code.py
index cee4a60..bff782c 100755
--- a/pw_presubmit/py/pw_presubmit/format_code.py
+++ b/pw_presubmit/py/pw_presubmit/format_code.py
@@ -62,6 +62,7 @@
)
from pw_presubmit.format.core import FormattedDiff, FormatFixStatus
from pw_presubmit.format.cpp import ClangFormatFormatter
+from pw_presubmit.format.gn import GnFormatter
from pw_presubmit.tools import (
exclude_paths,
file_summary,
@@ -201,22 +202,19 @@
def check_gn_format(ctx: _Context) -> dict[Path, str]:
"""Checks formatting; returns {path: diff} for files with bad formatting."""
- return _check_files(
- ctx.paths,
- lambda _, data: log_run(
- ['gn', 'format', '--stdin'],
- input=data,
- stdout=subprocess.PIPE,
- check=True,
- ).stdout,
- ctx.dry_run,
+ formatter = GnFormatter(tool_runner=PresubmitToolRunner())
+ return _make_formatting_diff_dict(
+ formatter.get_formatting_diffs(
+ ctx.paths,
+ ctx.dry_run,
+ )
)
def fix_gn_format(ctx: _Context) -> dict[Path, str]:
"""Fixes formatting for the provided files in place."""
- log_run(['gn', 'format', *ctx.paths], check=True)
- return {}
+ formatter = GnFormatter(tool_runner=PresubmitToolRunner())
+ return _make_format_fix_error_output_dict(formatter.format_files(ctx.paths))
def check_bazel_format(ctx: _Context) -> dict[Path, str]:
