From 56d83168a92982227c1ced9ac51391c7545f70da Mon Sep 17 00:00:00 2001 From: Rob Mohr Date: Sat, 8 Oct 2022 00:32:00 +0000 Subject: [PATCH] pw_presubmit: Add keep_sorted check Add a keep_sorted presubmit check inspired by the corresponding internal tool: http://go/keep-sorted. Also add 'pw keep-sorted' which sorts these blocks automatically. Bug: b/250875082 Change-Id: Id8d74c942c93176df8b2881339fbda2cc797346e Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/112211 Commit-Queue: Auto-Submit Reviewed-by: Wyatt Hepler Pigweed-Auto-Submit: Rob Mohr --- PW_PLUGINS | 3 +- pw_cli/py/pw_cli/pw_command_plugins.py | 2 + pw_presubmit/docs.rst | 20 ++ pw_presubmit/py/BUILD.gn | 2 + pw_presubmit/py/keep_sorted_test.py | 104 +++++++ pw_presubmit/py/pw_presubmit/keep_sorted.py | 277 ++++++++++++++++++ .../py/pw_presubmit/pigweed_presubmit.py | 37 ++- pw_presubmit/py/pw_presubmit/presubmit.py | 15 +- 8 files changed, 446 insertions(+), 14 deletions(-) create mode 100644 pw_presubmit/py/keep_sorted_test.py create mode 100644 pw_presubmit/py/pw_presubmit/keep_sorted.py diff --git a/PW_PLUGINS b/PW_PLUGINS index e368610fe1..3a840adb57 100644 --- a/PW_PLUGINS +++ b/PW_PLUGINS @@ -10,7 +10,7 @@ # virtual environment. The function must have no required arguments and should # return an int to use as the exit code. -# Pigweed's presubmit check script +# keep-sorted: start format pw_presubmit.format_code _pigweed_upstream_main heap-viewer pw_allocator.heap_viewer main ide pw_ide.__main__ main @@ -19,3 +19,4 @@ presubmit pw_presubmit.pigweed_presubmit main requires pw_cli.requires main rpc pw_hdlc.rpc_console main console pw_console.__main__ main +# keep-sorted: end diff --git a/pw_cli/py/pw_cli/pw_command_plugins.py b/pw_cli/py/pw_cli/pw_command_plugins.py index d8e7c443b0..4282f2e42c 100644 --- a/pw_cli/py/pw_cli/pw_command_plugins.py +++ b/pw_cli/py/pw_cli/pw_command_plugins.py @@ -33,6 +33,8 @@ def _register_builtin_plugins(registry: plugins.Registry) -> None: registry.register_by_name('python-packages', 'pw_env_setup.python_packages', 'main') registry.register_by_name('format', 'pw_presubmit.format_code', 'main') + registry.register_by_name('keep-sorted', 'pw_presubmit.keep_sorted', + 'main') registry.register_by_name('logdemo', 'pw_cli.log', 'main') registry.register_by_name('module', 'pw_module.__main__', 'main') registry.register_by_name('test', 'pw_unit_test.test_runner', 'main') diff --git a/pw_presubmit/docs.rst b/pw_presubmit/docs.rst index 227839b034..72b6372278 100644 --- a/pw_presubmit/docs.rst +++ b/pw_presubmit/docs.rst @@ -150,6 +150,26 @@ all use language-specific formatters like clang-format or yapf. These will suggest fixes using ``pw format --fix``. +Sorted Blocks +^^^^^^^^^^^^^ +Blocks of code can be required to be kept in sorted order using comments like +the following: + +.. code-block:: + + # keep-sorted: start + bar + baz + foo + # keep-sorted: end + +This can be included by adding ``pw_presubmit.keep_sorted.keep_sorted`` to a +presubmit program. + +These will suggest fixes using ``pw keep-sorted --fix``. + +Future versions may support multiline list items and case-insensitive sorting. + #pragma once ^^^^^^^^^^^^ There's a ``pragma_once`` check that confirms the first non-comment line of diff --git a/pw_presubmit/py/BUILD.gn b/pw_presubmit/py/BUILD.gn index 221fc156a5..179382a38d 100644 --- a/pw_presubmit/py/BUILD.gn +++ b/pw_presubmit/py/BUILD.gn @@ -31,6 +31,7 @@ pw_python_package("py") { "pw_presubmit/git_repo.py", "pw_presubmit/inclusive_language.py", "pw_presubmit/install_hook.py", + "pw_presubmit/keep_sorted.py", "pw_presubmit/npm_presubmit.py", "pw_presubmit/pigweed_presubmit.py", "pw_presubmit/presubmit.py", @@ -41,6 +42,7 @@ pw_python_package("py") { ] tests = [ "git_repo_test.py", + "keep_sorted_test.py", "presubmit_test.py", "tools_test.py", ] diff --git a/pw_presubmit/py/keep_sorted_test.py b/pw_presubmit/py/keep_sorted_test.py new file mode 100644 index 0000000000..593303e466 --- /dev/null +++ b/pw_presubmit/py/keep_sorted_test.py @@ -0,0 +1,104 @@ +#!/usr/bin/env python3 +# Copyright 2022 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 keep_sorted.""" + +from pathlib import Path +import tempfile +import unittest +from unittest.mock import MagicMock + +from pw_presubmit import keep_sorted + +# Only include these literals here so keep_sorted doesn't try to reorder later +# test lines. +START = keep_sorted.START +END = keep_sorted.END + +# pylint: disable=attribute-defined-outside-init + + +class TestKeepSorted(unittest.TestCase): + """Test KeepSorted class""" + def _run(self, contents: str) -> None: + self.ctx = MagicMock() + self.ctx.fail = MagicMock() + + with tempfile.TemporaryDirectory() as tempdir: + path = Path(tempdir) / 'foo' + + with path.open('w') as outs: + outs.write(contents) + + # pylint: disable=protected-access + self.sorter = keep_sorted._FileSorter(self.ctx, path) + + # pylint: enable=protected-access + + self.sorter.sort() + + # Truncate the file so it's obvious whether write() changed + # anything. + with path.open('w') as outs: + outs.write('') + + self.sorter.write(path) + with path.open() as ins: + self.contents = ins.read() + + def test_missing_end(self) -> None: + with self.assertRaises(keep_sorted.KeepSortedParsingError): + self._run(f'{START}\n') + + def test_missing_start(self) -> None: + with self.assertRaises(keep_sorted.KeepSortedParsingError): + self._run(f'{END}: end\n') + + def test_repeated_start(self) -> None: + with self.assertRaises(keep_sorted.KeepSortedParsingError): + self._run(f'{START}\n{START}\n') + + def test_already_sorted(self) -> None: + self._run(f'{START}\n1\n2\n3\n4\n{END}\n') + self.ctx.fail.assert_not_called() + self.assertEqual(self.contents, '') + + def test_not_sorted(self) -> None: + self._run(f'{START}\n4\n3\n2\n1\n{END}\n') + self.ctx.fail.assert_called() + self.assertEqual(self.contents, f'{START}\n1\n2\n3\n4\n{END}\n') + + def test_prefix_sorted(self) -> None: + self._run(f'foo\nbar\n{START}\n1\n2\n{END}\n') + self.ctx.fail.assert_not_called() + self.assertEqual(self.contents, '') + + def test_prefix_not_sorted(self) -> None: + self._run(f'foo\nbar\n{START}\n2\n1\n{END}\n') + self.ctx.fail.assert_called() + self.assertEqual(self.contents, f'foo\nbar\n{START}\n1\n2\n{END}\n') + + def test_suffix_sorted(self) -> None: + self._run(f'{START}\n1\n2\n{END}\nfoo\nbar\n') + self.ctx.fail.assert_not_called() + self.assertEqual(self.contents, '') + + def test_suffix_not_sorted(self) -> None: + self._run(f'{START}\n2\n1\n{END}\nfoo\nbar\n') + self.ctx.fail.assert_called() + self.assertEqual(self.contents, f'{START}\n1\n2\n{END}\nfoo\nbar\n') + + +if __name__ == '__main__': + unittest.main() diff --git a/pw_presubmit/py/pw_presubmit/keep_sorted.py b/pw_presubmit/py/pw_presubmit/keep_sorted.py new file mode 100644 index 0000000000..333637f72f --- /dev/null +++ b/pw_presubmit/py/pw_presubmit/keep_sorted.py @@ -0,0 +1,277 @@ +# Copyright 2022 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. +"""Keep specified lists sorted.""" + +import argparse +import dataclasses +import difflib +import logging +import os +from pathlib import Path +import re +import sys +from typing import Collection, List, Optional, Pattern, Sequence, Union + +import pw_cli +from . import cli, git_repo, presubmit, tools + +_COLOR = pw_cli.color.colors() +_LOG: logging.Logger = logging.getLogger(__name__) + +# Ignore a whole section. Please do not change the order of these lines. +_START = re.compile(r'keep-sorted: (begin|start)', re.IGNORECASE) +_END = re.compile(r'keep-sorted: (stop|end)', re.IGNORECASE) + +# Only include these literals here so keep_sorted doesn't try to reorder later +# test lines. +START = 'keep-sorted: start' +END = 'keep-sorted: end' + + +@dataclasses.dataclass +class KeepSortedContext: + paths: List[Path] + fix: bool + failed: bool = False + + def fail(self, + description: str = '', + path: Path = None, + line: int = None) -> None: + if not self.fix: + self.failed = True + + line_part: str = '' + if line is not None: + line_part = f'{line}:' + + log = _LOG.error + if self.fix: + log = _LOG.warning + + if path: + log('%s:%s %s', path, line_part, description) + else: + log('%s', description) + + +class KeepSortedParsingError(presubmit.PresubmitFailure): + pass + + +class _FileSorter: + def __init__(self, ctx: Union[presubmit.PresubmitContext, + KeepSortedContext], path: Path): + self.ctx = ctx + self.path: Path = path + self.all_lines: List[str] = [] + self.changed: bool = False + + def _process_block(self, start_line: str, lines: List[str], end_line: str, + i: int) -> Sequence[str]: + sorted_lines = sorted(lines) + + if lines != sorted_lines: + self.changed = True + self.ctx.fail('keep-sorted block is not sorted', self.path, i) + _LOG.info(' %s', start_line.rstrip()) + diff = difflib.Differ() + for dline in diff.compare( + [x.rstrip() for x in lines], + [x.rstrip() for x in sorted_lines], + ): + if dline.startswith('-'): + dline = _COLOR.red(dline) + elif dline.startswith('+'): + dline = _COLOR.green(dline) + _LOG.info(dline) + _LOG.info(' %s', end_line.rstrip()) + + return sorted_lines + + def _parse_file(self, ins): + in_block: bool = False + start_line: Optional[str] = None + end_line: Optional[str] = None + lines: List[str] = [] + + for i, line in enumerate(ins, start=1): + if in_block: + if _START.search(line): + raise KeepSortedParsingError( + f'found {line.strip()!r} inside keep-sorted block', + self.path, i) + + if _END.search(line): + _LOG.debug('Found end line %d %r', i, line) + end_line = line + in_block = False + assert start_line # Implicitly cast from Optional. + self.all_lines.extend( + self._process_block(start_line, lines, end_line, i)) + start_line = end_line = None + self.all_lines.append(line) + lines = [] + + else: + _LOG.debug('Adding to block line %d %r', i, line) + lines.append(line) + + elif _START.search(line): + _LOG.debug('Found start line %d %r', i, line) + start_line = line + in_block = True + self.all_lines.append(line) + + elif _END.search(line): + raise KeepSortedParsingError( + f'found {line.strip()!r} outside keep-sorted block', + self.path, i) + + else: + self.all_lines.append(line) + + if in_block: + raise KeepSortedParsingError( + f'found EOF while looking for "{END}"', self.path) + + def sort(self) -> None: + """Check for unsorted keep-sorted blocks.""" + _LOG.debug('Evaluating path %s', self.path) + try: + with self.path.open() as ins: + _LOG.debug('Processing %s', self.path) + self._parse_file(ins) + + except UnicodeDecodeError: + # File is not text, like a gif. + _LOG.debug('File %s is not a text file', self.path) + + def write(self, path: Path = None) -> None: + if not self.changed: + return + if not path: + path = self.path + with path.open('w') as outs: + outs.writelines(self.all_lines) + _LOG.info('Applied keep-sorted changes to %s', path) + + +def _print_howto_fix(paths: Sequence[Path]) -> None: + def path_relative_to_cwd(path): + try: + return Path(path).resolve().relative_to(Path.cwd().resolve()) + except ValueError: + return Path(path).resolve() + + message = (f' pw keep-sorted --fix {path_relative_to_cwd(path)}' + for path in paths) + _LOG.warning('To sort these blocks, run:\n\n%s\n', '\n'.join(message)) + + +def _process_files( + ctx: Union[presubmit.PresubmitContext, + KeepSortedContext]) -> Sequence[Path]: + fix = getattr(ctx, 'fix', False) + changed_paths = [] + + for path in ctx.paths: + if path.is_symlink() or path.is_dir(): + continue + + try: + sorter = _FileSorter(ctx, path) + + sorter.sort() + if sorter.changed: + changed_paths.append(path) + if fix: + sorter.write() + + except KeepSortedParsingError as exc: + ctx.fail(str(exc)) + + return changed_paths + + +@presubmit.Check +def keep_sorted(ctx: presubmit.PresubmitContext) -> None: + """Presubmit check that ensures specified lists remain sorted.""" + + changed_paths = _process_files(ctx) + + if changed_paths: + _print_howto_fix(changed_paths) + + +def parse_args() -> argparse.Namespace: + """Creates an argument parser and parses arguments.""" + + parser = argparse.ArgumentParser(description=__doc__) + cli.add_path_arguments(parser) + parser.add_argument('--fix', + action='store_true', + help='Apply fixes in place.') + + return parser.parse_args() + + +def keep_sorted_in_repo(paths: Collection[Union[Path, str]], fix: bool, + exclude: Collection[Pattern[str]], base: str) -> int: + """Checks or fixes keep-sorted blocks for files in a Git repo.""" + + files = [Path(path).resolve() for path in paths if os.path.isfile(path)] + repo = git_repo.root() if git_repo.is_repo() else None + + # Implement a graceful fallback in case the tracking branch isn't available. + if (base == git_repo.TRACKING_BRANCH_ALIAS + and not git_repo.tracking_branch(repo)): + _LOG.warning( + 'Failed to determine the tracking branch, using --base HEAD~1 ' + 'instead of listing all files') + base = 'HEAD~1' + + # If this is a Git repo, list the original paths with git ls-files or diff. + if repo: + project_root = Path(pw_cli.env.pigweed_environment().PW_PROJECT_ROOT) + _LOG.info( + 'Sorting %s', + git_repo.describe_files(repo, Path.cwd(), base, paths, exclude, + project_root)) + + # Add files from Git and remove duplicates. + files = sorted( + set(tools.exclude_paths(exclude, git_repo.list_files(base, paths))) + | set(files)) + elif base: + _LOG.critical( + 'A base commit may only be provided if running from a Git repo') + return 1 + + ctx = KeepSortedContext(paths=files, fix=fix) + changed_paths = _process_files(ctx) + + if not fix and changed_paths: + _print_howto_fix(changed_paths) + + return int(ctx.failed) + + +def main() -> int: + return keep_sorted_in_repo(**vars(parse_args())) + + +if __name__ == '__main__': + pw_cli.log.install(logging.INFO) + sys.exit(main()) diff --git a/pw_presubmit/py/pw_presubmit/pigweed_presubmit.py b/pw_presubmit/py/pw_presubmit/pigweed_presubmit.py index 88bfe0962f..6ac8ce836c 100755 --- a/pw_presubmit/py/pw_presubmit/pigweed_presubmit.py +++ b/pw_presubmit/py/pw_presubmit/pigweed_presubmit.py @@ -46,6 +46,7 @@ call, filter_paths, inclusive_language, + keep_sorted, npm_presubmit, plural, presubmit, @@ -416,6 +417,7 @@ def cmake_gcc(ctx: PresubmitContext): # TODO(b/235882003): Slowly remove targets from here that work with bazel until # none remain. _TARGETS_THAT_DO_NOT_BUILD_WITH_BAZEL = ( + # keep-sorted: start '-//pw_arduino_build', '-//pw_blob_store/...:all', '-//pw_boot/...:all', @@ -439,8 +441,7 @@ def cmake_gcc(ctx: PresubmitContext): '-//pw_snapshot:metadata_proto_py_pb2_genproto', '-//pw_snapshot:snapshot_proto_py_pb2', '-//pw_snapshot:snapshot_proto_py_pb2_genproto', - # TODO(b/232427554): Get pw_software_update to build. - '-//pw_software_update:bundled_update_py_pb2', + '-//pw_software_update:bundled_update_py_pb2', # TODO(b/232427554): Fix '-//pw_software_update:bundled_update_py_pb2_genproto', '-//pw_software_update:bundled_update_service', '-//pw_software_update:bundled_update_service_test', @@ -464,8 +465,7 @@ def cmake_gcc(ctx: PresubmitContext): '-//pw_tls_client/...:all', '-//pw_tls_client_boringssl/...:all', '-//pw_tls_client_mbedtls/...:all', - # TODO(b/241456982) Remove when passing. - '-//pw_tokenizer:tokenizer_proto_py_pb2', + '-//pw_tokenizer:tokenizer_proto_py_pb2', # TODO(b/241456982) Fix '-//pw_tokenizer:tokenizer_proto_py_pb2_genproto', '-//pw_trace/...:all', '-//pw_trace_tokenized/...:all', @@ -479,6 +479,7 @@ def cmake_gcc(ctx: PresubmitContext): '-//targets/rp2040/...:all', '-//third_party/boringssl/...:all', '-//third_party/micro_ecc/...:all', + # keep-sorted: end ) # TODO(b/235882003): Slowly remove targets from here that work with bazel until @@ -569,10 +570,13 @@ def edit_compile_commands(in_path: Path, out_path: Path, _EXCLUDE_FROM_COPYRIGHT_NOTICE: Sequence[str] = ( # Configuration + # keep-sorted: start r'^(?:.+/)?\..+$', r'\bPW_PLUGINS$', r'\bconstraint.list$', + # keep-sorted: end # Metadata + # keep-sorted: start r'^docker/tag$', r'\bAUTHORS$', r'\bLICENSE$', @@ -583,7 +587,9 @@ def edit_compile_commands(in_path: Path, out_path: Path, r'\bpackage.json$', r'\byarn.lock$', r'\bpackage-lock.json$', + # keep-sorted: end # Data files + # keep-sorted: start r'\.bin$', r'\.csv$', r'\.elf$', @@ -594,16 +600,23 @@ def edit_compile_commands(in_path: Path, out_path: Path, r'\.png$', r'\.svg$', r'\.xml$', + # keep-sorted: end # Documentation + # keep-sorted: start r'\.md$', r'\.rst$', + # keep-sorted: end # Generated protobuf files + # keep-sorted: start r'\.pb\.h$', r'\.pb\.c$', r'\_pb2.pyi?$', + # keep-sorted: end # Diff/Patch files + # keep-sorted: start r'\.diff$', r'\.patch$', + # keep-sorted: end ) # Regular expression for the copyright comment. "\1" refers to the comment @@ -847,11 +860,9 @@ def renode_check(ctx: PresubmitContext): # OTHER_CHECKS = ( - # Build that attempts to duplicate the build OSS-Fuzz does. Currently - # failing. - oss_fuzz_build, - # TODO(b/235277910): Enable all Bazel tests when they're fixed. - bazel_test, + # keep-sorted: start + oss_fuzz_build, # Attempts to duplicate OSS-Fuzz. Currently failing. + bazel_test, # TODO(b/235277910): Enable all Bazel tests when they're fixed. cmake_clang, cmake_gcc, gn_boringssl_build, @@ -861,29 +872,35 @@ def renode_check(ctx: PresubmitContext): gn_combined_build_check, gn_clang_build, gn_gcc_build, + keep_sorted.keep_sorted, pw_transfer_integration_test, renode_check, static_analysis, stm32f429i, npm_presubmit.npm_test, todo_check.create(todo_check.BUGS_OR_USERNAMES), + # keep-sorted: end ) # The misc program differs from other_checks in that checks in the misc # program block CQ on Linux. MISC = ( + # keep-sorted: start gn_nanopb_build, gn_pw_system_demo_build, gn_teensy_build, + # keep-sorted: end ) SANITIZERS = (cpp_checks.all_sanitizers(), ) # TODO(b/243380637) Merge into SECURITY. CRYPTO = ( + # keep-sorted: start gn_crypto_mbedtls_build, gn_crypto_boringssl_build, gn_crypto_micro_ecc_build, + # keep-sorted: end ) SECURITY = ( @@ -948,6 +965,7 @@ def renode_check(ctx: PresubmitContext): ) PROGRAMS = Programs( + # keep-sorted: start full=FULL, lintformat=LINTFORMAT, misc=MISC, @@ -956,6 +974,7 @@ def renode_check(ctx: PresubmitContext): crypto=CRYPTO, sanitizers=SANITIZERS, security=SECURITY, + # keep-sorted: end ) diff --git a/pw_presubmit/py/pw_presubmit/presubmit.py b/pw_presubmit/py/pw_presubmit/presubmit.py index dad5362818..c5b0014b6f 100644 --- a/pw_presubmit/py/pw_presubmit/presubmit.py +++ b/pw_presubmit/py/pw_presubmit/presubmit.py @@ -98,8 +98,15 @@ def _box(style, left, middle, right, box=tools.make_box('><>')) -> str: class PresubmitFailure(Exception): """Optional exception to use for presubmit failures.""" - def __init__(self, description: str = '', path: Path = None): - super().__init__(f'{path}: {description}' if path else description) + def __init__(self, + description: str = '', + path: Path = None, + line: int = None): + line_part: str = '' + if line is not None: + line_part = f'{line}:' + super().__init__( + f'{path}:{line_part} {description}' if path else description) class _Result(enum.Enum): @@ -227,13 +234,13 @@ class PresubmitContext: def failed(self) -> bool: return self._failed - def fail(self, description: str, path: Path = None): + def fail(self, description: str, path: Path = None, line: int = None): """Add a failure to this presubmit step. If this is called at least once the step fails, but not immediately—the check is free to continue and possibly call this method again. """ - _LOG.warning('%s', PresubmitFailure(description, path)) + _LOG.warning('%s', PresubmitFailure(description, path, line)) self._failed = True