diff --git a/build-support/bin/BUILD b/build-support/bin/BUILD index 0f87d81dd3a..0aea1f20cd0 100644 --- a/build-support/bin/BUILD +++ b/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', diff --git a/build-support/bin/black.py b/build-support/bin/black.py new file mode 100755 index 00000000000..2067fb97907 --- /dev/null +++ b/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() diff --git a/build-support/bin/common.py b/build-support/bin/common.py index d3a41760d71..12adf97ce8b 100644 --- a/build-support/bin/common.py +++ b/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") diff --git a/src/python/pants/backend/python/rules/python_fmt.py b/src/python/pants/backend/python/rules/python_fmt.py index aae2fe6a724..ad37ffe0bbe 100644 --- a/src/python/pants/backend/python/rules/python_fmt.py +++ b/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), ] diff --git a/src/python/pants/rules/core/fmt.py b/src/python/pants/rules/core/fmt.py index 3da7277f90e..74c387c529c 100644 --- a/src/python/pants/rules/core/fmt.py +++ b/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: diff --git a/src/python/pants/rules/core/lint.py b/src/python/pants/rules/core/lint.py new file mode 100644 index 00000000000..f4e2a32058d --- /dev/null +++ b/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, + ] diff --git a/src/python/pants/rules/core/register.py b/src/python/pants/rules/core/register.py index 20d89fdd387..ccf6ecf827d 100644 --- a/src/python/pants/rules/core/register.py +++ b/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(), diff --git a/tests/python/pants_test/backend/python/rules/test_python_fmt_integration.py b/tests/python/pants_test/backend/python/rules/test_python_fmt_integration.py index ca9fef6292b..738abc27c44 100644 --- a/tests/python/pants_test/backend/python/rules/test_python_fmt_integration.py +++ b/tests/python/pants_test/backend/python/rules/test_python_fmt_integration.py @@ -52,6 +52,20 @@ def test_black_one_python_source_should_leave_one_file_unchanged(self): self.assertNotIn("reformatted", pants_run.stderr_data) self.assertIn("1 file left unchanged", pants_run.stderr_data) + def test_black_lint_given_formatted_file(self): + with build_directory() as root_dir: + code = write_consistently_formatted_file(root_dir, "hello.py") + command = [ + 'lint-v2', + f'{root_dir}' + ] + pants_run = self.run_pants(command=command) + self.assert_success(pants_run) + after_formatting = code.read_text() + self.assertEqual(CONSISTENTLY_FORMATTED_HELLO, after_formatting) + self.assertNotIn("reformatted", pants_run.stderr_data) + self.assertIn("1 file would be left unchanged", pants_run.stderr_data) + def test_black_two_python_sources_should_leave_two_files_unchanged(self): with build_directory() as root_dir: foo = write_consistently_formatted_file(root_dir, "foo.py") @@ -109,3 +123,51 @@ def test_black_should_format_python_code(self): self.assertEqual(CONSISTENTLY_FORMATTED_HELLO, after_formatting) self.assertIn("1 file reformatted", pants_run.stderr_data) self.assertNotIn("unchanged", pants_run.stderr_data) + + def test_black_lint_should_fail_but_not_format_python_code(self): + with build_directory() as root_dir: + code = write_inconsistently_formatted_file(root_dir, "hello.py") + command = [ + 'lint-v2', + f'{root_dir}' + ] + pants_run = self.run_pants(command=command) + self.assert_failure(pants_run) + after_formatting = code.read_text() + self.assertEqual(INCONSISTENTLY_FORMATTED_HELLO, after_formatting) + self.assertIn("1 file would be reformatted", pants_run.stderr_data) + self.assertNotIn("1 file reformatted", pants_run.stderr_data) + self.assertNotIn("unchanged", pants_run.stderr_data) + + def test_black_lint_given_multiple_files(self): + with build_directory() as root_dir: + write_inconsistently_formatted_file(root_dir, "incorrect.py") + write_inconsistently_formatted_file(root_dir, "broken.py") + write_consistently_formatted_file(root_dir, "pristine.py") + command = [ + 'lint-v2', + f'{root_dir}' + ] + pants_run = self.run_pants(command=command) + self.assert_failure(pants_run) + self.assertIn("2 files would be reformatted", pants_run.stderr_data) + self.assertNotIn("2 files reformatted", pants_run.stderr_data) + self.assertIn("1 file would be left unchanged", pants_run.stderr_data) + self.assertNotIn("1 file left unchanged", pants_run.stderr_data) + + def test_black_lint_given_multiple_targets(self): + with build_directory() as a_dir: + with build_directory() as another_dir: + write_inconsistently_formatted_file(a_dir, "incorrect.py") + write_inconsistently_formatted_file(another_dir, "broken.py") + write_inconsistently_formatted_file(another_dir, "messed_up.py") + command = [ + 'lint-v2', + f'{a_dir}', + f'{another_dir}' + ] + pants_run = self.run_pants(command=command) + self.assert_failure(pants_run) + self.assertIn("1 file would be reformatted", pants_run.stderr_data) + self.assertIn("2 files would be reformatted", pants_run.stderr_data) + self.assertNotIn("unchanged", pants_run.stderr_data)