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

Building a target with a Bazel aspect overwrites the Target command line printout #10530

Open
r-hang opened this issue Jan 6, 2020 · 1 comment
Labels
help wanted Someone outside the Bazel team could own this team-Core Skyframe, bazel query, BEP, options parsing, bazelrc type: bug untriaged

Comments

@r-hang
Copy link

r-hang commented Jan 6, 2020

Description of the problem / feature request:

Building a target with a Bazel aspect overwrites the Target command line
printout. The Target printout previously contained information such as the
path to the generated build artifact.

For example:

$ bazel build :grpc_health_v1_proto --aspects=//rules:proto_linter.bzl%proto_linter_aspect --output_groups=+proto_lint_out

INFO: Writing tracer profile to
...
INFO: Found 1 target...
Target //idl/x/go/health/proto:grpc_health_v1_proto up-to-date:
  bazel-bin/idl/x/go/health/proto/grpc_health_v1_proto-descriptor-set.proto.bin
INFO: Elapsed time: 0.582s, Critical Path: 0.00s
INFO: 0 processes.
INFO: Build completed successfully, 1 total action

Has a different command line output then

$ bazel build :grpc_health_v1_proto

INFO: Writing tracer profile to 
...
INFO: Found 1 target...
Aspect //rules:proto_linter.bzl%proto_linter_aspect of //idl/x/go/health/proto:grpc_health_v1_proto up-to-date:
  bazel-bin/idl/xl/go/health/proto/grpc_health_v1_proto.protolint_result
INFO: Elapsed time: 0.493s, Critical Path: 0.00s
INFO: 0 processes.
INFO: Build completed successfully, 1 total action

The path to the aspect build artifact is printed, but the original build artifact path is lost.

I believe this issue is related to this [logic](https://source.bazel.build/bazel/+/master:src/main/java/com/google/devtools/build/lib/buildtool/BuildResultPrinter.java;bpv=;l=81 l) in Bazel.

Feature requests: what underlying problem are you trying to solve with this feature?

Building a Bazel target with an aspect should preserve the build target output message.

What operating system are you running Bazel on?

darwin18.0

What's the output of bazel info release?

release 1.1.0

Have you found anything relevant by searching the web?

I haven't found a relevant discussion online as of yet.

I believe this issue is related to this [logic](https://source.bazel.build/bazel/+/master:src/main/java/com/google/devtools/build/lib/buildtool/BuildResultPrinter.java;bpv=;l=81 l) in Bazel.

@irengrig irengrig added team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website untriaged labels Jan 8, 2020
@philwo philwo added P3 We're not considering working on this, but happy to review a PR. (No assignee) type: bug and removed untriaged labels Dec 9, 2020
@aschleck
Copy link

aschleck commented May 27, 2022

I am experiencing this too, using an internal type checking aspect similar to this mypy one.

Aspect //build_defs:pyright.bzl%pyright_check of //path/to/target:target up-to-date:
  bazel-bin/path/to/target_type_check

Which makes it very difficult to know what outputs different rules create when I am uncertain how the rules work. I am also glad there seems to be a recent PR for #8739, because the suggestion to just look at bazel build isn't possible when using aspects in this way.

I recognize that this is marked P3 and unlikely to get picked up, does the Bazel team have an idea of an acceptable way this could be implemented? I'd be open to submitting a PR.

Looking at the code it seems to do

aspectsToPrint = ... gather aspects ...
if (aspectsToPrint.isEmpty()) {
  ... print output ...
} else {
  for (aspect in aspectsToPrint) {
    for (artifact in aspect) {
      ... print it ...
    }
  }
}

Would it be acceptable to add an or-condition to the first arm? Like

aspectsToPrint = ... gather aspects ...
successfulAspects = result.getSuccessfulAspects();
anyNonValidationAspects =
          aspectsToPrint.stream()
              .filter(k ->
                  Sets.intersection(
                          OutputGroupInfo.get(aspects.get(k)).getFieldNames(), 
                          ImmutableSet.of(OutputGroupInfo.VALIDATION))
                      .isEmpty())
              .findAny()
              .isPresent()
anyFailedValidationAspects = 
          aspectsToPrint.stream()
              .filter(k -> !successfulAspects.contains(k))
              .filter(k ->
                  !Sets.intersection(
                          OutputGroupInfo.get(aspects.get(k)).getFieldNames(), 
                          ImmutableSet.of(OutputGroupInfo.VALIDATION))
                      .isEmpty())
              .findAny()
              .isPresent();
if (aspectsToPrint.isEmpty() || (!anyNonValidationAspects && !anyFailedValidationAspects)) {
  ... print output ...
} else {
  ...
}

brentleyjones added a commit to MobileNativeFoundation/rules_xcodeproj that referenced this issue Jun 3, 2022
brentleyjones added a commit to MobileNativeFoundation/rules_xcodeproj that referenced this issue Jun 3, 2022
@meteorcloudy meteorcloudy added team-Core Skyframe, bazel query, BEP, options parsing, bazelrc untriaged help wanted Someone outside the Bazel team could own this and removed team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website P3 We're not considering working on this, but happy to review a PR. (No assignee) labels Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Someone outside the Bazel team could own this team-Core Skyframe, bazel query, BEP, options parsing, bazelrc type: bug untriaged
Projects
None yet
Development

No branches or pull requests

5 participants