From b0bd5c3c1da8e76320b61b78b9486c0fe6ea9ad7 Mon Sep 17 00:00:00 2001 From: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> Date: Sun, 16 Aug 2020 18:12:21 -0700 Subject: [PATCH] Rename `--per-target-caching` to `--per-file-caching` for `lint` and `fmt` (#10630) Thanks to https://github.com/pantsbuild/pants/pull/10511, targets are no longer an atomic unit Pants runs on. Instead, files are the atomic unit. This option rename reflects that model change. [ci skip-rust] [ci skip-build-wheels] --- src/python/pants/core/goals/fmt.py | 37 +++++++++++++++---- src/python/pants/core/goals/fmt_test.py | 32 ++++++++-------- src/python/pants/core/goals/lint.py | 47 +++++++++++++++++------- src/python/pants/core/goals/lint_test.py | 32 ++++++++-------- 4 files changed, 97 insertions(+), 51 deletions(-) diff --git a/src/python/pants/core/goals/fmt.py b/src/python/pants/core/goals/fmt.py index 14168b0f660..03083dc3781 100644 --- a/src/python/pants/core/goals/fmt.py +++ b/src/python/pants/core/goals/fmt.py @@ -6,6 +6,7 @@ from dataclasses import dataclass from typing import ClassVar, Iterable, List, Optional, Tuple, Type, cast +from pants.base.deprecated import resolve_conflicting_options from pants.core.util_rules.filter_empty_sources import TargetsWithSources, TargetsWithSourcesRequest from pants.engine.console import Console from pants.engine.engine_aware import EngineAware @@ -138,24 +139,44 @@ class FmtSubsystem(GoalSubsystem): def register_options(cls, register) -> None: super().register_options(register) register( - "--per-target-caching", + "--per-file-caching", advanced=True, type=bool, default=False, help=( - "Rather than running all targets in a single batch, run each target as a " + "Rather than formatting all files in a single batch, format each file as a " "separate process. Why do this? You'll get many more cache hits. Why not do this? " "Formatters both have substantial startup overhead and are cheap to add one " "additional file to the run. On a cold cache, it is much faster to use " - "`--no-per-target-caching`. We only recommend using `--per-target-caching` if you " + "`--no-per-file-caching`. We only recommend using `--per-file-caching` if you " "are using a remote cache or if you have benchmarked that this option will be " - "faster than `--no-per-target-caching` for your use case." + "faster than `--no-per-file-caching` for your use case." + ), + ) + register( + "--per-target-caching", + advanced=True, + type=bool, + default=False, + help="See `--per-file-caching`.", + removal_version="2.1.0.dev0", + removal_hint=( + "Use the renamed `--per-file-caching` option instead. If this option is set, Pants " + "will now run per every file, rather than per target." ), ) @property - def per_target_caching(self) -> bool: - return cast(bool, self.options.per_target_caching) + def per_file_caching(self) -> bool: + val = resolve_conflicting_options( + old_option="per_target_caching", + new_option="per_file_caching", + old_container=self.options, + new_container=self.options, + old_scope=self.name, + new_scope=self.name, + ) + return cast(bool, val) class Fmt(Goal): @@ -202,7 +223,7 @@ async def fmt( if language_targets_with_sources ) - if fmt_subsystem.per_target_caching: + if fmt_subsystem.per_file_caching: per_language_results = await MultiGet( Get( LanguageFmtResults, @@ -244,7 +265,7 @@ async def fmt( # We group all results for the same formatter so that we can give one final status in the # summary. This is only relevant if there were multiple results because of - # `--per-target-caching`. + # `--per-file-caching`. formatter_to_results = defaultdict(set) for result in individual_results: formatter_to_results[result.formatter_name].add(result) diff --git a/src/python/pants/core/goals/fmt_test.py b/src/python/pants/core/goals/fmt_test.py index 02d3372535d..f13f58e362f 100644 --- a/src/python/pants/core/goals/fmt_test.py +++ b/src/python/pants/core/goals/fmt_test.py @@ -143,7 +143,7 @@ def run_fmt_rule( language_target_collection_types: List[Type[LanguageFmtTargets]], targets: List[Target], result_digest: Digest, - per_target_caching: bool, + per_file_caching: bool, include_sources: bool = True, ) -> str: console = MockConsole(use_colors=False) @@ -153,7 +153,9 @@ def run_fmt_rule( rule_args=[ console, Targets(targets), - create_goal_subsystem(FmtSubsystem, per_target_caching=per_target_caching), + create_goal_subsystem( + FmtSubsystem, per_file_caching=per_file_caching, per_target_caching=False + ), Workspace(self.scheduler), union_membership, ], @@ -193,40 +195,40 @@ def assert_workspace_modified( assert smalltalk_file.read_text() == self.smalltalk_file.content.decode() def test_empty_target_noops(self) -> None: - def assert_noops(*, per_target_caching: bool) -> None: + def assert_noops(*, per_file_caching: bool) -> None: stderr = self.run_fmt_rule( language_target_collection_types=[FortranTargets], targets=[self.make_target()], result_digest=self.fortran_digest, - per_target_caching=per_target_caching, + per_file_caching=per_file_caching, include_sources=False, ) assert stderr.strip() == "" self.assert_workspace_modified(fortran_formatted=False, smalltalk_formatted=False) - assert_noops(per_target_caching=False) - assert_noops(per_target_caching=True) + assert_noops(per_file_caching=False) + assert_noops(per_file_caching=True) def test_invalid_target_noops(self) -> None: - def assert_noops(*, per_target_caching: bool) -> None: + def assert_noops(*, per_file_caching: bool) -> None: stderr = self.run_fmt_rule( language_target_collection_types=[InvalidTargets], targets=[self.make_target()], result_digest=self.fortran_digest, - per_target_caching=per_target_caching, + per_file_caching=per_file_caching, ) assert stderr.strip() == "" self.assert_workspace_modified(fortran_formatted=False, smalltalk_formatted=False) - assert_noops(per_target_caching=False) - assert_noops(per_target_caching=True) + assert_noops(per_file_caching=False) + assert_noops(per_file_caching=True) def test_summary(self) -> None: """Tests that the final summary is correct. This checks that we: * Merge multiple results for the same formatter together (when you use - `--per-target-caching`). + `--per-file-caching`). * Correctly distinguish between skipped, changed, and did not change. """ fortran_addresses = [Address.parse(":f1"), Address.parse(":needs_formatting")] @@ -239,12 +241,12 @@ def test_summary(self) -> None: self.make_target(addr, target_cls=SmalltalkTarget) for addr in smalltalk_addresses ] - def assert_expected(*, per_target_caching: bool) -> None: + def assert_expected(*, per_file_caching: bool) -> None: stderr = self.run_fmt_rule( language_target_collection_types=[FortranTargets, SmalltalkTargets], targets=[*fortran_targets, *smalltalk_targets], result_digest=self.merged_digest, - per_target_caching=per_target_caching, + per_file_caching=per_file_caching, ) self.assert_workspace_modified(fortran_formatted=True, smalltalk_formatted=True) assert stderr == dedent( @@ -256,8 +258,8 @@ def assert_expected(*, per_target_caching: bool) -> None: """ ) - assert_expected(per_target_caching=False) - assert_expected(per_target_caching=True) + assert_expected(per_file_caching=False) + assert_expected(per_file_caching=True) def test_streaming_output_skip() -> None: diff --git a/src/python/pants/core/goals/lint.py b/src/python/pants/core/goals/lint.py index 98a4c81329c..feb3c9bc1ff 100644 --- a/src/python/pants/core/goals/lint.py +++ b/src/python/pants/core/goals/lint.py @@ -6,6 +6,7 @@ from dataclasses import dataclass from typing import Iterable, Optional, Tuple, cast +from pants.base.deprecated import resolve_conflicting_options from pants.core.goals.style_request import StyleRequest from pants.core.util_rules.filter_empty_sources import ( FieldSetsWithSources, @@ -149,18 +150,30 @@ class LintSubsystem(GoalSubsystem): def register_options(cls, register) -> None: super().register_options(register) register( - "--per-target-caching", + "--per-file-caching", advanced=True, type=bool, default=False, help=( - "Rather than running all targets in a single batch, run each target as a " + "Rather than linting all files in a single batch, lint each file as a " "separate process. Why do this? You'll get many more cache hits. Why not do this? " "Linters both have substantial startup overhead and are cheap to add one " "additional file to the run. On a cold cache, it is much faster to use " - "`--no-per-target-caching`. We only recommend using `--per-target-caching` if you " + "`--no-per-file-caching`. We only recommend using `--per-file-caching` if you " "are using a remote cache or if you have benchmarked that this option will be " - "faster than `--no-per-target-caching` for your use case." + "faster than `--no-per-file-caching` for your use case." + ), + ) + register( + "--per-target-caching", + advanced=True, + type=bool, + default=False, + help="See `--per-file-caching`.", + removal_version="2.1.0.dev0", + removal_hint=( + "Use the renamed `--per-file-caching` option instead. If this option is set, Pants " + "will now run per every file, rather than per target." ), ) register( @@ -171,13 +184,21 @@ def register_options(cls, register) -> None: advanced=True, help=( "Specifying a directory causes linters that support writing report files to write " - "into this directory.", + "into this directory." ), ) @property - def per_target_caching(self) -> bool: - return cast(bool, self.options.per_target_caching) + def per_file_caching(self) -> bool: + val = resolve_conflicting_options( + old_option="per_target_caching", + new_option="per_file_caching", + old_container=self.options, + new_container=self.options, + old_scope=self.name, + new_scope=self.name, + ) + return cast(bool, val) @property def reports_dir(self) -> Optional[str]: @@ -215,8 +236,8 @@ async def lint( if request ) - if lint_subsystem.per_target_caching: - all_per_target_results = await MultiGet( + if lint_subsystem.per_file_caching: + all_per_file_results = await MultiGet( Get(LintResults, LintRequest, request.__class__([field_set])) for request in valid_requests for field_set in request.field_sets @@ -225,12 +246,12 @@ async def lint( all_results = tuple( LintResults( itertools.chain.from_iterable( - per_target_results.results for per_target_results in all_linter_results + per_file_results.results for per_file_results in all_linter_results ), linter_name=linter_name, ) for linter_name, all_linter_results in itertools.groupby( - all_per_target_results, key=lambda results: results.linter_name + all_per_file_results, key=lambda results: results.linter_name ) ) else: @@ -247,8 +268,8 @@ async def lint( results.linter_name for results in all_results if len(results.reports) > 1 ] if linters_with_multiple_reports: - if lint_subsystem.per_target_caching: - suggestion = "Try running without `--lint-per-target-caching` set." + if lint_subsystem.per_file_caching: + suggestion = "Try running without `--lint-per-file-caching` set." else: suggestion = ( "The linters likely partitioned the input targets, such as grouping by Python " diff --git a/src/python/pants/core/goals/lint_test.py b/src/python/pants/core/goals/lint_test.py index f9f162ac4f1..ecf770d42aa 100644 --- a/src/python/pants/core/goals/lint_test.py +++ b/src/python/pants/core/goals/lint_test.py @@ -109,7 +109,7 @@ def run_lint_rule( *, lint_request_types: List[Type[LintRequest]], targets: List[Target], - per_target_caching: bool, + per_file_caching: bool, include_sources: bool = True, ) -> Tuple[int, str]: console = MockConsole(use_colors=False) @@ -121,7 +121,9 @@ def run_lint_rule( console, workspace, Targets(targets), - create_goal_subsystem(LintSubsystem, per_target_caching=per_target_caching), + create_goal_subsystem( + LintSubsystem, per_file_caching=per_file_caching, per_target_caching=False + ), union_membership, ], mock_gets=[ @@ -147,43 +149,43 @@ def run_lint_rule( return result.exit_code, console.stderr.getvalue() def test_empty_target_noops(self) -> None: - def assert_noops(per_target_caching: bool) -> None: + def assert_noops(per_file_caching: bool) -> None: exit_code, stderr = self.run_lint_rule( lint_request_types=[FailingRequest], targets=[self.make_target()], - per_target_caching=per_target_caching, + per_file_caching=per_file_caching, include_sources=False, ) assert exit_code == 0 assert stderr == "" - assert_noops(per_target_caching=False) - assert_noops(per_target_caching=True) + assert_noops(per_file_caching=False) + assert_noops(per_file_caching=True) def test_invalid_target_noops(self) -> None: - def assert_noops(per_target_caching: bool) -> None: + def assert_noops(per_file_caching: bool) -> None: exit_code, stderr = self.run_lint_rule( lint_request_types=[InvalidRequest], targets=[self.make_target()], - per_target_caching=per_target_caching, + per_file_caching=per_file_caching, ) assert exit_code == 0 assert stderr == "" - assert_noops(per_target_caching=False) - assert_noops(per_target_caching=True) + assert_noops(per_file_caching=False) + assert_noops(per_file_caching=True) def test_summary(self) -> None: """Test that we render the summary correctly. This tests that we: - * Merge multiple results belonging to the same linter (`--per-target-caching`). + * Merge multiple results belonging to the same linter (`--per-file-caching`). * Decide correctly between skipped, failed, and succeeded. """ good_address = Address.parse(":good") bad_address = Address.parse(":bad") - def assert_expected(*, per_target_caching: bool) -> None: + def assert_expected(*, per_file_caching: bool) -> None: exit_code, stderr = self.run_lint_rule( lint_request_types=[ ConditionallySucceedsRequest, @@ -192,7 +194,7 @@ def assert_expected(*, per_target_caching: bool) -> None: SuccessfulRequest, ], targets=[self.make_target(good_address), self.make_target(bad_address)], - per_target_caching=per_target_caching, + per_file_caching=per_file_caching, ) assert exit_code == FailingRequest.exit_code([bad_address]) assert stderr == dedent( @@ -205,8 +207,8 @@ def assert_expected(*, per_target_caching: bool) -> None: """ ) - assert_expected(per_target_caching=False) - assert_expected(per_target_caching=True) + assert_expected(per_file_caching=False) + assert_expected(per_file_caching=True) def test_streaming_output_skip() -> None: