Add lint-v2 implementation for python with black, and script to be used in pre-commit hook (#8553)

* Add lint-v2 implementation for python with black, and script to be used in pre-commit hook

Author: pierrechevalier83

build-support/bin/BUILD

@@ -14,6 +14,12 @@ files(
   sources = globs('*.py'),
 )
 
+python_binary(
+  name = 'black',
+  source = 'black.py',
+  tags = {'type_checked'},
+)
+
 python_binary(
   name = 'check_banned_imports',
   source = 'check_banned_imports.py',

build-support/bin/black.py

@@ -0,0 +1,31 @@
+#!/usr/bin/env python3
+# Copyright 2019 Pants project contributors (see CONTRIBUTORS.md).
+# Licensed under the Apache License, Version 2.0 (see LICENSE).
+
+import argparse
+import subprocess
+import sys
+
+from common import git_merge_base
+
+
+def create_parser() -> argparse.ArgumentParser:
+  parser = argparse.ArgumentParser(
+    description="Formats all python files since master with black through pants.",
+  )
+  parser.add_argument("-f", "--fix", action="store_true",
+    help="Instead of erroring on files with incorrect formatting, fix those files."
+  )
+  return parser
+
+
+def main() -> None:
+  args= create_parser().parse_args()
+  merge_base = git_merge_base()
+  goal = "fmt-v2" if args.fix else "lint-v2"
+  command = ["./pants", f"--changed-parent={merge_base}", goal]
+  process = subprocess.run(command)
+  sys.exit(process.returncode)
+
+if __name__ == "__main__":
+  main()

build-support/bin/common.py

@@ -19,6 +19,7 @@
 Callers of this file, however, are free to dogfood Pants as they'd like, and any script
 may be called via `./pants run` instead of direct invocation if desired."""
 
+import subprocess
 import time
 from contextlib import contextmanager
 from pathlib import Path
@@ -53,6 +54,17 @@ def elapsed_time() -> Tuple[int, int]:
   return elapsed_seconds // 60, elapsed_seconds % 60
 
 
+def git_merge_base() -> str:
+  get_tracking_branch = ["git",
+                         "rev-parse",
+                         "--symbolic-full-name",
+                         "--abbrev-ref",
+                         "HEAD@{upstream}"
+                        ]
+  process = subprocess.run(get_tracking_branch, stdout=subprocess.PIPE, stderr=subprocess.PIPE, encoding="utf-8")
+  return str(process.stdout.rstrip()) if process.stdout else "master"
+
+
 @contextmanager
 def travis_section(slug: str, message: str) -> Iterator[None]:
   travis_fold_state_path = Path("/tmp/.travis_fold_current")

src/python/pants/backend/python/rules/python_fmt.py

@@ -4,14 +4,18 @@
 import re
 from dataclasses import dataclass
 from pathlib import Path
-from typing import Any, Set
+from typing import Any, Set, Tuple
 
 from pants.backend.python.rules.pex import CreatePex, Pex, PexInterpreterContraints, PexRequirements
 from pants.backend.python.subsystems.black import Black
 from pants.backend.python.subsystems.python_setup import PythonSetup
 from pants.backend.python.subsystems.subprocess_environment import SubprocessEncodingEnvironment
 from pants.engine.fs import Digest, DirectoriesToMerge, PathGlobs, Snapshot
-from pants.engine.isolated_process import ExecuteProcessRequest, ExecuteProcessResult
+from pants.engine.isolated_process import (
+  ExecuteProcessRequest,
+  ExecuteProcessResult,
+  FallibleExecuteProcessResult,
+)
 from pants.engine.legacy.structs import (
   PantsPluginAdaptor,
   PythonAppAdaptor,
@@ -21,7 +25,8 @@
 )
 from pants.engine.rules import UnionRule, optionable_rule, rule
 from pants.engine.selectors import Get
-from pants.rules.core.fmt import FmtResult, FmtTarget
+from pants.rules.core.fmt import FmtResult, TargetWithSources
+from pants.rules.core.lint import LintResult
 
 
 # Note: this is a workaround until https://github.com/pantsbuild/pants/issues/8343 is addressed
@@ -32,13 +37,18 @@ class FormattablePythonTarget:
   target: Any
 
 
+@dataclass(frozen=True)
+class BlackInput:
+  config_path: Path
+  resolved_requirements_pex: Pex
+  merged_input_files: Digest
+
+
 @rule
-def run_black(
+def get_black_input(
   wrapped_target: FormattablePythonTarget,
   black: Black,
-  python_setup: PythonSetup,
-  subprocess_encoding_environment: SubprocessEncodingEnvironment,
-  ) -> FmtResult:
+  ) -> BlackInput:
   config_path = black.get_options().config
   config_snapshot = yield Get(Snapshot, PathGlobs(include=(config_path,)))
 
@@ -62,28 +72,58 @@ def run_black(
     Digest,
     DirectoriesToMerge(directories=tuple(all_input_digests)),
   )
+  yield BlackInput(config_path, resolved_requirements_pex, merged_input_files)
 
+
+def _generate_black_pex_args(files: Set[str], config_path: str, *, check_only: bool) -> Tuple[str, ...]:
   # The exclude option from Black only works on recursive invocations,
   # so call black with the directories in which the files are present
   # and passing the full file names with the include option
   dirs: Set[str] = set()
-  for filename in target.sources.snapshot.files:
+  for filename in files:
     dirs.add(f"{Path(filename).parent}")
   pex_args= tuple(sorted(dirs))
+  if check_only:
+    pex_args += ("--check", )
   if config_path:
     pex_args += ("--config", config_path)
-  if target.sources.snapshot.files:
-    pex_args += ("--include", "|".join(re.escape(f) for f in target.sources.snapshot.files))
+  if files:
+    pex_args += ("--include", "|".join(re.escape(f) for f in files))
+  return pex_args
+
 
-  request = resolved_requirements_pex.create_execute_request(
+def _generate_black_request(
+  wrapped_target: FormattablePythonTarget,
+  black_input: BlackInput,
+  python_setup: PythonSetup,
+  subprocess_encoding_environment: SubprocessEncodingEnvironment,
+  *,
+  check_only: bool,
+  ):
+  target = wrapped_target.target
+  pex_args = _generate_black_pex_args(target.sources.snapshot.files, black_input.config_path, check_only = check_only)
+
+  request = black_input.resolved_requirements_pex.create_execute_request(
     python_setup=python_setup,
     subprocess_encoding_environment=subprocess_encoding_environment,
     pex_path="./black.pex",
     pex_args=pex_args,
-    input_files=merged_input_files,
+    input_files=black_input.merged_input_files,
     output_files=target.sources.snapshot.files,
     description=f'Run Black for {target.address.reference()}',
   )
+  return request
+
+
+@rule
+def fmt_with_black(
+  wrapped_target: FormattablePythonTarget,
+  black_input: BlackInput,
+  python_setup: PythonSetup,
+  subprocess_encoding_environment: SubprocessEncodingEnvironment,
+  ) -> FmtResult:
+
+  request = _generate_black_request(wrapped_target, black_input, python_setup, subprocess_encoding_environment, check_only = False)
 
   result = yield Get(ExecuteProcessResult, ExecuteProcessRequest, request)
 
@@ -94,6 +134,25 @@ def run_black(
   )
 
 
+@rule
+def lint_with_black(
+  wrapped_target: FormattablePythonTarget,
+  black_input: BlackInput,
+  python_setup: PythonSetup,
+  subprocess_encoding_environment: SubprocessEncodingEnvironment,
+  ) -> LintResult:
+
+  request = _generate_black_request(wrapped_target, black_input, python_setup, subprocess_encoding_environment, check_only = True)
+
+  result = yield Get(FallibleExecuteProcessResult, ExecuteProcessRequest, request)
+
+  yield LintResult(
+    exit_code=result.exit_code,
+    stdout=result.stdout.decode(),
+    stderr=result.stderr.decode(),
+  )
+
+
 # TODO: remove this workaround once https://github.com/pantsbuild/pants/issues/8343 is addressed
 @rule
 def target_adaptor(target: PythonTargetAdaptor) -> FormattablePythonTarget:
@@ -131,12 +190,14 @@ def rules():
     binary_adaptor,
     tests_adaptor,
     plugin_adaptor,
-    run_black,
-    UnionRule(FmtTarget, PythonTargetAdaptor),
-    UnionRule(FmtTarget, PythonAppAdaptor),
-    UnionRule(FmtTarget, PythonBinaryAdaptor),
-    UnionRule(FmtTarget, PythonTestsAdaptor),
-    UnionRule(FmtTarget, PantsPluginAdaptor),
+    get_black_input,
+    fmt_with_black,
+    lint_with_black,
+    UnionRule(TargetWithSources, PythonTargetAdaptor),
+    UnionRule(TargetWithSources, PythonAppAdaptor),
+    UnionRule(TargetWithSources, PythonBinaryAdaptor),
+    UnionRule(TargetWithSources, PythonTestsAdaptor),
+    UnionRule(TargetWithSources, PantsPluginAdaptor),
     optionable_rule(Black),
     optionable_rule(PythonSetup),
   ]

src/python/pants/rules/core/fmt.py

@@ -9,13 +9,7 @@
 from pants.engine.fs import Digest, FilesContent
 from pants.engine.goal import Goal
 from pants.engine.legacy.graph import HydratedTargets
-from pants.engine.legacy.structs import (
-  PythonAppAdaptor,
-  PythonBinaryAdaptor,
-  PythonTargetAdaptor,
-  PythonTestsAdaptor,
-)
-from pants.engine.rules import console_rule, union
+from pants.engine.rules import UnionMembership, console_rule, union
 from pants.engine.selectors import Get
 
 
@@ -27,7 +21,7 @@ class FmtResult:
 
 
 @union
-class FmtTarget:
+class TargetWithSources:
   """A union for registration of a formattable target type."""
 
 
@@ -40,17 +34,13 @@ class Fmt(Goal):
 
 
 @console_rule
-def fmt(console: Console, targets: HydratedTargets) -> Fmt:
+def fmt(console: Console, targets: HydratedTargets, union_membership: UnionMembership) -> Fmt:
   results = yield [
-          Get(FmtResult, FmtTarget, target.adaptor)
+          Get(FmtResult, TargetWithSources, target.adaptor)
           for target in targets
-          # @union assumes that all targets passed implement the union, so we manually
-          # filter the targets we know do; this should probably no-op or log or something
-          # configurable for non-matching targets.
-          # We also would want to remove the workaround that filters adaptors which have a
-          # `sources` attribute.
-          # See https://github.com/pantsbuild/pants/issues/4535
-          if isinstance(target.adaptor, (PythonAppAdaptor, PythonTargetAdaptor, PythonTestsAdaptor, PythonBinaryAdaptor)) and hasattr(target.adaptor, "sources")
+          # TODO: make TargetAdaptor return a 'sources' field with an empty snapshot instead of
+          # raising to remove the hasattr() checks here!
+          if union_membership.is_member(TargetWithSources, target.adaptor) and hasattr(target.adaptor, "sources")
           ]
 
   for result in results:

src/python/pants/rules/core/lint.py

@@ -0,0 +1,54 @@
+# Copyright 2019 Pants project contributors (see CONTRIBUTORS.md).
+# Licensed under the Apache License, Version 2.0 (see LICENSE).
+
+from dataclasses import dataclass
+
+from pants.engine.console import Console
+from pants.engine.goal import Goal
+from pants.engine.legacy.graph import HydratedTargets
+from pants.engine.rules import UnionMembership, console_rule
+from pants.engine.selectors import Get
+from pants.rules.core.fmt import TargetWithSources
+
+
+@dataclass(frozen=True)
+class LintResult:
+  exit_code: int
+  stdout: str
+  stderr: str
+
+
+class Lint(Goal):
+  """Lint source code."""
+
+  # TODO: make this "lint"
+  # Blocked on https://github.com/pantsbuild/pants/issues/8351
+  name = 'lint-v2'
+
+
+@console_rule
+def lint(console: Console, targets: HydratedTargets, union_membership: UnionMembership) -> Lint:
+  results = yield [
+          Get(LintResult, TargetWithSources, target.adaptor)
+          for target in targets
+          # TODO: make TargetAdaptor return a 'sources' field with an empty snapshot instead of
+          # raising to remove the hasattr() checks here!
+          if union_membership.is_member(TargetWithSources, target.adaptor) and hasattr(target.adaptor, "sources")
+          ]
+
+  exit_code = 0
+  for result in results:
+    if result.stdout:
+      console.print_stdout(result.stdout)
+    if result.stderr:
+      console.print_stderr(result.stderr)
+    if result.exit_code != 0:
+      exit_code = result.exit_code
+
+  yield Lint(exit_code)
+
+
+def rules():
+  return [
+      lint,
+    ]

src/python/pants/rules/core/register.py

@@ -5,6 +5,7 @@
   binary,
   filedeps,
   fmt,
+  lint,
   list_roots,
   list_targets,
   strip_source_root,
@@ -16,6 +17,7 @@ def rules():
   return [
     *binary.rules(),
     *fmt.rules(),
+    *lint.rules(),
     *list_roots.rules(),
     *list_targets.rules(),
     *filedeps.rules(),