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

cquery with starlark output uses wrong label for alias targets #18421

Open
illicitonion opened this issue May 16, 2023 · 2 comments
Open

cquery with starlark output uses wrong label for alias targets #18421

illicitonion opened this issue May 16, 2023 · 2 comments
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions type: bug

Comments

@illicitonion
Copy link
Contributor

Description of the bug:

https://bazel.build/extending/platforms#cquery-incompatible-target-detection gives the following snippet as the canonical way of filtering incompatible targets:

$ cat example.cquery

def format(target):
  if "IncompatiblePlatformProvider" not in providers(target):
    return target.label
  return ""


$ bazel cquery //... --output=starlark --starlark:file=example.cquery

Unfortunately, target.label returns the label of actual, not the alias itself, for alias targets:

% cat java/example/BUILD.bazel 
alias(
    name = "example_test",
    actual = ":ExampleTest",
)

java_test(
    name = "ExampleTest",
    srcs = ["ExampleTest.java"],
)
% USE_BAZEL_VERSION=6.2.0 bazel cquery --output=starlark '--starlark:expr=target.label if "IncompatiblePlatformProvider" not in providers(target) else ""' ... 2>/dev/null
@//java/example:ExampleTest
@//java/example:ExampleTest

The repr of the target contains both pieces of information:

% USE_BAZEL_VERSION=6.2.0 bazel cquery --output=starlark '--starlark:expr=target if "IncompatiblePlatformProvider" not in providers(target) else ""' ... 2>/dev/null 
<target //java/example:ExampleTest>
<alias target //java/example:example_test of //java/example:ExampleTest>

I would expect target.label to return the actual label of all targets, and for any alias lookup to require looking at a specific property or provider, rather than automatically passing through.

AliasConfiguredTarget explicitly overrides getLabel to have this behaviour,

@Override
public Label getLabel() {
return actual.getLabel();
}

AFAICT the quick way to fix this would be to use a custom StarlarkSemantics for cquery, and override Object getValue(StarlarkSemantics semantics, String name) on AliasConfiguredTarget so that getValue(CquerySemantics, "label") returns the original label not the aliased one. I suspect actually changing any of the behaviours of getLabel anywhere else would be a huge scary change...

It's worth noting this isn't the first time this has come up - see

/**
* This method has to exist because {@link AliasConfiguredTarget#getLabel()} returns the label of
* the "actual" target instead of the alias target. Grr.
*/
@Override
public Label getCorrectLabel(KeyedConfiguredTarget target) {
// Dereference any aliases that might be present.
return target.getConfiguredTarget().getOriginalLabel();
}
and
@Override
public Label getCorrectLabel(KeyedConfiguredTargetValue keyedConfiguredTargetValue) {
ConfiguredTarget target = keyedConfiguredTargetValue.getConfiguredTarget();
// Dereference any aliases that might be present.
return target.getOriginalLabel();
}

/cc @lberki as the author of AliasConfiguredTarget, @gregestren and @fmeum as cquery --output=starlark folks.

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

No response

Which operating system are you running Bazel on?

No response

What is the output of bazel info release?

release 6.2.0

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response

@fmeum
Copy link
Collaborator

fmeum commented May 16, 2023

Instead of using semantics, we could also add a new field to target that contains the label as specified by the user (i.e. the label of an alias if the target is one). See also #18100 for other use cases.

Cc @comius

@illicitonion
Copy link
Contributor Author

we could also add a new field to target that contains the label as specified by the user (i.e. the label of an alias if the target is one

I think if we were to go about this, ideally we should swap the meaning - I think most users when writing a starlark query would expect target.label to be the label of the target they're querying for, and that target.alias_actual_label or something would exist as a special-case, rather than the other way around. But I recognise there are backward-compatibility issues too...

@gregestren gregestren added the team-Configurability platforms, toolchains, cquery, select(), config transitions label May 16, 2023
illicitonion added a commit to illicitonion/target-determinator that referenced this issue May 16, 2023
illicitonion added a commit to illicitonion/target-determinator that referenced this issue May 17, 2023
illicitonion added a commit to bazel-contrib/target-determinator that referenced this issue May 17, 2023
@gregestren gregestren added P2 We'll consider working on this in future. (Assignee optional) and removed untriaged labels Jun 26, 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) team-Configurability platforms, toolchains, cquery, select(), config transitions type: bug
Projects
None yet
Development

No branches or pull requests

5 participants