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

target_compatible_with with genquery fails in cases where query would succeed #12948

Closed
keith opened this issue Feb 2, 2021 · 8 comments
Closed
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) team-Configurability platforms, toolchains, cquery, select(), config transitions type: bug

Comments

@keith
Copy link
Member

keith commented Feb 2, 2021

With this root BUILD file:

constraint_setting(name = "incompatible_setting")

constraint_value(
    name = "incompatible",
    constraint_setting = ":incompatible_setting",
)

sh_test(
    name = "bar",
    srcs = ["BUILD"],
    target_compatible_with = ["//:incompatible"],
)

genquery(
    name = "query",
    testonly = True,
    expression = "//:bar",
    scope = ["//:bar"],
)

If you run bazel build //:query you get this failure:

% bazel build //:query
ERROR: Target //:query is incompatible and cannot be built, but was explicitly requested.
Dependency chain:
    //:query
    //:bar   <-- target platform didn't satisfy constraint //:incompatible
INFO: Elapsed time: 0.075s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (1 packages loaded, 5 targets configured)

Interestingly if you omit the //: from the target, it skips the genquery all together:

% bazel build query
INFO: Analyzed target //:query (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target //:query was skipped
INFO: Elapsed time: 0.100s, Critical Path: 0.00s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action

If you just query this target, it works:

% bazel query //:bar
//:bar

I'm not sure what the expectations are around genquery and it's similarity to query, but it would be ideal in this case if genquery would also succeed. Also I think the difference in behavior when you have //: or not is unexpected.

What operating system are you running Bazel on?

macOS

What's the output of bazel info release?

release 4.0.0

@oquenchil oquenchil added team-Configurability platforms, toolchains, cquery, select(), config transitions type: bug untriaged labels Feb 4, 2021
@aiuto aiuto added P2 We'll consider working on this in future. (Assignee optional) and removed untriaged labels Feb 12, 2021
@gregestren
Copy link
Contributor

gregestren commented Feb 12, 2021

Interesting. There's a good argument here. I'm not sure what the right answer is.

I believe the reason the first build fails is because genquery is just a target depending on another target (through scope = ["//:bar"]). So when you build, these get analyzed just like any other targets. And since incompatibility propagates upward, //:bar's incompatibility applies to //:query.

But yes, since the only purpose of that dep is to scope the space of queryable targets, the target configuration isn't important for the actual query.

On the other hand, if //:bar really isn't buildable, we don't want the whole thing to just fail with a cryptic message. That's the whole point of incompatibility checking.

Conceptually I lean toward your intuition but I don't know how to fit that into genquery's implementation. The only "easy" fix I can think of is to just exempt genquery from propagation and accept the possibility of cryptic errors.

FYI some relevant code is here:

ruleContext.attributes().get("scope", BuildType.LABEL_LIST),

Re: //:query vs. query, I strongly suspect this is incompatible skipping logic checking for explicitly vs. implicitly requested targets. It considers bazel build //:query explicit and bazel build //... implicit. And since query doesn't match //:query it treats that as implicit too.

FYI @philsc

@gregestren gregestren added P3 We're not considering working on this, but happy to review a PR. (No assignee) and removed P2 We'll consider working on this in future. (Assignee optional) labels Feb 12, 2021
@gregestren gregestren removed their assignment Feb 12, 2021
@philsc
Copy link
Contributor

philsc commented Feb 13, 2021

At the very least I should fix the query vs. //:query difference. It really shouldn't be different.

Mayber after that I'll have my head wrapped around what the better behaviour is.

@philsc
Copy link
Contributor

philsc commented Feb 13, 2021

I split the query vs. //:query behaviour into #13019.

@philsc
Copy link
Contributor

philsc commented Feb 14, 2021

Just to clarify for my own thoughts, can you elaborate on what you mean by the following @keith ?

it would be ideal in this case if genquery would also succeed

Do you mean that it would be ideal for the sake of consistency or is there a particular use case for querying incompatible targets you have in mind?

@keith
Copy link
Member Author

keith commented Feb 16, 2021

I meant for consistency. In this case I just want to query these targets "like normal" even though they're incompatible in some configurations.

@gregestren
Copy link
Contributor

You can get a similar failure even without target_compatible_with:

config_setting(
    name = "a",
    define_values = {"a": "1"})

genrule(
    name = "g",
    srcs = [],
    outs = ["g.out"],
    cmd = select({":a": "echo hi > $@",}))

genquery(
    name = "gq",
    expression = "//testapp:g",
    scope = [":g"])
$ bazel build //testapp:gq
ERROR: testapp/BUILD:8:8: Configurable attribute "cmd" doesn't match this configuration (would a
default condition help?).
Conditions checked:
 //testapp:a
ERROR: Analysis of target '//testapp:gq' failed; build aborted: 
$ bazel build //testapp:gq --define a=1
INFO: Build completed successfully, 1 total action

@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 2+ 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 Apr 28, 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 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) team-Configurability platforms, toolchains, cquery, select(), config transitions type: bug
Projects
None yet
Development

No branches or pull requests

5 participants