Stringify `Label`s in display form in `Args` #21702

fmeum · 2024-03-14T17:46:19Z

Labels added to Args are now formatted in display form, which means that they will be repo-relative if referring to the main repo and use an apparent repository name if possible.

Previously, labels were formatted in repo-relative form if referring to the main repo and using canonical repository names exclusively for external repos. At the same time, Args docs explicitly stated that the exact stringification of any type other than File is unspecified. The previous behavior was problematic since it neither always used canonical labels (which could be useful for writing tests that check these labels against an allowlist) nor provided labels that could be included in BUILD files (canonical names are explicitly unstable). Furthermore, whether a label resulted in a string prefixed with a single or two @s already dependended on a user choice, namely the value of --enable_bzlmod.

Thus, this change is not considered to be breaking. It makes it so that existing rulesets relying on default stringification to return a BUILD-file compatible label in WORKSPACE continue to do so with Bzlmod with no code changes.

This change aims to be as memory efficient as possible:

Single labels or sequences of labels that reference targets in the main repo incur no memory overhead.
Labels referring to external repos as well as NestedSets of labels result in an additional Skyframe dependency for each target using the command line as well as one additional reference (4 bytes) stored in the command line's arguments.

Work towards #20486

fmeum · 2024-03-14T21:46:22Z

@justinhorvitz This is my attempt at realizing your suggestions to reduce memory usage while also addressing @Wyverald's concerns about adding yet another method to Label that only has a very niche use case. In fact, this PR adds no new API and instead, in my opinion, improves the so far unspecified default stringification behavior.

Edit: I will add unit test coverage for the new Args logic if you agree with the overall approach.

fmeum · 2024-03-14T22:08:34Z

@zhengwei143 I am using Java 21 API features in this PR, but it results in compilation failures on Windows arm64. Are you still working on this?

zhengwei143 · 2024-03-14T22:39:16Z

@zhengwei143 I am using Java 21 API features in this PR, but it results in compilation failures on Windows arm64. Are you still working on this?

We've identified this towards the later part of today, 2 parts need that likely need to be fixed:

Flipping this to use remotejdk_21.
Adding win_arm64 to rules_java: https://github.com/bazelbuild/rules_java/blob/fa37f6120af318849eaf87d1b471e154adf01c61/MODULE.bazel#L60-L67

We'll look into this tomorrow morning.

per #21702 (comment)

justinhorvitz · 2024-03-22T02:36:49Z

Sorry for the delay. Can you sync to pick up de4bcb1? I suspect you might have non-trivial merge conflicts. Then I'll review.

fmeum · 2024-03-22T11:20:45Z

@justinhorvitz I resolved the conflicts.

justinhorvitz

Overall approach looks good.

src/main/java/com/google/devtools/build/lib/analysis/starlark/Args.java

src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java

justinhorvitz · 2024-03-22T17:47:45Z

src/main/java/com/google/devtools/build/lib/starlarkbuildapi/CommandLineArgsApi.java

+            + " context of the main repository. If possible, the string representation uses the"
+            + " apparent name of a repository in favor of the repository's canonical name, which"
+            + " makes this representation suited for use in BUILD files. While the exact form of"
+            + " the representation is not guaranteed, typical examples are"


Why is it not guaranteed?

I didn't want to commit to the full details of how labels are formed: Are we emitting @foo as @foo or @foo//:foo? Do we always return labels with an apparent repo name if there is one for the repo? I could expand on the guarantees if you think that's worthwhile and doesn't unnecessarily constrain us.

I think we should get an opinion from @Wyverald for this documentation. I don't deal with repository mappings much so I'm not sure what to document here. In general documentation that makes it sound like a mystery is concerning to me, so an additional opinion would help.

I don't have strong opinions here; but IMO the things you mentioned --

Are we emitting @foo as @foo or @foo//:foo? Do we always return labels with an apparent repo name if there is one for the repo?

-- are perfectly fine to guarantee. I can't really foresee a situation where we'd want to emit @foo//:foo instead of @foo, or use a canonical label when an apparent one exists.

We do emit @foo//:foo at the moment though. I could see getting rid of the colon ending up breaking tooling that consumes these labels, so I'm not sure whether that would make it safe to cherry-pick. At the same time, guaranteeing that we never use the shorthand also doesn't seem ideal...

hmm, that's fair. I'll leave it up to your judgment.

Since the change would be doc only, I'll mentally snooze and revisit it later.

fmeum · 2024-03-27T12:29:18Z

I fixed the test failure, the remaining one appears to be by chance.

justinhorvitz

Looks good!

fmeum · 2024-03-27T13:49:58Z

@bazel-io fork 7.2.0

...est/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java

`Label`s added to `Args` are now formatted in display form, which means that they will be repo-relative if referring to the main repo and use an apparent repository name if possible. Previously, labels were formatted in repo-relative form if referring to the main repo and using canonical repository names exclusively for external repos. At the same time, `Args` docs explicitly stated that the exact stringification of any type other than `File` is unspecified. The previous behavior was problematic since it neither always used canonical labels (which could be useful for writing tests that check these labels against an allowlist) nor provided labels that could be included in BUILD files (canonical names are explicitly unstable). Furthermore, whether a label resulted in a string prefixed with a single or two `@`s already dependended on a user choice, namely the value of `--enable_bzlmod`. Thus, this change is not considered to be breaking. It makes it so that existing rulesets relying on default stringification to return a BUILD- file compatible label in WORKSPACE continue to do so with Bzlmod with no code changes. This change aims to be as memory efficient as possible: * Single labels or sequences of labels that reference targets in the main repo incur no memory overhead. * Labels referring to external repos as well as `NestedSet`s of labels result in an additional Skyframe dependency for each target using the command line as well as one additional reference (4 bytes) stored in the command line's `arguments`.

The change in #21702 only fixed the buildozer fixup messages for `java_*` actions entirely defined in Starlark. This commit makes the analogous change to the remaining native actions. Related to #20486 and #21702 Closes #21827. PiperOrigin-RevId: 623253302 Change-Id: Iadc2b0d8ce64f359b921ad7a7c489111a3a29997

The change in bazelbuild#21702 only fixed the buildozer fixup messages for `java_*` actions entirely defined in Starlark. This commit makes the analogous change to the remaining native actions. Related to bazelbuild#20486 and bazelbuild#21702 Closes bazelbuild#21827. PiperOrigin-RevId: 623253302 Change-Id: Iadc2b0d8ce64f359b921ad7a7c489111a3a29997

`Label`s added to `Args` are now formatted in display form, which means that they will be repo-relative if referring to the main repo and use an apparent repository name if possible. Previously, labels were formatted in repo-relative form if referring to the main repo and using canonical repository names exclusively for external repos. At the same time, `Args` docs explicitly stated that the exact stringification of any type other than `File` is unspecified. The previous behavior was problematic since it neither always used canonical labels (which could be useful for writing tests that check these labels against an allowlist) nor provided labels that could be included in BUILD files (canonical names are explicitly unstable). Furthermore, whether a label resulted in a string prefixed with a single or two `@`s already dependended on a user choice, namely the value of `--enable_bzlmod`. Thus, this change is not considered to be breaking. It makes it so that existing rulesets relying on default stringification to return a BUILD-file compatible label in WORKSPACE continue to do so with Bzlmod with no code changes. This change aims to be as memory efficient as possible: * Single labels or sequences of labels that reference targets in the main repo incur no memory overhead. * Labels referring to external repos as well as `NestedSet`s of labels result in an additional Skyframe dependency for each target using the command line as well as one additional reference (4 bytes) stored in the command line's `arguments`. Work towards bazelbuild#20486 Closes bazelbuild#21702. PiperOrigin-RevId: 620925978 Change-Id: I54aa807c41bf783aee223482d2309f5cee2726b5

The change in bazelbuild#21702 only fixed the buildozer fixup messages for `java_*` actions entirely defined in Starlark. This commit makes the analogous change to the remaining native actions. Related to bazelbuild#20486 and bazelbuild#21702 Closes bazelbuild#21827. PiperOrigin-RevId: 623253302 Change-Id: Iadc2b0d8ce64f359b921ad7a7c489111a3a29997

fmeum force-pushed the 20486-label-args branch 3 times, most recently from efd5fa4 to 1dea68c Compare March 14, 2024 17:57

fmeum changed the title ~~20486 label args~~ Stringify Labels in display form in Args Mar 14, 2024

fmeum force-pushed the 20486-label-args branch 3 times, most recently from ec8a22c to f3aa519 Compare March 14, 2024 21:42

fmeum requested review from justinhorvitz and Wyverald March 14, 2024 21:44

fmeum marked this pull request as ready for review March 14, 2024 21:44

fmeum requested a review from a team as a code owner March 14, 2024 21:44

fmeum requested review from katre and removed request for a team and katre March 14, 2024 21:44

github-actions bot added team-Configurability platforms, toolchains, cquery, select(), config transitions awaiting-review PR is awaiting review from an assigned reviewer labels Mar 14, 2024

fmeum force-pushed the 20486-label-args branch 2 times, most recently from 671986c to 21296d7 Compare March 14, 2024 21:50

fmeum mentioned this pull request Mar 20, 2024

Split ModuleThreadContext off from ModuleFileGlobals #21749

Closed

Wyverald added a commit that referenced this pull request Mar 21, 2024

Use remotejdk_21 on windows_arm64

0db171d

per #21702 (comment)

Wyverald mentioned this pull request Mar 21, 2024

Use remotejdk_21 on windows_arm64 #21761

Closed

fmeum force-pushed the 20486-label-args branch from e96c574 to a902cd5 Compare March 22, 2024 11:20

justinhorvitz reviewed Mar 22, 2024

View reviewed changes

fmeum force-pushed the 20486-label-args branch from c22a9f8 to 287d696 Compare March 23, 2024 21:02

fmeum requested a review from justinhorvitz March 23, 2024 22:33

justinhorvitz approved these changes Mar 27, 2024

View reviewed changes

bazel-io mentioned this pull request Mar 27, 2024

[7.2.0] Stringify Labels in display form in Args #21826

Closed

fmeum mentioned this pull request Mar 27, 2024

Show display form labels in java_* buildozer fixups #21827

Closed

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 Mar 28, 2024

Wyverald reviewed Mar 29, 2024

View reviewed changes

...est/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java Outdated Show resolved Hide resolved

fmeum force-pushed the 20486-label-args branch from c62ee1b to 6bf0fd0 Compare March 29, 2024 23:07

fmeum requested a review from Wyverald March 29, 2024 23:07

fmeum added 8 commits March 31, 2024 11:11

WIP test

84d24e7

Address most review comments

0973ab5

Fix bug and add tests

2283485

Revert to previous attempt

d4cacd3

Update Args.java

985d169

Add explicit module file

40f23aa

Attempt to use a regular bazel_dep in tests

6cc995b

fmeum force-pushed the 20486-label-args branch from 6bf0fd0 to 6cc995b Compare March 31, 2024 09:11

copybara-service bot closed this in 9d3a8b0 Apr 1, 2024

github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Apr 1, 2024

fmeum deleted the 20486-label-args branch April 1, 2024 22:04

iancha1992 mentioned this pull request Apr 9, 2024

[7.2.0] Show display form labels in java_* buildozer fixups #21948

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stringify `Label`s in display form in `Args` #21702

Stringify `Label`s in display form in `Args` #21702

fmeum commented Mar 14, 2024 •

edited

Loading

fmeum commented Mar 14, 2024 •

edited

Loading

fmeum commented Mar 14, 2024

zhengwei143 commented Mar 14, 2024

justinhorvitz commented Mar 22, 2024

fmeum commented Mar 22, 2024

justinhorvitz left a comment

justinhorvitz Mar 22, 2024

fmeum Mar 23, 2024

justinhorvitz Mar 27, 2024

Wyverald Mar 28, 2024

fmeum Mar 28, 2024

Wyverald Mar 28, 2024

fmeum Mar 28, 2024

fmeum commented Mar 27, 2024

justinhorvitz left a comment

fmeum commented Mar 27, 2024

Stringify Labels in display form in Args #21702

Stringify Labels in display form in Args #21702

Conversation

fmeum commented Mar 14, 2024 • edited Loading

fmeum commented Mar 14, 2024 • edited Loading

fmeum commented Mar 14, 2024

zhengwei143 commented Mar 14, 2024

justinhorvitz commented Mar 22, 2024

fmeum commented Mar 22, 2024

justinhorvitz left a comment

Choose a reason for hiding this comment

justinhorvitz Mar 22, 2024

Choose a reason for hiding this comment

fmeum Mar 23, 2024

Choose a reason for hiding this comment

justinhorvitz Mar 27, 2024

Choose a reason for hiding this comment

Wyverald Mar 28, 2024

Choose a reason for hiding this comment

fmeum Mar 28, 2024

Choose a reason for hiding this comment

Wyverald Mar 28, 2024

Choose a reason for hiding this comment

fmeum Mar 28, 2024

Choose a reason for hiding this comment

fmeum commented Mar 27, 2024

justinhorvitz left a comment

Choose a reason for hiding this comment

fmeum commented Mar 27, 2024

Stringify `Label`s in display form in `Args` #21702

Stringify `Label`s in display form in `Args` #21702

fmeum commented Mar 14, 2024 •

edited

Loading

fmeum commented Mar 14, 2024 •

edited

Loading