Skip to content

Commit

Permalink
Make check output more useful for Go and Java (#13379)
Browse files Browse the repository at this point in the history
Before:

```
❯ ./pants check testprojects/src/go::
14:51:39.15 [INFO] Completed: pants.backend.go.goals.check.check_go - go succeeded.
Partition #1 - github.com/pantsbuild/pants/testprojects/src/go/pants_test:

Partition #2 - github.com/pantsbuild/pants/testprojects/src/go/pants_test/bar:



✓ go succeeded.
```
```
❯ ./pants check testprojects/src/go::
14:52:54.54 [ERROR] Completed: pants.backend.go.goals.check.check_go - go failed (exit code 1).
Partition #1 - github.com/pantsbuild/pants/testprojects/src/go/pants_test:
./testprojects/src/go/pants_test/foo.go:3:1: syntax error: non-declaration statement outside function body

Partition #2 - github.com/pantsbuild/pants/testprojects/src/go/pants_test/bar:



𐄂 go failed.
```

After:

```
❯ ./pants check testprojects/src/go::
14:20:40.79 [INFO] Completed: Check Go compilation - go succeeded.

✓ go compile succeeded.
```

```
❯ ./pants check testprojects/src/go::
14:23:16.18 [ERROR] Completed: Compile with Go - github.com/pantsbuild/pants/testprojects/src/go/pants_test failed (exit code 1).
./testprojects/src/go/pants_test/foo.go:3:1: syntax error: non-declaration statement outside function body

14:23:16.18 [ERROR] Completed: Check Go compilation - go failed (exit code 1).

𐄂 go compile failed.
```

That is, we now:

- Stream results for each package, rather than waiting to batch at the very end.
- Don't log the `Partition #1` part, which is verbose and confusing.
- Log success at DEBUG level, rather than INFO level, for less noise. (Reminder: these workunits will show up in the dynamic UI still)

[ci skip-rust]
  • Loading branch information
Eric-Arellano authored Oct 28, 2021
1 parent c879c40 commit f9e5cf1
Show file tree
Hide file tree
Showing 6 changed files with 64 additions and 58 deletions.
36 changes: 11 additions & 25 deletions src/python/pants/backend/go/goals/check.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from dataclasses import dataclass
from typing import cast

from pants.backend.go.target_types import GoFirstPartyPackageSourcesField
from pants.backend.go.util_rules.build_pkg import (
Expand All @@ -15,6 +14,7 @@
from pants.engine.rules import Get, MultiGet, collect_rules, rule
from pants.engine.target import FieldSet
from pants.engine.unions import UnionRule
from pants.util.logging import LogLevel


@dataclass(frozen=True)
Expand All @@ -28,7 +28,7 @@ class GoCheckRequest(CheckRequest):
field_set_type = GoCheckFieldSet


@rule
@rule(desc="Check Go compilation", level=LogLevel.DEBUG)
async def check_go(request: GoCheckRequest) -> CheckResults:
build_requests = await MultiGet(
Get(FallibleBuildGoPackageRequest, BuildGoPackageTargetRequest(field_set.address))
Expand All @@ -46,30 +46,16 @@ async def check_go(request: GoCheckRequest) -> CheckResults:
Get(FallibleBuiltGoPackage, BuildGoPackageRequest, request) for request in valid_requests
)

# TODO: Update `build_pkg.py` to use streaming workunits to log compilation results, which has
# the benefit of other contexts like `test.py` using it. Switch this to only preserve the
# exit code.
check_results = [
*(
CheckResult(
result.exit_code,
"",
cast(str, result.stderr),
partition_description=result.import_path,
)
for result in invalid_requests
# NB: We don't pass stdout/stderr as it will have already been rendered as streaming.
exit_code = next(
(
result.exit_code # type: ignore[attr-defined]
for result in (*build_results, *invalid_requests)
if result.exit_code != 0 # type: ignore[attr-defined]
),
*(
CheckResult(
result.exit_code,
result.stdout or "",
result.stderr or "",
partition_description=result.import_path,
)
for result in build_results
),
]
return CheckResults(check_results, checker_name="go")
0,
)
return CheckResults([CheckResult(exit_code, "", "")], checker_name="go compile")


def rules():
Expand Down
7 changes: 1 addition & 6 deletions src/python/pants/backend/go/goals/check_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,4 @@ def test_check(rule_runner: RuleRunner) -> None:
results = rule_runner.request(
CheckResults, [GoCheckRequest(GoCheckFieldSet.create(tgt) for tgt in targets)]
).results
assert set(results) == {
CheckResult(0, "", "", "example.com/greeter/good"),
CheckResult(
1, "", "bad/f.go:1:1: expected 'package', found invalid\n", "example.com/greeter/bad"
),
}
assert set(results) == {CheckResult(1, "", "")}
49 changes: 44 additions & 5 deletions src/python/pants/backend/go/util_rules/build_pkg.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,13 @@
from pants.backend.go.util_rules.sdk import GoSdkProcess
from pants.backend.go.util_rules.third_party_pkg import ThirdPartyPkgInfo, ThirdPartyPkgInfoRequest
from pants.build_graph.address import Address
from pants.engine.engine_aware import EngineAwareParameter
from pants.engine.engine_aware import EngineAwareParameter, EngineAwareReturnType
from pants.engine.fs import AddPrefix, Digest, MergeDigests
from pants.engine.process import FallibleProcessResult
from pants.engine.rules import Get, MultiGet, collect_rules, rule
from pants.engine.target import Dependencies, DependenciesRequest, UnexpandedTargets, WrappedTarget
from pants.util.frozendict import FrozenDict
from pants.util.logging import LogLevel
from pants.util.strutil import path_safe


Expand All @@ -58,7 +59,7 @@ def debug_hint(self) -> str | None:


@dataclass(frozen=True)
class FallibleBuildGoPackageRequest(EngineAwareParameter):
class FallibleBuildGoPackageRequest(EngineAwareParameter, EngineAwareReturnType):
"""Request to build a package, but fallible if determining the request metadata failed.
When creating "synthetic" packages, use `GoPackageRequest` directly. This type is only intended
Expand All @@ -70,9 +71,25 @@ class FallibleBuildGoPackageRequest(EngineAwareParameter):
exit_code: int = 0
stderr: str | None = None

def level(self) -> LogLevel:
return LogLevel.ERROR if self.exit_code != 0 else LogLevel.DEBUG

def message(self) -> str:
message = self.import_path
message += (
" succeeded." if self.exit_code == 0 else f" failed (exit code {self.exit_code})."
)
if self.stderr:
message += f"\n{self.stderr}"
return message

def cacheable(self) -> bool:
# Failed compile outputs should be re-rendered in every run.
return self.exit_code == 0


@dataclass(frozen=True)
class FallibleBuiltGoPackage:
class FallibleBuiltGoPackage(EngineAwareReturnType):
"""Fallible version of `BuiltGoPackage` with error details."""

output: BuiltGoPackage | None
Expand All @@ -81,6 +98,24 @@ class FallibleBuiltGoPackage:
stdout: str | None = None
stderr: str | None = None

def level(self) -> LogLevel:
return LogLevel.ERROR if self.exit_code != 0 else LogLevel.DEBUG

def message(self) -> str:
message = self.import_path
message += (
" succeeded." if self.exit_code == 0 else f" failed (exit code {self.exit_code})."
)
if self.stdout:
message += f"\n{self.stdout}"
if self.stderr:
message += f"\n{self.stderr}"
return message

def cacheable(self) -> bool:
# Failed compile outputs should be re-rendered in every run.
return self.exit_code == 0


@dataclass(frozen=True)
class BuiltGoPackage:
Expand All @@ -93,7 +128,9 @@ class BuiltGoPackage:
import_paths_to_pkg_a_files: FrozenDict[str, str]


@rule
# NB: We must have a description for the streaming of this rule to work properly
# (triggered by `FallibleBuiltGoPackage` subclassing `EngineAwareReturnType`).
@rule(desc="Compile with Go")
async def build_go_package(request: BuildGoPackageRequest) -> FallibleBuiltGoPackage:
maybe_built_deps = await MultiGet(
Get(FallibleBuiltGoPackage, BuildGoPackageRequest, build_request)
Expand Down Expand Up @@ -227,7 +264,9 @@ def debug_hint(self) -> str:
return str(self.address)


@rule
# NB: We must have a description for the streaming of this rule to work properly
# (triggered by `FallibleBuildGoPackageRequest` subclassing `EngineAwareReturnType`).
@rule(desc="Set up Go compilation request")
async def setup_build_go_package_target_request(
request: BuildGoPackageTargetRequest,
) -> FallibleBuildGoPackageRequest:
Expand Down
19 changes: 4 additions & 15 deletions src/python/pants/backend/java/compile/javac.py
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ async def compile_java_source(
)


@rule(desc="Check compilation for javac", level=LogLevel.DEBUG)
@rule(desc="Check javac compilation", level=LogLevel.DEBUG)
async def javac_check(request: JavacCheckRequest) -> CheckResults:
coarsened_targets = await Get(
CoarsenedTargets, Addresses(field_set.address for field_set in request.field_sets)
Expand All @@ -243,20 +243,9 @@ async def javac_check(request: JavacCheckRequest) -> CheckResults:
for t in coarsened_targets
)

# NB: We return CheckResults with exit codes for the root targets, but we do not pass
# stdout/stderr because it will already have been rendered as streaming.
return CheckResults(
[
CheckResult(
result.exit_code,
stdout="",
stderr="",
partition_description=str(coarsened_target),
)
for result, coarsened_target in zip(results, coarsened_targets)
],
checker_name="javac",
)
# NB: We don't pass stdout/stderr as it will have already been rendered as streaming.
exit_code = next((result.exit_code for result in results if result.exit_code != 0), 0)
return CheckResults([CheckResult(exit_code, "", "")], checker_name="javac")


def rules():
Expand Down
9 changes: 3 additions & 6 deletions src/python/pants/backend/java/compile/javac_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
from pants.backend.java.target_types import JavaSourcesGeneratorTarget
from pants.backend.java.target_types import rules as target_types_rules
from pants.build_graph.address import Address
from pants.core.goals.check import CheckResults
from pants.core.goals.check import CheckResult, CheckResults
from pants.core.util_rules import archive, config_files, source_files
from pants.core.util_rules.archive import UnzipBinary
from pants.core.util_rules.external_tool import rules as external_tool_rules
Expand Down Expand Up @@ -200,11 +200,8 @@ def test_compile_no_deps(rule_runner: RuleRunner) -> None:
[JavacCheckRequest.field_set_type.create(coarsened_target.representative)]
)
],
)

assert len(check_results.results) == 1
check_result = check_results.results[0]
assert check_result.partition_description == str(coarsened_target)
).results
assert set(check_results) == {CheckResult(0, "", "")}


@maybe_skip_jdk_test
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/jvm/compile.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ def prep_output(s: bytes) -> str:
)

def level(self) -> LogLevel:
return LogLevel.ERROR if self.exit_code != 0 else LogLevel.INFO
return LogLevel.ERROR if self.exit_code != 0 else LogLevel.DEBUG

def message(self) -> str:
message = self.description
Expand Down

0 comments on commit f9e5cf1

Please sign in to comment.