Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flake8 output file support. #10371

Merged
merged 10 commits into from Jul 21, 2020
34 changes: 28 additions & 6 deletions src/python/pants/backend/python/lint/flake8/rules.py
Expand Up @@ -15,14 +15,14 @@
from pants.backend.python.subsystems import python_native_code, subprocess_environment
from pants.backend.python.subsystems.subprocess_environment import SubprocessEncodingEnvironment
from pants.backend.python.target_types import PythonInterpreterCompatibility, PythonSources
from pants.core.goals.lint import LintRequest, LintResult, LintResults
from pants.core.goals.lint import LintRequest, LintResult, LintResultFile, LintResults
from pants.core.util_rules import determine_source_files, strip_source_roots
from pants.core.util_rules.determine_source_files import (
AllSourceFilesRequest,
SourceFiles,
SpecifiedSourceFilesRequest,
)
from pants.engine.fs import Digest, MergeDigests, PathGlobs, Snapshot
from pants.engine.fs import Digest, MergeDigests, PathGlobs, Snapshot, SnapshotSubset
from pants.engine.process import FallibleProcessResult, Process
from pants.engine.rules import SubsystemRule, rule
from pants.engine.selectors import Get, MultiGet
Expand Down Expand Up @@ -51,10 +51,14 @@ class Flake8Partition:
interpreter_constraints: PexInterpreterConstraints


def generate_args(*, specified_source_files: SourceFiles, flake8: Flake8) -> Tuple[str, ...]:
def generate_args(
*, specified_source_files: SourceFiles, flake8: Flake8, output_file: Optional[str]
) -> Tuple[str, ...]:
args = []
if flake8.options.config is not None:
args.append(f"--config={flake8.options.config}")
if output_file:
args.append(f"--output-file={output_file}")
args.extend(flake8.options.args)
args.extend(specified_source_files.files)
return tuple(args)
Expand Down Expand Up @@ -117,19 +121,37 @@ async def flake8_lint_partition(
address_references = ", ".join(
sorted(field_set.address.reference() for field_set in partition.field_sets)
)

output_path = flake8.output_file_path
flake8_args = generate_args(
specified_source_files=specified_source_files,
flake8=flake8,
output_file=output_path.name if output_path else None,
)
process = requirements_pex.create_process(
python_setup=python_setup,
subprocess_encoding_environment=subprocess_encoding_environment,
pex_path="./flake8.pex",
pex_args=generate_args(specified_source_files=specified_source_files, flake8=flake8),
pex_args=flake8_args,
output_files=(output_path.name,) if output_path else None,
input_digest=input_digest,
description=(
f"Run Flake8 on {pluralize(len(partition.field_sets), 'target')}: {address_references}."
),
)
result = await Get(FallibleProcessResult, Process, process)
return LintResult.from_fallible_process_result(result, linter_name="Flake8")
results_file = None

if output_path:
report_file_snapshot = await Get(
Snapshot, SnapshotSubset(result.output_digest, PathGlobs([output_path.name]))
)
if report_file_snapshot.is_empty or len(report_file_snapshot.files) != 1:
raise Exception(f"Unexpected report file snapshot: {report_file_snapshot}")
results_file = LintResultFile(output_path=output_path, digest=report_file_snapshot.digest)

return LintResult.from_fallible_process_result(
result, linter_name="Flake8", results_file=results_file
)


@rule(desc="Lint using Flake8")
Expand Down
Expand Up @@ -9,7 +9,7 @@
from pants.base.specs import FilesystemLiteralSpec, OriginSpec, SingleAddress
from pants.core.goals.lint import LintResults
from pants.engine.addresses import Address
from pants.engine.fs import FileContent
from pants.engine.fs import DigestContents, FileContent
from pants.engine.rules import RootRule
from pants.engine.selectors import Params
from pants.engine.target import TargetWithOrigin
Expand Down Expand Up @@ -78,13 +78,15 @@ def test_passing_source(self) -> None:
assert len(result) == 1
assert result[0].exit_code == 0
assert result[0].stdout.strip() == ""
assert result[0].results_file is None

def test_failing_source(self) -> None:
target = self.make_target_with_origin([self.bad_source])
result = self.run_flake8([target])
assert len(result) == 1
assert result[0].exit_code == 1
assert "bad.py:1:1: F401" in result[0].stdout
assert result[0].results_file is None

def test_mixed_sources(self) -> None:
target = self.make_target_with_origin([self.good_source, self.bad_source])
Expand Down Expand Up @@ -173,3 +175,14 @@ def test_3rdparty_plugin(self) -> None:
assert len(result) == 1
assert result[0].exit_code == 1
assert "bad.py:1:1: PB11" in result[0].stdout

def test_output_file(self) -> None:
target = self.make_target_with_origin([self.bad_source])
result = self.run_flake8([target], additional_args=["--flake8-output-file=report.txt"])
assert len(result) == 1
assert result[0].exit_code == 1
assert result[0].stdout.strip() == ""
assert result[0].results_file is not None
output_files = self.request_single_product(DigestContents, result[0].results_file.digest)
assert len(output_files) == 1
assert "bad.py:1:1: F401" in output_files[0].content.decode()
16 changes: 16 additions & 0 deletions src/python/pants/backend/python/lint/flake8/subsystem.py
@@ -1,6 +1,9 @@
# Copyright 2019 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from pathlib import Path
from typing import Optional

from pants.backend.python.subsystems.python_tool_base import PythonToolBase
from pants.option.custom_types import file_option, shell_str

Expand Down Expand Up @@ -34,3 +37,16 @@ def register_options(cls, register):
advanced=True,
help="Path to `.flake8` or alternative Flake8 config file",
)
register(
"--output-file",
type=str,
metavar="filename",
default=None,
advanced=True,
help="Redirect report to a file.",
)
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than an output file, could this potentially be an output directory, located on the Lint Goal? That way each linter could emit its output in subdirectories of a shared output directory... and we could probably give it a reasonable default so that reports are always generated.

Since the LintOutputFile type is exposed to the Goal, it would be good to enable more linters to be able to use it (without specifying an output file per linter).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, updated.
But keep in mind that now we hard code the report file name for each linter in the rule.
Which is not optimal, since each linter has other options to control the format of the file (using pass-thru args or by adding plugins to the requirements).
so the issue there is that the file name will not reflect the format of the file.
We can obviously expose the file name as an option on each linter, but then, a user will have to configure things in two places, which is confusing.

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True... but it feels like whether or not a linter has multiple file formats is a per linter concern... the linter can choose which filename it wants to within the directory. Fine either way.


@property
def output_file_path(self) -> Optional[Path]:
output_file = self.options.output_file
return Path(output_file) if output_file else None
31 changes: 29 additions & 2 deletions src/python/pants/core/goals/lint.py
Expand Up @@ -3,7 +3,8 @@

import itertools
from dataclasses import dataclass
from typing import Iterable
from pathlib import Path
from typing import Iterable, Optional

from pants.core.goals.style_request import StyleRequest
from pants.core.util_rules.filter_empty_sources import (
Expand All @@ -12,6 +13,7 @@
)
from pants.engine.collection import Collection
from pants.engine.console import Console
from pants.engine.fs import Digest, DirectoryToMaterialize, Workspace
from pants.engine.goal import Goal, GoalSubsystem
from pants.engine.process import FallibleProcessResult
from pants.engine.rules import goal_rule
Expand All @@ -21,16 +23,27 @@
from pants.util.strutil import strip_v2_chroot_path


@dataclass(frozen=True)
class LintResultFile:
output_path: Path
digest: Digest


@dataclass(frozen=True)
class LintResult:
exit_code: int
stdout: str
stderr: str
linter_name: str
results_file: Optional[LintResultFile]

@staticmethod
def from_fallible_process_result(
process_result: FallibleProcessResult, *, linter_name: str, strip_chroot_path: bool = False
process_result: FallibleProcessResult,
*,
linter_name: str,
strip_chroot_path: bool = False,
results_file: Optional[LintResultFile] = None,
) -> "LintResult":
def prep_output(s: bytes) -> str:
return strip_v2_chroot_path(s) if strip_chroot_path else s.decode()
Expand All @@ -40,7 +53,19 @@ def prep_output(s: bytes) -> str:
stdout=prep_output(process_result.stdout),
stderr=prep_output(process_result.stderr),
linter_name=linter_name,
results_file=results_file,
)

def materialize(self, console: Console, workspace: Workspace) -> None:
if not self.results_file:
return
output_path = self.results_file.output_path
workspace.materialize_directory(
DirectoryToMaterialize(
self.results_file.digest, path_prefix=output_path.parent.as_posix(),
)
)
console.print_stdout(f"Wrote {self.linter_name} report to: {output_path.as_posix()}")


class LintResults(Collection[LintResult]):
Expand Down Expand Up @@ -94,6 +119,7 @@ class Lint(Goal):
@goal_rule
async def lint(
console: Console,
workspace: Workspace,
targets_with_origins: TargetsWithOrigins,
options: LintOptions,
union_membership: UnionMembership,
Expand Down Expand Up @@ -145,6 +171,7 @@ async def lint(
console.print_stderr(result.stderr)
if result != sorted_results[-1]:
console.print_stderr("")
result.materialize(console, workspace)
if result.exit_code != 0:
exit_code = result.exit_code

Expand Down
6 changes: 5 additions & 1 deletion src/python/pants/core/goals/lint_test.py
Expand Up @@ -12,6 +12,7 @@
FieldSetsWithSourcesRequest,
)
from pants.engine.addresses import Address
from pants.engine.fs import Workspace
from pants.engine.target import (
FieldSetWithOrigin,
Sources,
Expand Down Expand Up @@ -57,6 +58,7 @@ def lint_results(self) -> LintResults:
self.stdout(addresses),
"",
linter_name=self.linter_name,
results_file=None,
)
]
)
Expand Down Expand Up @@ -131,20 +133,22 @@ def make_target_with_origin(address: Optional[Address] = None) -> TargetWithOrig
origin=SingleAddress(directory=address.spec_path, name=address.target_name),
)

@staticmethod
def run_lint_rule(
self,
*,
lint_request_types: List[Type[LintRequest]],
targets: List[TargetWithOrigin],
per_target_caching: bool,
include_sources: bool = True,
) -> Tuple[int, str]:
console = MockConsole(use_colors=False)
workspace = Workspace(self.scheduler)
union_membership = UnionMembership({LintRequest: lint_request_types})
result: Lint = run_rule(
lint,
rule_args=[
console,
workspace,
TargetsWithOrigins(targets),
create_goal_subsystem(LintOptions, per_target_caching=per_target_caching),
union_membership,
Expand Down