Add `--consistent_labels` flag to all query commands by fmeum · Pull Request #19508 · bazelbuild/bazel

fmeum · 2023-09-13T12:18:28Z

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.

fmeum · 2023-09-13T13:15:58Z

@gregestren @Wyverald This doesn't have tests yet. I would first like to confirm the approach taken by this PR as well as testing strategies.

src/test/py/bazel/bzlmod/bzlmod_query_test.py

Wyverald · 2023-09-13T17:43:42Z

To fix #18543, I was thinking that I'd change Label#toString() to return the @@-prefixed version. Do you think that affects this PR?

gregestren · 2023-09-13T17:45:47Z

I chatted with @Wyverald for some basic context.

I'm still not sure I understand the argument for *query-based Starlark output looking different than other Starlark output?

And the ultimate relationship between Starlark-formatted *query output and non-Starlark *query output?

fmeum · 2023-09-13T18:10:54Z

To fix #18543, I was thinking that I'd change Label#toString() to return the @@-prefixed version. Do you think that affects this PR?

But doesn't that run into the problem that Label#toString() doesn't have enough information to decide whether it should emit the @@ form? For example, if Bzlmod isn't enabled, it probably shouldn't. Speaking long-term I fully agree that this is the right approach.

It would affect this PR in that it would allow us to get rid of the legacy printer as it would become identical to the canonical printer.

Wyverald · 2023-09-13T18:15:19Z

For example, if Bzlmod isn't enabled, it probably shouldn't.

I think it's fine as the @@-syntax is valid whether or not Bzlmod is enabled. And it doesn't affect the semantics of str(Label(...)) whatsoever, so the "is Bzlmod enabled" check in bazel_features still works. WDYT?

fmeum · 2023-09-13T18:17:12Z

I'm still not sure I understand the argument for *query-based Starlark output looking different than other Starlark output?

There is no argument for that, that's simply a bug. See my other PR in which I wire up the correct StarlarkSemantics with cquery. That is fully separate from the current one and by itself ensures that any kind of Starlark output, be it print(str(Label(...))) in a rule or cquery --output=starlark, is consistent.

And the ultimate relationship between Starlark-formatted *query output and non-Starlark *query output?

With this commit and the flag enabled, they would become identical, which is very useful for any kind of tooling. But that requires e.g. regular query output to be of the form @@//pkg:target for main repo targets, which I assume we can't just roll out as the default (or ever inside Google). That's why the flag is, in my opinion, needed: The consistent output is too "ugly" to force on everyone by default.

fmeum · 2023-09-13T18:25:43Z

I think it's fine as the @@-syntax is valid whether or not Bzlmod is enabled. And it doesn't affect the semantics of str(Label(...)) whatsoever, so the "is Bzlmod enabled" check in bazel_features still works. WDYT?

I would be a huge fan of that change, but I so far didn't consider it possible: It is probably very breaking towards any kind of tooling, so to roll it out in Bazel 7 we would probably need a flag. Since Label#toString() doesn't have any other input to work with, this would probably need to be a startup flag that is statically injected into Label and hard-disabled inside Google.

If you think that this can work, I am all for it. Even with this PR cleaning up all query variants, there are many other places in Bazel that will still emit unusable label if we don't change Label#toString() output, so we will probably have to make that change anyway at some point. The only question is whether this can happen before WORKSPACE and all the tooling built around it can go away. ;-)

Wyverald · 2023-09-13T22:37:34Z

Yes I somehow forgot that Google exists :P. Make that "use @@-prefixed labels for everything outside the main repo" then (basically, where there used to be one @, make it two). I'm trying it and am running into a lot of surprising failures (actions having null owners??), so will need to dig some more.

Wyverald

overall LGTM. we should have (not a lot of) time to get this in for 6.4.0

Wyverald · 2023-09-15T21:25:38Z

src/main/java/com/google/devtools/build/lib/packages/LabelPrinter.java

+   * com.google.devtools.build.lib.query2.common.CommonQueryOptions#getLabelPrinter(StarlarkSemantics,
+   * RepositoryMapping)} instead.
+   */
+  static LabelPrinter canonical(StarlarkSemantics starlarkSemantics) {


I think I'd prefer to call this starlark or something (and in the same vein, call the other displayForm). canonical could sometimes output "apparent" labels (ie. with bzlmod off) and apparent, canonical labels (ie. when not visible to main repo).

Wyverald · 2023-09-15T21:28:23Z

src/main/java/com/google/devtools/build/lib/packages/LabelPrinter.java

+
+      @Override
+      public String toString(PackageIdentifier packageIdentifier) {
+        // This is adapted from the implementation of Label#str.


WDYT about just doing toString(Label.create(packageIdentifier, "DUMMY")).substring(0, -6)? (imaginary support for "-6" aside)

It avoids the code duplication and makes this much more easily maintainable, at the cost of a not-very-palatable runtime trick (which is presumably not a big deal since how often do we stringify PackageIdentifiers?)

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.

Wyverald

the changes look good. I'm iffy about the flag name --consistent_labels but I don't have a better idea, so unless @gregestren suggests otherwise, I say we merge this.

iancha1992 · 2023-09-18T16:41:15Z

@bazel-io fork 6.4.0

iancha1992 · 2023-09-18T16:48:31Z

@gregestren @Wyverald @fmeum looks like one of the tests is failing. I've tried a couple times. Can you please take a look?
cc: @bazelbuild/triage

fmeum · 2023-09-18T18:40:48Z

The failure is:

ERROR: Error computing the main repository mapping: in module dependency chain <root> -> bazel_tools@_ -> rules_java@6.3.1: Error accessing registry https://bcr.bazel.build: Connect timed out

This does look like an infrastructure problem to me, but it's surprising that it is persistent.

Wyverald · 2023-09-18T18:42:04Z

yeah I retried it yesterday and it was persistent too. I've asked Ian to try importing it anyway and see if the test fails there too.

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

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 <fabian@meumertzhe.im>

meteorcloudy · 2023-09-20T10:10:37Z

The GIT_AUTHOR name seems to be lost in 20cdacc during resolving internal conflicts, @fmeum hopefully you don't mind ;)

fmeum force-pushed the canonical_labels branch 9 times, most recently from 33d8b60 to 72b8dc9 Compare September 13, 2023 13:14

fmeum requested review from Wyverald and gregestren September 13, 2023 13:15

fmeum marked this pull request as ready for review September 13, 2023 13:15

fmeum requested review from lberki and meteorcloudy as code owners September 13, 2023 13:15

fmeum removed request for lberki and meteorcloudy September 13, 2023 13:15

fmeum force-pushed the canonical_labels branch 3 times, most recently from 77b7817 to d8787fc Compare September 13, 2023 13:49

fmeum commented Sep 13, 2023

View reviewed changes

src/test/py/bazel/bzlmod/bzlmod_query_test.py Show resolved Hide resolved

fmeum force-pushed the canonical_labels branch 3 times, most recently from c14ecd5 to 57e5f14 Compare September 13, 2023 16:10

fmeum mentioned this pull request Sep 15, 2023

Getting segfault when running gopackagesdriver on v0.40.0 & v0.41.0 bazel-contrib/rules_go#3604

Closed

Wyverald requested changes Sep 15, 2023

View reviewed changes

fmeum mentioned this pull request Sep 16, 2023

Check target incompatibility before toolchain resolution #17947

Closed

fmeum force-pushed the canonical_labels branch from 57e5f14 to e8ff2b0 Compare September 17, 2023 13:50

fmeum changed the title ~~Add --canonical_labels flag to all query commands~~ Add --consistent_labels flag to all query commands Sep 17, 2023

fmeum force-pushed the canonical_labels branch from e8ff2b0 to 947791c Compare September 17, 2023 14:03

fmeum force-pushed the canonical_labels branch from 947791c to 56bd5f4 Compare September 17, 2023 14:39

Wyverald approved these changes Sep 17, 2023

View reviewed changes

Wyverald added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Sep 17, 2023

gregestren approved these changes Sep 18, 2023

View reviewed changes

bazel-io mentioned this pull request Sep 18, 2023

[6.4.0] Add --consistent_labels flag to all query commands #19552

Closed

copybara-service bot closed this in 20cdacc Sep 19, 2023

github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Sep 19, 2023

Wyverald mentioned this pull request Sep 19, 2023

[6.4.0] Add --consistent_labels flag to all query commands #19567

Merged

fmeum deleted the canonical_labels branch September 19, 2023 21:57

Conversation

fmeum commented Sep 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmeum commented Sep 13, 2023

Uh oh!

Uh oh!

Wyverald commented Sep 13, 2023

Uh oh!

gregestren commented Sep 13, 2023

Uh oh!

fmeum commented Sep 13, 2023

Uh oh!

Wyverald commented Sep 13, 2023

Uh oh!

fmeum commented Sep 13, 2023

Uh oh!

fmeum commented Sep 13, 2023

Uh oh!

Wyverald commented Sep 13, 2023

Uh oh!

Wyverald left a comment

Choose a reason for hiding this comment

Uh oh!

Wyverald Sep 15, 2023

Choose a reason for hiding this comment

Uh oh!

Wyverald Sep 15, 2023

Choose a reason for hiding this comment

Uh oh!

Wyverald left a comment

Choose a reason for hiding this comment

Uh oh!

iancha1992 commented Sep 18, 2023

Uh oh!

iancha1992 commented Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmeum commented Sep 18, 2023

Uh oh!

Wyverald commented Sep 18, 2023

Uh oh!

meteorcloudy commented Sep 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fmeum commented Sep 13, 2023 •

edited

Loading

iancha1992 commented Sep 18, 2023 •

edited

Loading