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

bazel query output order is wrong for rules with attr specifying aspects that depend on utility target #12320

Closed
ghvg1313 opened this issue Oct 20, 2020 · 8 comments
Assignees
Labels
P2 We'll consider working on this in future. (Assignee optional) query bugs Bugs related to "bazel (c)query" stale Issues or PRs that are stale (no activity for 30 days) team-Performance Issues for Performance teams type: bug under investigation

Comments

@ghvg1313
Copy link

Description of the problem:

We've been using bazel query --output=proto --order_output=full approach to calculate the targethash for our repo. This has been working relatively well until we introduced a custom rule (target instance A) that:

  • has an attr that that supports aspect propagation (attr.label(aspect=...)))
  • The underlying aspect implementation relies on another target (B)

This forms dependency from A to B, and by invoking GetRuleInput on A seems to confirm that, but the output order is wrong despite --order_output=full is specified.

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

I've forked the bazel example repo and made some small changes to simplify the case.
bazel query --output=proto --order_output=full "deps(//aspect:example_rule)" and check the output proto file to see the issue

What operating system are you running Bazel on?

macos 10.15.6

What's the output of bazel info release?

3.3.1

Have you found anything relevant by searching the web?

No

@ghvg1313 ghvg1313 changed the title bazel query output order is wrong bazel query output order is wrong for rules with attr specifying aspects that depend on utility target Oct 20, 2020
@jin jin added query bugs Bugs related to "bazel (c)query" team-Core Skyframe, bazel query, BEP, options parsing, bazelrc type: bug untriaged labels Oct 21, 2020
@meisterT meisterT added under investigation team-Performance Issues for Performance teams and removed team-Core Skyframe, bazel query, BEP, options parsing, bazelrc labels Oct 21, 2020
@zhengwei143
Copy link
Contributor

Could you clarify further what you meant by "the output order is wrong" in the proto file generated by your example, and how that affects the calculation of your target hash?

@ghvg1313
Copy link
Author

ghvg1313 commented Oct 27, 2020

when order_output=all is specified, the expectation is that, if target A depends on B, A should be appearing before B in the query output, with the example usage sometimes B will appear before A.

We are using the approach discussed in here to calculate the targethash and detect changed targets internally, parsing query output from bottom to top, with the assumption that query output order will reflect the dependency chain

@zhengwei143
Copy link
Contributor

zhengwei143 commented Oct 28, 2020

I think you meant order_output=full? Anyways, in this case I'm assuming A refers to example_rule and B refers to //aspect/util:UtilBin, correct me if I'm wrong.

I've tried reproducing it with your example and while B is a rule_input of A, it is not a dependency.

$ bazel query "deps(//aspect:example_rule)"
//aspect:example_rule
//aspect:lib.cc

Running bazel query --output=proto --order_output=full "deps(//aspect:example_rule)" only prints the targets (as above) and in the correct order.

The flag --order_output=full is supposed to be deterministic so it would be a problem if this is actually happening sometimes. If you happen to have a screenshot or a paste of your command/terminal outputting the wrong proto format with this example, I can help you look further into this.

@zhengwei143
Copy link
Contributor

It seems there might be an issue with --output=proto --order_output=deps here - result.getLabels() does not return the Target(s) in topological order which contradicts the documentation.

Not sure if this is related to your issue though since the order_output flag values are different.

@meisterT meisterT added the P1 I'll work on this now. (Assignee required) label Nov 9, 2020
@martinxsliu
Copy link

martinxsliu commented Nov 19, 2020

I have noticed something similar after upgrading to Bazel 3.7.0, maybe this is the issue @zhengwei143 mentions.

In my case it's the ordering of 2 package groups with --order_output=deps that is wrong. @bazel_tools//tools/whitelists/function_transition_whitelist:function_transition_whitelist depends on @bazel_tools//tools/allowlists/function_transition_allowlist:function_transition_allowlist. With deps I would expect allowlist to come first, however I see the opposite.

(base) ➜  code git:(master) bazel query --order_output=deps 'deps(//path/to/pkg)' | grep package
package group @bazel_tools//tools/whitelists/function_transition_whitelist:function_transition_whitelist
package group @bazel_tools//tools/allowlists/function_transition_allowlist:function_transition_allowlist

^ I've used output=label here, but I see the same with output=proto as well.

@zhengwei143 zhengwei143 added P2 We'll consider working on this in future. (Assignee optional) and removed P1 I'll work on this now. (Assignee required) labels Dec 8, 2020
@ghvg1313
Copy link
Author

ghvg1313 commented Apr 13, 2021

I think you meant order_output=full?

Yeah I meant order_output=full

@github-actions
Copy link

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 14 days unless any other activity occurs or one of the following labels is added: "not stale", "awaiting-bazeler". Please reach out to the triage team (@bazelbuild/triage) if you think this issue is still relevant or you are interested in getting the issue resolved.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label May 10, 2023
@github-actions
Copy link

This issue has been automatically closed due to inactivity. If you're still interested in pursuing this, please reach out to the triage team (@bazelbuild/triage). Thanks!

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We'll consider working on this in future. (Assignee optional) query bugs Bugs related to "bazel (c)query" stale Issues or PRs that are stale (no activity for 30 days) team-Performance Issues for Performance teams type: bug under investigation
Projects
None yet
Development

No branches or pull requests

6 participants