pw_presubmit: Update Python checks

Change top-level BUILD.gn python target to depend on both python and
target_support_packages in pw_env_setup.

Change presubmit steps in python_checks.py to use GN and include the
lint checks in Pigweed's lintformat program.

Change-Id: I895f663a8657a8be6aaa7ad37b6c11f09a964d80
Bug: 454
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/57700
Commit-Queue: Rob Mohr <mohrr@google.com>
Reviewed-by: Wyatt Hepler <hepler@google.com>

3 files changed, 50 insertions, 143 deletions

diff --git a/pw_presubmit/docs.rst b/pw_presubmit/docs.rst
index cd4ca6bdd..063f52296 100644
--- a/pw_presubmit/docs.rst
+++ b/pw_presubmit/docs.rst
@@ -116,6 +116,13 @@ There's a ``pragma_once`` check that confirms the first non-comment line of
 C/C++ headers is ``#pragma once``. This is enabled by adding
 ``pw_presubmit.pragma_once`` to a presubmit program.
 
+Python Checks
+=============
+There are two checks in the ``pw_presubmit.python_checks`` module, ``gn_lint``
+and ``gn_python_check``. They assume there's a top-level ``python`` GN target.
+``gn_lint`` runs Pylint and Mypy checks and ``gn_python_check`` runs Pylint,
+Mypy, and all Python tests.
+
 Inclusive Language
 ==================
 .. inclusive-language: disable
@@ -169,7 +176,7 @@ See ``pigweed_presubmit.py`` for a more complex presubmit check script example.
 
   import pw_presubmit
   from pw_presubmit import build, cli, environment, format_code, git_repo
-  from pw_presubmit import inclusive_language, python_checks, filter_paths
+  from pw_presubmit import inclusive_language, filter_paths, python_checks
   from pw_presubmit import PresubmitContext
   from pw_presubmit.install_hook import install_hook
 
@@ -177,19 +184,6 @@ See ``pigweed_presubmit.py`` for a more complex presubmit check script example.
   PROJECT_ROOT = Path(os.environ['MY_PROJECT_ROOT'])
   PIGWEED_ROOT = PROJECT_ROOT / 'pigweed'
 
-  #
-  # Initialization
-  #
-  def init_cipd(ctx: PresubmitContext):
-      environment.init_cipd(PIGWEED_ROOT, ctx.output_dir)
-
-
-  def init_virtualenv(ctx: PresubmitContext):
-      environment.init_virtualenv(PIGWEED_ROOT,
-                                  ctx.output_dir,
-                                  setup_py_roots=[PROJECT_ROOT])
-
-
   # Rerun the build if files with these extensions change.
   _BUILD_EXTENSIONS = frozenset(
       ['.rst', '.gn', '.gni', *format_code.C_FORMAT.extensions])
diff --git a/pw_presubmit/py/pw_presubmit/pigweed_presubmit.py b/pw_presubmit/py/pw_presubmit/pigweed_presubmit.py
index a78773c6e..fdad98f77 100755
--- a/pw_presubmit/py/pw_presubmit/pigweed_presubmit.py
+++ b/pw_presubmit/py/pw_presubmit/pigweed_presubmit.py
@@ -40,6 +40,7 @@ import pw_package.pigweed_packages
 from pw_presubmit import build, cli, format_code, git_repo, call, filter_paths
 import pw_presubmit.inclusive_language
 from pw_presubmit import plural, PresubmitContext, PresubmitFailure, Programs
+from pw_presubmit import python_checks
 from pw_presubmit.install_hook import install_hook
 
 _LOG = logging.getLogger(__name__)
@@ -230,18 +231,6 @@ def oss_fuzz_build(ctx: PresubmitContext):
     build.ninja(ctx.output_dir, "fuzzers")
 
 
-@filter_paths(endswith='.py')
-def python_checks(ctx: PresubmitContext):
-    build.gn_gen(ctx.root, ctx.output_dir)
-    build.ninja(
-        ctx.output_dir,
-        ':python.lint',
-        ':python.tests',
-        ':target_support_packages.lint',
-        ':target_support_packages.tests',
-    )
-
-
 def _run_cmake(ctx: PresubmitContext) -> None:
     build.install_package(ctx.package_root, 'nanopb')
 
@@ -834,7 +823,7 @@ OTHER_CHECKS = (
     stm32f429i,
 )
 
-LINTFORMAT = (
+_LINTFORMAT = (
     commit_message_format,
     copyright_notice,
     format_code.presubmit_checks(),
@@ -844,8 +833,13 @@ LINTFORMAT = (
     source_is_in_build_files,
 )
 
+LINTFORMAT = (
+    _LINTFORMAT,
+    python_checks.gn_lint,
+)
+
 QUICK = (
-    LINTFORMAT,
+    _LINTFORMAT,
     gn_quick_build_check,
     # TODO(pwbug/141): Re-enable CMake and Bazel for Mac after we have fixed the
     # the clang issues. The problem is that all clang++ invocations need the
@@ -854,7 +848,7 @@ QUICK = (
 )
 
 FULL = (
-    LINTFORMAT,
+    _LINTFORMAT,
     gn_host_build,
     gn_arm_build,
     gn_docs_build,
@@ -869,7 +863,7 @@ FULL = (
     gn_qemu_build if sys.platform != 'win32' else (),
     gn_qemu_clang_build if sys.platform != 'win32' else (),
     source_is_in_build_files,
-    python_checks,
+    python_checks.gn_python_check,
     build_env_setup,
     # Skip gn_teensy_build if running on Windows. The Teensycore installer is
     # an exe that requires an admin role.
diff --git a/pw_presubmit/py/pw_presubmit/python_checks.py b/pw_presubmit/py/pw_presubmit/python_checks.py
index 0f693e8ee..bfab37496 100644
--- a/pw_presubmit/py/pw_presubmit/python_checks.py
+++ b/pw_presubmit/py/pw_presubmit/python_checks.py
@@ -18,9 +18,8 @@ These checks assume that they are running in a preconfigured Python environment.
 
 import logging
 import os
-from pathlib import Path
 import sys
-from typing import Callable, Iterable, List, Set, Tuple
+from typing import Callable, Tuple
 
 try:
     import pw_presubmit
@@ -31,123 +30,41 @@ except ImportError:
         os.path.abspath(__file__))))
     import pw_presubmit
 
-from pw_presubmit import call, filter_paths
-from pw_presubmit.git_repo import python_packages_containing, list_files
-from pw_presubmit.git_repo import PythonPackage
+from pw_presubmit import build, filter_paths, PresubmitContext
 
 _LOG = logging.getLogger(__name__)
 
+_PYTHON_EXTENSIONS = ('.py', '.gn', '.gni')
 
-def run_module(*args, **kwargs):
-    return call('python', '-m', *args, **kwargs)
-
-
-TEST_PATTERNS = ('*_test.py', )
-
-
-@filter_paths(endswith='.py')
-def test_python_packages(ctx: pw_presubmit.PresubmitContext,
-                         patterns: Iterable[str] = TEST_PATTERNS) -> None:
-    """Finds and runs test files in Python package directories.
-
-    Finds the Python packages containing the affected paths, then searches
-    within that package for test files. All files matching the provided patterns
-    are executed with Python.
-    """
-    packages: List[PythonPackage] = []
-    for repo in ctx.repos:
-        packages += python_packages_containing(ctx.paths, repo=repo)[0]
-
-    if not packages:
-        _LOG.info('No Python packages were found.')
-        return
-
-    for package in packages:
-        for test in list_files(pathspecs=tuple(patterns),
-                               repo_path=package.root):
-            call('python', test)
-
-
-@filter_paths(endswith='.py')
-def pylint(ctx: pw_presubmit.PresubmitContext) -> None:
-    disable_checkers = [
-        # BUG(pwbug/22): Hanging indent check conflicts with YAPF 0.29. For
-        # now, use YAPF's version even if Pylint is doing the correct thing
-        # just to keep operations simpler. When YAPF upstream fixes the issue,
-        # delete this code.
-        #
-        # See also: https://github.com/google/yapf/issues/781
-        'bad-continuation',
-    ]
-    run_module(
-        'pylint',
-        '--jobs=0',
-        f'--disable={",".join(disable_checkers)}',
-        *ctx.paths,
-        cwd=ctx.root,
-    )
-
-
-@filter_paths(endswith='.py')
-def mypy(ctx: pw_presubmit.PresubmitContext) -> None:
-    """Runs mypy on all paths and their packages."""
-    packages: List[PythonPackage] = []
-    other_files: List[Path] = []
-
-    for repo, paths in ctx.paths_by_repo().items():
-        new_packages, files = python_packages_containing(paths, repo=repo)
-        packages += new_packages
-        other_files += files
-
-        for package in new_packages:
-            other_files += package.other_files
-
-    # Under some circumstances, mypy cannot check multiple Python files with the
-    # same module name. Group filenames so that no duplicates occur in the same
-    # mypy invocation. Also, omit setup.py from mypy checks.
-    filename_sets: List[Set[str]] = [set()]
-    path_sets: List[List[Path]] = [list(p.package for p in packages)]
-
-    for path in (p for p in other_files if p.name != 'setup.py'):
-        for filenames, paths in zip(filename_sets, path_sets):
-            if path.name not in filenames:
-                paths.append(path)
-                filenames.add(path.name)
-                break
-        else:
-            path_sets.append([path])
-            filename_sets.append({path.name})
-
-    env = os.environ.copy()
-    # Use this environment variable to force mypy to colorize output.
-    # See https://github.com/python/mypy/issues/7771
-    env['MYPY_FORCE_COLOR'] = '1'
-
-    for paths in path_sets:
-        run_module(
-            'mypy',
-            *paths,
-            '--pretty',
-            '--color-output',
-            '--show-error-codes',
-            # TODO(pwbug/146): Some imports from installed packages fail. These
-            # imports should be fixed and this option removed. See
-            # https://mypy.readthedocs.io/en/stable/installed_packages.html
-            '--ignore-missing-imports',
-            env=env)
-
-
-_LINT_CHECKS = (
-    pylint,
-    mypy,
-)
-
-_ALL_CHECKS = (
-    test_python_packages,
-    *_LINT_CHECKS,
-)
 
+# TODO(mohrr) Remove gn_check=False when it passes for all downstream projects.
+@filter_paths(endswith=_PYTHON_EXTENSIONS)
+def gn_python_check(ctx: PresubmitContext):
+    build.gn_gen(ctx.root, ctx.output_dir, gn_check=False)
+    build.ninja(ctx.output_dir, 'python.tests', 'python.lint')
 
+
+# TODO(mohrr) Remove gn_check=False when it passes for all downstream projects.
+@filter_paths(endswith=_PYTHON_EXTENSIONS)
+def gn_lint(ctx: pw_presubmit.PresubmitContext) -> None:
+    build.gn_gen(ctx.root, ctx.output_dir, gn_check=False)
+    build.ninja(ctx.output_dir, 'python.lint')
+
+
+# TODO(mohrr) Remove gn_check=False when it passes for all downstream projects.
+# TODO(pwbug/454) Remove after downstream projects switch to gn_python_check.
+@filter_paths(endswith=_PYTHON_EXTENSIONS)
+def test_python_packages(ctx: PresubmitContext):
+    build.gn_gen(ctx.root, ctx.output_dir, gn_check=False)
+    build.ninja(ctx.output_dir, 'python.tests')
+
+
+_LINT_CHECKS = (gn_lint, )
+_ALL_CHECKS = (gn_python_check, test_python_packages)
+
+
+# TODO(pwbug/454) Remove after downstream projects switch to using functions
+# directly.
 def lint_checks(endswith: str = '.py',
                 **filter_paths_args) -> Tuple[Callable, ...]:
     return tuple(
@@ -155,6 +72,8 @@ def lint_checks(endswith: str = '.py',
         for function in _LINT_CHECKS)
 
 
+# TODO(pwbug/454) Remove after downstream projects switch to using functions
+# directly.
 def all_checks(endswith: str = '.py',
                **filter_paths_args) -> Tuple[Callable, ...]:
     return tuple(