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 label outputs inconsistent after bazel 6 #17864

Closed
illicitonion opened this issue Mar 23, 2023 · 10 comments
Closed

cquery label outputs inconsistent after bazel 6 #17864

illicitonion opened this issue Mar 23, 2023 · 10 comments
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Configurability platforms, toolchains, cquery, select(), config transitions team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: bug

Comments

@illicitonion
Copy link
Contributor

illicitonion commented Mar 23, 2023

Description of the bug:

% USE_BAZEL_VERSION=5.4.0 bazel cquery --output=starlark --starlark:expr='target.label' ... 2>/dev/null
//java/example/simple:simple
//java/example/simple:simple_dep
% USE_BAZEL_VERSION=6.0.0 bazel cquery --output=starlark --starlark:expr='target.label' ... 2>/dev/null
@//java/example/simple:simple
@//java/example/simple:simple_dep

It looks like in Bazel 6, in-repo labels started stringifying with a leading @.

This is inconsistent with regular cquery output, which still doesn't have leading @s for labels:

% USE_BAZEL_VERSION=6.0.0 bazel cquery ... 2>/dev/null                  
//java/example/simple:simple (a42135a)
//java/example/simple:simple_dep (a42135a)

This means that if trying to filter cquery output based on some --output=starlark query expression (e.g. filtering out incompatible targets, as documented here), manual code needs to be written to add or strip a leading @.

It would seem useful for all cquery commands, and the native starlark label stringification, to use consistent stringification, so that simpler equality checks can be performed when processing the output.

@illicitonion
Copy link
Contributor Author

/cc @gregestren @sdtwigg @fmeum

@fmeum
Copy link
Collaborator

fmeum commented Mar 23, 2023

CC @Wyverald

Getting rid of the @ would probably require wiring up the actual StarlarkSemantics in

StarlarkThread thread = new StarlarkThread(mu, StarlarkSemantics.DEFAULT);
instead of using the defaults.

Having consistent stringification across cquery output modes is challenging with Bzlmod: In the interest of emitting human-readable labels, I implemented main repository mapping "unmapping" to the default output mode. This means that canonical repository names such as @rules_go~1.2.3~go_deps~foo will be printed as @foo if the main repository declares the apparent name foo for this repo.

But in general Starlark code, the stringification of a label should always yield a canonical label string such as @@rules_go~1.2.3~go_deps~foo//.... Changing that behavior just for cquery would seem to require passing in a custom StarlarkSemantics object for cquery that includes the main repository mapping.

@ShreeM01 ShreeM01 added type: bug team-Configurability platforms, toolchains, cquery, select(), config transitions untriaged labels Mar 23, 2023
@aiuto
Copy link
Contributor

aiuto commented Mar 29, 2023

I've also seen @ vs @@ showing up as differences across linux vs. windows, within the same bazel version.
I normalizae them to all be single @

@Wyverald
Copy link
Member

I've also seen @ vs @@ showing up as differences across linux vs. windows, within the same bazel version.

That shouldn't be possible. (Or it's a bug that we need to fix.)

In any case, the proper way to fix for this issue is unclear to me because of what @fmeum wrote above. The StarlarkSemantics object is really just a collection of flag values; plugging in a complex data structure like the main repo mapping sounds like a big change if possible at all.

@katre
Copy link
Member

katre commented Mar 31, 2023

@Wyverald Can you triage this and reassign it to the correct label?

@Wyverald Wyverald added P3 We're not considering working on this, but happy to review a PR. (No assignee) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. and removed untriaged labels Mar 31, 2023
@Wyverald
Copy link
Member

team-Configurability isn't technically wrong since this is about cquery. But I'll keep an eye on this too.

@fmeum
Copy link
Collaborator

fmeum commented Jul 27, 2023

Looking into this again. Would anyone find it surprising if Labels stringify differently in cquery formatters than they do in regular Starlark if this brings us consistency with query and other cquery outputs?

@fmeum
Copy link
Collaborator

fmeum commented Aug 22, 2023

As noticed in bazel-contrib/rules_go#3659 (comment), there needs to be a way to match the output of str(Label(...)) to that of query and cquery.

Thus, instead of trying to force the main repo mapping into cquery --output=starlark, I think that we need a --canonical_labels flag for query and cquery that ensures that all labels in the output are in unambiguous canonical form.

@Wyverald
Copy link
Member

Wyverald commented Sep 8, 2023

I think that we need a --canonical_labels flag for query and cquery that ensures that all labels in the output are in unambiguous canonical form.

This sounds good to me. It's probably the least messy way to fix this... mess.

Wyverald added a commit that referenced this issue Sep 19, 2023
If enabled, labels are emitted as if by the Starlark `str` function applied to a `Label` instance. This is useful for tools that need to match the output of different query commands and/or labels emitted by rules. If not enabled, output formatters are free to emit apparent repository names (relative to the main repository) instead to make the output more readable.

This is implemented by replacing all `Label#toString()` calls in query code with an indirection through a `LabelPrinter` object constructed in a central place. There are currently three types of instances:
* `canonical` behaves just like `str(Label(...))`;
* `apparent` matches the current behavior, which renders main repo labels in repo-relative form and attempts to use the apparent names for external repos if possible;
* `legacy` is used in all non-query callsites and uses `Label#toString()`.

Fixes #17864

RELNOTES: The new `--consistent_labels` option on `query`, `cquery`, and `aquery` can be used to force consistent label formatting across all output modes that is also compatible with `str(Label(...))` in Starlark.

Closes #19508.

PiperOrigin-RevId: 566723654
Change-Id: Ifa08a82e281f423faa971c46bf7277cb307698b5
copybara-service bot pushed a commit that referenced this issue Sep 19, 2023
Previously, `BuildLanguageOption`s had no effect as the default semantics were used.

Work towards #17864

Closes #19334.

PiperOrigin-RevId: 566747820
Change-Id: I5071f97c68e02808ea8183b9c52c89a2e8e39620
Wyverald pushed a commit that referenced this issue Sep 19, 2023
…ark`

Previously, `BuildLanguageOption`s had no effect as the default semantics were used.

Work towards #17864

Closes #19334.

PiperOrigin-RevId: 566747820
Change-Id: I5071f97c68e02808ea8183b9c52c89a2e8e39620
Wyverald added a commit that referenced this issue Sep 19, 2023
If enabled, labels are emitted as if by the Starlark `str` function
applied to a `Label` instance. This is useful for tools that need to
match the output of different query commands and/or labels emitted by
rules. If not enabled, output formatters are free to emit apparent
repository names (relative to the main repository) instead to make the
output more readable.

This is implemented by replacing all `Label#toString()` calls in query
code with an indirection through a `LabelPrinter` object constructed in
a central place. There are currently three types of instances:
* `canonical` behaves just like `str(Label(...))`;
* `apparent` matches the current behavior, which renders main repo
labels in repo-relative form and attempts to use the apparent names for
external repos if possible;
* `legacy` is used in all non-query callsites and uses
`Label#toString()`.

Fixes #17864

RELNOTES: The new `--consistent_labels` option on `query`, `cquery`, and
`aquery` can be used to force consistent label formatting across all
output modes that is also compatible with `str(Label(...))` in Starlark.

Closes #19508.

PiperOrigin-RevId: 566723654
Change-Id: Ifa08a82e281f423faa971c46bf7277cb307698b5

---------

Co-authored-by: Fabian Meumertzheim <[email protected]>
@iancha1992
Copy link
Member

A fix for this issue has been included in Bazel 6.4.0 RC1. Please test out the release candidate and report any issues as soon as possible. Thanks!

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) team-Configurability platforms, toolchains, cquery, select(), config transitions team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants