Adjust --top_level_targets_for_symlinks

gregestren · copybara-github · commit ceb9955cc36e · 2023-07-10T11:51:37.000-07:00
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. Closes #18854. PiperOrigin-RevId: 546938509 Change-Id: I75adf0b8c2094522125c5e65d8c450eb2436d392
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java
@@ -766,16 +766,39 @@ private ImmutableList<ConvenienceSymlink> createConvenienceSymlinks(
     Reporter reporter = env.getReporter();
 
     // Gather configurations to consider.
-    Set<BuildConfigurationValue> targetConfigurations =
-        buildRequestOptions.useTopLevelTargetsForSymlinks() && !targetsToBuild.isEmpty()
-            ? targetsToBuild.stream()
-                .map(ConfiguredTarget::getActual)
-                .map(ConfiguredTarget::getConfigurationKey)
-                .filter(Objects::nonNull)
-                .distinct()
-                .map((key) -> executor.getConfiguration(reporter, key))
-                .collect(toImmutableSet())
-            : ImmutableSet.of(configuration);
+    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 =
@@ -787,7 +810,7 @@ private ImmutableList<ConvenienceSymlink> createConvenienceSymlinks(
           env.getWorkspace(),
           env.getDirectories(),
           getReporter(),
-          targetConfigurations,
+          targetConfigs,
           options -> getConfiguration(executor, reporter, options),
           productName);
     }
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/OutputDirectoryLinksUtils.java b/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();
diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/ConvenienceSymlinkTest.java b/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(
