[6.3.0] Adjust --top_level_targets_for_symlinks (#18916)

* 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.

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

@@ -707,7 +707,10 @@ public void handleConvenienceSymlinks(AnalysisResult analysisResult) {
       ImmutableList<ConvenienceSymlink> convenienceSymlinks = ImmutableList.of();
       if (request.getBuildOptions().experimentalConvenienceSymlinks
           != ConvenienceSymlinksMode.IGNORE) {
-        convenienceSymlinks = createConvenienceSymlinks(request.getBuildOptions(), analysisResult);
+        convenienceSymlinks = createConvenienceSymlinks(
+            request.getBuildOptions(),
+            analysisResult.getTargetsToBuild(),
+            analysisResult.getConfigurationCollection().getTargetConfiguration());
       }
       if (request.getBuildOptions().experimentalConvenienceSymlinksBepEvent) {
         env.getEventBus().post(new ConvenienceSymlinksIdentifiedEvent(convenienceSymlinks));
@@ -729,22 +732,46 @@ public void handleConvenienceSymlinks(AnalysisResult analysisResult) {
    * in fact gets removed if it was already present from a previous invocation.
    */
   private ImmutableList<ConvenienceSymlink> createConvenienceSymlinks(
-      BuildRequestOptions buildRequestOptions, AnalysisResult analysisResult) {
+      BuildRequestOptions buildRequestOptions,
+      ImmutableSet<ConfiguredTarget> targetsToBuild,
+      BuildConfigurationValue configuration) {
     SkyframeExecutor executor = env.getSkyframeExecutor();
     Reporter reporter = env.getReporter();
 
     // Gather configurations to consider.
-    Set<BuildConfigurationValue> targetConfigurations =
-        buildRequestOptions.useTopLevelTargetsForSymlinks()
-                && !analysisResult.getTargetsToBuild().isEmpty()
-            ? analysisResult.getTargetsToBuild().stream()
-                .map(ConfiguredTarget::getActual)
-                .map(ConfiguredTarget::getConfigurationKey)
-                .filter(Objects::nonNull)
-                .distinct()
-                .map((key) -> executor.getConfiguration(reporter, key))
-                .collect(toImmutableSet())
-            : ImmutableSet.of(analysisResult.getConfigurationCollection().getTargetConfiguration());
+    ImmutableSet<BuildConfigurationValue> targetConfigs;
+    if (buildRequestOptions.useTopLevelTargetsForSymlinks() && !targetsToBuild.isEmpty()) {
+      // Collect the configuration of each top-level requested target. These may be different than
+      // the build's top-level configuration because of self-transitions.
+      ImmutableSet<BuildConfigurationValue> requestedTargetConfigs =
+          targetsToBuild.stream()
+              .map(ConfiguredTarget::getActual)
+              .map(ConfiguredTarget::getConfigurationKey)
+              .filter(Objects::nonNull)
+              .distinct()
+              .map((key) -> executor.getConfiguration(reporter, key))
+              .collect(toImmutableSet());
+      if (requestedTargetConfigs.size() == 1) {
+        // All top-level targets have the same configuration, so use that one.
+        targetConfigs = requestedTargetConfigs;
+      } else if (requestedTargetConfigs.stream()
+          .anyMatch(
+              c -> c.getOutputDirectoryName().equals(configuration.getOutputDirectoryName()))) {
+        // Mixed configs but at least one matches the top-level config's output path (this doesn't
+        // mean it's the same as the top-level config: --trim_test_configuration means non-test
+        // targets use the default output path but lack the top-level config's TestOptions). Set
+        // symlinks to the top-level config so at least non-transitioned targets resolve. See
+        // 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
+        // they won't contain any relevant data. This is handled in the
+        // createOutputDirectorySymlinks call below.
+        targetConfigs = requestedTargetConfigs;
+      }
+    } else {
+      targetConfigs = ImmutableSet.of(configuration);
+    }
 
     String productName = runtime.getProductName();
     try (SilentCloseable c =
@@ -756,7 +783,7 @@ private ImmutableList<ConvenienceSymlink> createConvenienceSymlinks(
           env.getWorkspace(),
           env.getDirectories(),
           getReporter(),
-          targetConfigurations,
+          targetConfigs,
           options -> getConfiguration(executor, reporter, options),
           productName);
     }

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 "
+                      + "requested targets' outputs. Those targets self-transition to multiple "
+                      + "distinct configurations",
                   Joiner.on(", ").join(ambiguousLinks))));
     }
     return convenienceSymlinksBuilder.build();

src/test/java/com/google/devtools/build/lib/buildtool/ConvenienceSymlinkTest.java

@@ -417,7 +417,8 @@ public void buildingOnlyTargetsWithNullConfigurations_unsetsSymlinks() throws Ex
   }
 
   @Test
-  public void buildingTargetsWithDifferentOutputDirectories_unsetsSymlinks() throws Exception {
+  public void buildingTargetsWithDifferentOutputDirectories_unsetsSymlinksIfNoneAreTopLevel()
+      throws Exception {
     addOptions("--symlink_prefix=ambiguous-", "--incompatible_skip_genfiles_symlink=false");
 
     Path config = getOutputPath().getRelative("some-imaginary-config");
@@ -431,10 +432,9 @@ public void buildingTargetsWithDifferentOutputDirectories_unsetsSymlinks() throw
 
     write(
         "targets/BUILD",
-        "basic_rule(name='default')",
         "incoming_transition_rule(name='config1', path='set_from_config1')",
         "incoming_transition_rule(name='config2', path='set_from_config2')");
-    buildTarget("//targets:default", "//targets:config1", "//targets:config2");
+    buildTarget("//targets:config1", "//targets:config2");
 
     // there should be nothing at any of the convenience symlinks which depend on configuration -
     // the symlinks put there during the simulated prior build should have been deleted
@@ -454,6 +454,49 @@ public void buildingTargetsWithDifferentOutputDirectories_unsetsSymlinks() throw
             getOutputPath());
   }
 
+  @Test
+  public void buildingTargetsWithDifferentOutputDirectories_setsSymlinksIfAnyAreTopLevel()
+      throws Exception {
+    addOptions(
+        "--symlink_prefix=ambiguous-",
+        "--incompatible_skip_genfiles_symlink=false",
+        "--incompatible_merge_genfiles_directory=false",
+        "--incompatible_skip_genfiles_symlink=false");
+
+    Path config = getOutputPath().getRelative("some-imaginary-config");
+    // put symlinks at the convenience symlinks spots to simulate a prior build
+    Path binLink = getWorkspace().getChild("ambiguous-bin");
+    binLink.createSymbolicLink(config.getChild("bin"));
+    Path genfilesLink = getWorkspace().getChild("ambiguous-genfiles");
+    genfilesLink.createSymbolicLink(config.getChild("genfiles"));
+    Path testlogsLink = getWorkspace().getChild("ambiguous-testlogs");
+    testlogsLink.createSymbolicLink(config.getChild("testlogs"));
+
+    write(
+        "targets/BUILD",
+        "basic_rule(name='default')",
+        "incoming_transition_rule(name='config1', path='set_from_config1')");
+    buildTarget("//targets:default", "//targets:config1");
+
+    assertThat(getConvenienceSymlinks())
+        .containsExactly(
+            "ambiguous-bin",
+            getOutputPath()
+                .getRelative("default-" + getTargetConfiguration().getCpu() + "-fastbuild/bin"),
+            "ambiguous-genfiles",
+            getOutputPath()
+                .getRelative(
+                    "default-" + getTargetConfiguration().getCpu() + "-fastbuild/genfiles"),
+            "ambiguous-testlogs",
+            getOutputPath()
+                .getRelative(
+                    "default-" + getTargetConfiguration().getCpu() + "-fastbuild/testlogs"),
+            "ambiguous-" + TestConstants.WORKSPACE_NAME,
+            getExecRoot(),
+            "ambiguous-out",
+            getOutputPath());
+  }
+
   @Test
   public void buildingTargetsWithSameConfiguration_setsSymlinks() throws Exception {
     addOptions(