Adjust --top_level_targets_for_symlinks #18854

gregestren · 2023-07-06T21:44:59Z

Adjust --top_level_targets_for_symlinks.

Old semantics:

If all targets in $ bazel build //... have the top-level config, bazel-bin, etc. point to that config
If all targets in $ bazel build //... have the same transitioned config, bazel-bin, etc. point to that config
If targets in $ bazel build //... have mixed configs, bazel-bin, etc. are deleted

New semantics:

If all targets in $ bazel build //... have the top-level config, bazel-bin, etc. point to that config
If all targets in $ bazel build //... have the same transitioned config, bazel-bin, etc. point to that config
If targets in $ bazel build //... have mixed configs and at least one of them is the top-level config, bazel-bin, etc. point to the top-level config
If targets in $ bazel build //... have mixed configs and none are the top-level config, bazel-bin, etc. are deleted

Fixes #17081.

gregestren · 2023-07-06T21:47:41Z

src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java

+        // https://github.com/bazelbuild/bazel/issues/17081.
+        targetConfigs = ImmutableSet.of(configuration);
+      } else {
+        // Mixed configs, none of which include the top-level config. Delete the symlinks because


Should we include this case?

Pro: the symlink has nothing useful
Con: this will trigger when $ bazel build //... doesn't include any tests, since --trim_test_configuration will remove test options from all targets. Users may be confused why there are no symlinks.

Could you elaborate on the Con? Does --trim_test_configuration result in duplicate configs? If so, could we somehow recognize when a config differs from the top-level config only in test options and ignore those configs here to fix the Con?

It's just a con in that it means bazel-bin will still be deleted more than users expect. If $ bazel build //:all expands to 10 binaries, where only 1 has a self-transition, bazel-bin will still be deleted. If $ bazel build //:all also includes a test, bazel-bin will show up.

Worse, the non-transitioning binaries would actually appear in bazel-bin if it wasn't deleted. Even though they lack TestOptions they still have the same bazel-out/foo-bar/bin path because that instance of config trimming doesn't affect paths.

I'd rather not get into explicitly examining the configuration diffs. Another approach is to not compare the configs but their output paths.

Latest commit implements that, and manual testing shows intuitive results (I tried every combo of test, non-transitioning non-test, transitioning non-test).

I like that change, comparing paths instead of configs makes sense as those are all we care about in the end.

gregestren · 2023-07-06T21:51:03Z

Re: testing:

This is an opportunity to open-source another Google test.

ConvenienceSymlinkTest covers --use_top_level_targets_for_symlinks. I'm trying to open-source it in https://bazel-review.googlesource.com/c/bazel/+/224150. CI mostly looks good but seems to be struggling with Apple: https://buildkite.com/bazel/google-bazel-presubmit/builds/69162

Options:

Push this PR ASAP, add tests as a separate PR after https://bazel-review.googlesource.com/c/bazel/+/224150 lands
Block this PR on https://bazel-review.googlesource.com/c/bazel/+/224150, add tests here

gregestren · 2023-07-06T22:21:48Z

ConvenienceSymlinkTest covers --use_top_level_targets_for_symlinks. I'm trying to open-source it in https://bazel-review.googlesource.com/c/bazel/+/224150. CI mostly looks good but seems to be struggling with Apple: https://buildkite.com/bazel/google-bazel-presubmit/builds/69162

A small patch fixes the CI: https://buildkite.com/bazel/google-bazel-presubmit/builds/69167. I'll push https://bazel-review.googlesource.com/c/bazel/+/224150 into Google code review to get the test open-sourced however we decide the above.

fmeum · 2023-07-07T09:59:38Z

src/main/java/com/google/devtools/build/lib/buildtool/OutputDirectoryLinksUtils.java

@@ -169,7 +169,9 @@ static ImmutableList<ConvenienceSymlink> createOutputDirectoryLinks(
      eventHandler.handle(
          Event.warn(
              String.format(
-                  "cleared convenience symlink(s) %s because their destinations would be ambiguous",
+                  "cleared convenience symlink(s) %s because they wouldn't contain "


I like that this is more detailed now, but I could see this message being intimidating for the end user who is usually not the author of the self transition. I have a hard time coming up with something simpler though that would still be precise.

I agree. I hope this is at least a signal to ask for expert help with a clearly diagnosable message. But yes end-user friendly messages would always be great.

This is probably getting off-topic but I like the error pattern idea of a super-concise non-intimidating message with a link to a help page with more context. That page can contextualize as much as needed and distinguish user vs. expert content and not be so overwhelming at the CLI.

fmeum · 2023-07-07T10:00:09Z

@bazel-io flag

fmeum · 2023-07-07T10:00:46Z

We can probably afford to wait for the tests to become open-source and cherry-pick this into 6.3.0 next week, after the first rc.

aranguyen

LGTM

gregestren · 2023-07-07T15:53:16Z

Latest CI for https://bazel-review.googlesource.com/c/bazel/+/224150 is totally green: https://buildkite.com/bazel/google-bazel-presubmit/builds/69185.

In support of #18854. PiperOrigin-RevId: 546346128 Change-Id: Ie3f6976b74cba351573a7f35b0d09391da10fb55

Old semantics: New semantics: Fixes bazelbuild#17081. PiperOrigin-RevId: 546025529 Change-Id: I095564bb9233eba4b008df7f9e6aedae3e973b23

gregestren · 2023-07-07T21:25:34Z

Rebased to open-sourced ConvenienceSymlinkTest and adjusted tests. This is now ready to merge.

iancha1992 · 2023-07-07T23:21:56Z

@bazel-io fork 6.3.0

iancha1992 · 2023-07-10T21:32:15Z

@gregestren @aranguyen @fmeum We're trying to cherry-pick this change for release-6.3.0. However, looks like the file, "src/test/java/com/google/devtools/build/lib/buildtool/ConvenienceSymlinkTest.java" is missing in the release branch currently, which is causing the conflicts. Can you please take a look and let us know which commit is missing?

cc: @bazelbuild/triage

gregestren · 2023-07-10T21:34:04Z

You'll need 8781e5c.

Can you merge that?

In support of bazelbuild#18854. PiperOrigin-RevId: 546346128 Change-Id: Ie3f6976b74cba351573a7f35b0d09391da10fb55

Adjust `--top_level_targets_for_symlinks`. - If all targets in `$ bazel build //...` have the top-level config, `bazel-bin`, etc. point to that config - If all targets in `$ bazel build //...` have the same transitioned config, `bazel-bin`, etc. point to that config - If targets in `$ bazel build //...` have mixed configs, `bazel-bin`, etc. are deleted - If all targets in `$ bazel build //...` have the top-level config, `bazel-bin`, etc. point to that config - If all targets in `$ bazel build //...` have the same transitioned config, `bazel-bin`, etc. point to that config - If targets in `$ bazel build //...` have mixed configs and at least one of them is the top-level config, `bazel-bin`, etc. point to the top-level config - If targets in `$ bazel build //...` have mixed configs and none are the top-level config, `bazel-bin`, etc. are deleted Fixes bazelbuild#17081. Closes bazelbuild#18854. PiperOrigin-RevId: 546938509 Change-Id: I75adf0b8c2094522125c5e65d8c450eb2436d392

In support of bazelbuild#18854. PiperOrigin-RevId: 546346128 Change-Id: Ie3f6976b74cba351573a7f35b0d09391da10fb55

Adjust `--top_level_targets_for_symlinks`. - If all targets in `$ bazel build //...` have the top-level config, `bazel-bin`, etc. point to that config - If all targets in `$ bazel build //...` have the same transitioned config, `bazel-bin`, etc. point to that config - If targets in `$ bazel build //...` have mixed configs, `bazel-bin`, etc. are deleted - If all targets in `$ bazel build //...` have the top-level config, `bazel-bin`, etc. point to that config - If all targets in `$ bazel build //...` have the same transitioned config, `bazel-bin`, etc. point to that config - If targets in `$ bazel build //...` have mixed configs and at least one of them is the top-level config, `bazel-bin`, etc. point to the top-level config - If targets in `$ bazel build //...` have mixed configs and none are the top-level config, `bazel-bin`, etc. are deleted Fixes bazelbuild#17081. Closes bazelbuild#18854. PiperOrigin-RevId: 546938509 Change-Id: I75adf0b8c2094522125c5e65d8c450eb2436d392

In support of bazelbuild#18854. PiperOrigin-RevId: 546346128 Change-Id: Ie3f6976b74cba351573a7f35b0d09391da10fb55

In support of #18854. PiperOrigin-RevId: 546346128 Change-Id: Ie3f6976b74cba351573a7f35b0d09391da10fb55 Co-authored-by: Googler <gregce@google.com>

Adjust `--top_level_targets_for_symlinks`. - If all targets in `$ bazel build //...` have the top-level config, `bazel-bin`, etc. point to that config - If all targets in `$ bazel build //...` have the same transitioned config, `bazel-bin`, etc. point to that config - If targets in `$ bazel build //...` have mixed configs, `bazel-bin`, etc. are deleted - If all targets in `$ bazel build //...` have the top-level config, `bazel-bin`, etc. point to that config - If all targets in `$ bazel build //...` have the same transitioned config, `bazel-bin`, etc. point to that config - If targets in `$ bazel build //...` have mixed configs and at least one of them is the top-level config, `bazel-bin`, etc. point to the top-level config - If targets in `$ bazel build //...` have mixed configs and none are the top-level config, `bazel-bin`, etc. are deleted Fixes bazelbuild#17081. Closes bazelbuild#18854. PiperOrigin-RevId: 546938509 Change-Id: I75adf0b8c2094522125c5e65d8c450eb2436d392

iancha1992 · 2023-07-11T20:26:13Z

Done, thanks. Now it is cherry-pickable. @gregestren

Adjust `--top_level_targets_for_symlinks`. - If all targets in `$ bazel build //...` have the top-level config, `bazel-bin`, etc. point to that config - If all targets in `$ bazel build //...` have the same transitioned config, `bazel-bin`, etc. point to that config - If targets in `$ bazel build //...` have mixed configs, `bazel-bin`, etc. are deleted - If all targets in `$ bazel build //...` have the top-level config, `bazel-bin`, etc. point to that config - If all targets in `$ bazel build //...` have the same transitioned config, `bazel-bin`, etc. point to that config - If targets in `$ bazel build //...` have mixed configs and at least one of them is the top-level config, `bazel-bin`, etc. point to the top-level config - If targets in `$ bazel build //...` have mixed configs and none are the top-level config, `bazel-bin`, etc. are deleted Fixes bazelbuild#17081. Closes bazelbuild#18854. PiperOrigin-RevId: 546938509 Change-Id: I75adf0b8c2094522125c5e65d8c450eb2436d392

* Adjust --top_level_targets_for_symlinks Adjust `--top_level_targets_for_symlinks`. - If all targets in `$ bazel build //...` have the top-level config, `bazel-bin`, etc. point to that config - If all targets in `$ bazel build //...` have the same transitioned config, `bazel-bin`, etc. point to that config - If targets in `$ bazel build //...` have mixed configs, `bazel-bin`, etc. are deleted - If all targets in `$ bazel build //...` have the top-level config, `bazel-bin`, etc. point to that config - If all targets in `$ bazel build //...` have the same transitioned config, `bazel-bin`, etc. point to that config - If targets in `$ bazel build //...` have mixed configs and at least one of them is the top-level config, `bazel-bin`, etc. point to the top-level config - If targets in `$ bazel build //...` have mixed configs and none are the top-level config, `bazel-bin`, etc. are deleted Fixes #17081. Closes #18854. PiperOrigin-RevId: 546938509 Change-Id: I75adf0b8c2094522125c5e65d8c450eb2436d392 * Merge with 6.3.0 baseline. Particularly manually merge relevant signature changes from 1cd3588. Thsi is different than 1cd3588 itself because that also uses a baseline too new for 6.3.0.

github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Jul 6, 2023

gregestren added team-Configurability platforms, toolchains, cquery, select(), config transitions and removed awaiting-review PR is awaiting review from an assigned reviewer labels Jul 6, 2023

gregestren commented Jul 6, 2023

View reviewed changes

gregestren requested review from fmeum and aranguyen July 6, 2023 21:51

gregestren changed the title ~~Adjust --top_level_targets_for_symlinks.~~ Adjust --top_level_targets_for_symlinks Jul 6, 2023

fmeum reviewed Jul 7, 2023

View reviewed changes

bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jul 7, 2023

fmeum approved these changes Jul 7, 2023

View reviewed changes

aranguyen approved these changes Jul 7, 2023

View reviewed changes

aranguyen added the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Jul 7, 2023

keith mentioned this pull request Jul 7, 2023

Symlink targets to bazel-bin even with multiple transitions #12389

Open

copybara-service bot pushed a commit that referenced this pull request Jul 7, 2023

Open-source Google test ConvenienceSymlinkTest

8781e5c

In support of #18854. PiperOrigin-RevId: 546346128 Change-Id: Ie3f6976b74cba351573a7f35b0d09391da10fb55

gregestren and others added 4 commits July 7, 2023 17:19

Adjust --top_level_targets_for_symlinks.

8d0daaa

Old semantics: New semantics: Fixes bazelbuild#17081. PiperOrigin-RevId: 546025529 Change-Id: I095564bb9233eba4b008df7f9e6aedae3e973b23

Fix message typo.

38a25d1

Compare config output directories, not the configs themselves.

dbecfb5

Remove merge markers

71a02bf

gregestren force-pushed the change-224151 branch from 9169620 to 71a02bf Compare July 7, 2023 21:21

Update tests.

d1b66b0

bazel-io mentioned this pull request Jul 7, 2023

[6.3.0] Adjust --top_level_targets_for_symlinks #18862

Closed

bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jul 7, 2023

copybara-service bot closed this in ceb9955 Jul 10, 2023

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

gregestren deleted the change-224151 branch July 10, 2023 21:32

iancha1992 pushed a commit to iancha1992/bazel that referenced this pull request Jul 10, 2023

Open-source Google test ConvenienceSymlinkTest

a3b4218

In support of bazelbuild#18854. PiperOrigin-RevId: 546346128 Change-Id: Ie3f6976b74cba351573a7f35b0d09391da10fb55

iancha1992 pushed a commit to iancha1992/bazel that referenced this pull request Jul 10, 2023

Open-source Google test ConvenienceSymlinkTest

0172720

In support of bazelbuild#18854. PiperOrigin-RevId: 546346128 Change-Id: Ie3f6976b74cba351573a7f35b0d09391da10fb55

iancha1992 pushed a commit to iancha1992/bazel that referenced this pull request Jul 10, 2023

Open-source Google test ConvenienceSymlinkTest

1ccbe65

In support of bazelbuild#18854. PiperOrigin-RevId: 546346128 Change-Id: Ie3f6976b74cba351573a7f35b0d09391da10fb55

iancha1992 mentioned this pull request Jul 10, 2023

[6.3.0] Open-source Google test ConvenienceSymlinkTest #18890

Merged

iancha1992 added a commit that referenced this pull request Jul 11, 2023

Open-source Google test ConvenienceSymlinkTest (#18890)

70c2056

In support of #18854. PiperOrigin-RevId: 546346128 Change-Id: Ie3f6976b74cba351573a7f35b0d09391da10fb55 Co-authored-by: Googler <gregce@google.com>

iancha1992 mentioned this pull request Jul 11, 2023

[6.3.0] Adjust --top_level_targets_for_symlinks #18906

Closed

gregestren mentioned this pull request Jul 12, 2023

[6.3.0] Adjust --top_level_targets_for_symlinks #18916

Merged

gregestren mentioned this pull request Nov 28, 2023

Add cfg = _test_arg_transition in configurations/cc_test/defs.bzl bazelbuild/examples#367

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust --top_level_targets_for_symlinks #18854

Adjust --top_level_targets_for_symlinks #18854

gregestren commented Jul 6, 2023 •

edited

Loading

gregestren Jul 6, 2023

fmeum Jul 7, 2023

gregestren Jul 7, 2023

fmeum Jul 7, 2023

gregestren commented Jul 6, 2023

gregestren commented Jul 6, 2023

fmeum Jul 7, 2023

gregestren Jul 7, 2023

fmeum commented Jul 7, 2023

fmeum commented Jul 7, 2023

aranguyen left a comment

gregestren commented Jul 7, 2023

gregestren commented Jul 7, 2023

iancha1992 commented Jul 7, 2023

iancha1992 commented Jul 10, 2023

gregestren commented Jul 10, 2023

iancha1992 commented Jul 11, 2023

Adjust --top_level_targets_for_symlinks #18854

Adjust --top_level_targets_for_symlinks #18854

Conversation

gregestren commented Jul 6, 2023 • edited Loading

Old semantics:

New semantics:

gregestren Jul 6, 2023

Choose a reason for hiding this comment

fmeum Jul 7, 2023

Choose a reason for hiding this comment

gregestren Jul 7, 2023

Choose a reason for hiding this comment

fmeum Jul 7, 2023

Choose a reason for hiding this comment

gregestren commented Jul 6, 2023

gregestren commented Jul 6, 2023

fmeum Jul 7, 2023

Choose a reason for hiding this comment

gregestren Jul 7, 2023

Choose a reason for hiding this comment

fmeum commented Jul 7, 2023

fmeum commented Jul 7, 2023

aranguyen left a comment

Choose a reason for hiding this comment

gregestren commented Jul 7, 2023

gregestren commented Jul 7, 2023

iancha1992 commented Jul 7, 2023

iancha1992 commented Jul 10, 2023

gregestren commented Jul 10, 2023

iancha1992 commented Jul 11, 2023

gregestren commented Jul 6, 2023 •

edited

Loading