Tighten up ConfiguredTargetKey API by adding fromConfiguredTarget

Constructing a correct ConfiguredTargetKey is perilous as the provided configuration keymust be the post-transition configuration (including self-transitions and trimming) or else issues will arise. This is part of an effort to audit all creations of a ConfiguredTargetKey to make sure they are compliant with the aforementioned contract. In this case, a ConfiguredTarget already exists so it is very easy to construct a ConfiguredTargetKey to get it. The new API returns the key already built because that is what all the callsites want (and assures the configuration can't be improperly changed). PiperOrigin-RevId: 462271525 Change-Id: Id2d3245a618d1f687d909025c897b1ac34369620
bazelbuild · Jul 21, 2022 · 24f39f3 · 24f39f3
1 parent e9f5b40
commit 24f39f3
Show file tree

Hide file tree

Showing 10 changed files with 24 additions and 75 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/TargetCompleteEvent.java b/src/main/java/com/google/devtools/build/lib/analysis/TargetCompleteEvent.java
@@ -146,10 +146,7 @@ private TargetCompleteEvent(
     this.label = targetAndData.getConfiguredTarget().getLabel();
     this.aliasLabel = targetAndData.getConfiguredTarget().getOriginalLabel();
     this.configuredTargetKey =
-        ConfiguredTargetKey.builder()
-            .setConfiguredTarget(targetAndData.getConfiguredTarget())
-            .setConfiguration(targetAndData.getConfiguration())
-            .build();
+        ConfiguredTargetKey.fromConfiguredTarget(targetAndData.getConfiguredTarget());
     postedAfterBuilder.add(BuildEventIdUtil.targetConfigured(aliasLabel));
     DetailedExitCode mostImportantDetailedExitCode = null;
     for (Cause cause : getRootCauses().toList()) {

diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildResultPrinter.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildResultPrinter.java
@@ -119,7 +119,7 @@ void showBuildResult(
           skipped.add(target);
         } else if (successfulTargets.contains(target)
             && validated.getOrDefault(
-                ConfiguredTargetKey.builder().setConfiguredTarget(target).build(), Boolean.TRUE)) {
+                ConfiguredTargetKey.fromConfiguredTarget(target), Boolean.TRUE)) {
           succeeded.add(target);
         } else {
           failed.add(target);

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
@@ -835,11 +835,7 @@ static ImmutableSet<ConfiguredTarget> determineSuccessfulTargets(
     // iteration order as configuredTargets.
     ImmutableSet.Builder<ConfiguredTarget> successfulTargets = ImmutableSet.builder();
     for (ConfiguredTarget target : configuredTargets) {
-      if (builtTargets.contains(
-          ConfiguredTargetKey.builder()
-              .setConfiguredTarget(target)
-              .setConfigurationKey(target.getConfigurationKey())
-              .build())) {
+      if (builtTargets.contains(ConfiguredTargetKey.fromConfiguredTarget(target))) {
         successfulTargets.add(target);
       }
     }

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKey.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKey.java
@@ -200,6 +200,14 @@ public static Builder builder() {
     return new Builder();
   }
 
+  /** Returns a new {@link ConfiguredTargetKey}. */
+  public static ConfiguredTargetKey fromConfiguredTarget(ConfiguredTarget configuredTarget) {
+    return builder()
+        .setLabel(configuredTarget.getOriginalLabel())
+        .setConfigurationKey(configuredTarget.getConfigurationKey())
+        .build();
+  }
+
   /** A helper class to create instances of {@link ConfiguredTargetKey}. */
   public static final class Builder {
     private Label label = null;
@@ -215,21 +223,8 @@ public Builder setLabel(Label label) {
       return this;
     }
 
-    /**
-     * Sets the {@link ConfiguredTarget} that we want a key for.
-     *
-     * <p>This sets both the label and configurationKey data.
-     */
-    @CanIgnoreReturnValue
-    public Builder setConfiguredTarget(ConfiguredTarget configuredTarget) {
-      setLabel(configuredTarget.getOriginalLabel());
-      if (this.configurationKey == null) {
-        setConfigurationKey(configuredTarget.getConfigurationKey());
-      }
-      return this;
-    }
-
     /** Sets the {@link BuildConfigurationValue} for the configured target. */
+    @CanIgnoreReturnValue
     public Builder setConfiguration(@Nullable BuildConfigurationValue buildConfiguration) {
       return setConfigurationKey(buildConfiguration == null ? null : buildConfiguration.getKey());
     }

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetCompletionValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetCompletionValue.java
@@ -44,12 +44,7 @@ public static Iterable<TargetCompletionKey> keys(
         targets,
         ct ->
             TargetCompletionKey.create(
-                ConfiguredTargetKey.builder()
-                    .setConfiguredTarget(ct)
-                    .setConfigurationKey(ct.getConfigurationKey())
-                    .build(),
-                ctx,
-                targetsToTest.contains(ct)));
+                ConfiguredTargetKey.fromConfiguredTarget(ct), ctx, targetsToTest.contains(ct)));
   }
 
   /** {@link com.google.devtools.build.skyframe.SkyKey} for {@link TargetCompletionValue}. */

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionValue.java
@@ -49,10 +49,7 @@ public static Iterable<SkyKey> keys(
         targets,
         ct ->
             TestCompletionKey.create(
-                ConfiguredTargetKey.builder()
-                    .setConfiguredTarget(ct)
-                    .setConfigurationKey(ct.getConfigurationKey())
-                    .build(),
+                ConfiguredTargetKey.fromConfiguredTarget(ct),
                 topLevelArtifactContext,
                 exclusiveTesting));
   }

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
@@ -601,11 +601,7 @@ protected Artifact getBinArtifact(String packageRelativePath, ConfiguredTarget o
         .getDerivedArtifact(
             label.getPackageFragment().getRelative(packageRelativePath),
             getTargetConfiguration().getBinDirectory(label.getRepository()),
-            ConfiguredTargetKey.builder()
-                .setConfiguredTarget(owner)
-                .setConfiguration(
-                    skyframeExecutor.getConfiguration(reporter, owner.getConfigurationKey()))
-                .build());
+            ConfiguredTargetKey.fromConfiguredTarget(owner));
   }
 
   protected Set<ActionLookupKey> getSkyframeEvaluatedTargetKeys() {

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
@@ -304,8 +304,7 @@ public Collection<ConfiguredTarget> getDirectPrerequisitesForTesting(
 
       // Load the keys of the dependencies of the target, based on data currently in skyframe.
       Iterable<SkyKey> directPrerequisites =
-          walkableGraph.getDirectDeps(
-              ConfiguredTargetKey.builder().setConfiguredTarget(ct).build());
+          walkableGraph.getDirectDeps(ConfiguredTargetKey.fromConfiguredTarget(ct));
 
       // Turn the keys back into ConfiguredTarget instances, possibly merging in aspects that were
       // propagated from the original target.
@@ -663,11 +662,7 @@ public RuleContext getRuleContextForTesting(
         .setHostConfiguration(configurations.getHostConfiguration())
         .setConfigurationFragmentPolicy(
             target.getAssociatedRule().getRuleClassObject().getConfigurationFragmentPolicy())
-        .setActionOwnerSymbol(
-            ConfiguredTargetKey.builder()
-                .setConfiguredTarget(configuredTarget)
-                .setConfigurationKey(configuredTarget.getConfigurationKey())
-                .build())
+        .setActionOwnerSymbol(ConfiguredTargetKey.fromConfiguredTarget(configuredTarget))
         .setMutability(Mutability.create("configured target"))
         .setVisibility(
             NestedSetBuilder.create(

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
@@ -1456,11 +1456,7 @@ protected final Artifact.DerivedArtifact getDerivedArtifact(
    * "foo.o".
    */
   protected final Artifact getTreeArtifact(String packageRelativePath, ConfiguredTarget owner) {
-    ActionLookupKey actionLookupKey =
-        ConfiguredTargetKey.builder()
-            .setConfiguredTarget(owner)
-            .setConfigurationKey(owner.getConfigurationKey())
-            .build();
+    ActionLookupKey actionLookupKey = ConfiguredTargetKey.fromConfiguredTarget(owner);
     return getDerivedArtifact(
         owner.getLabel().getPackageFragment().getRelative(packageRelativePath),
         getConfiguration(owner).getBinDirectory(RepositoryName.MAIN),
@@ -1517,11 +1513,7 @@ protected final Artifact getBinArtifact(String packageRelativePath, ConfiguredTa
     return getPackageRelativeDerivedArtifact(
         packageRelativePath,
         getConfiguration(owner).getBinDirectory(RepositoryName.MAIN),
-        ConfiguredTargetKey.builder()
-            .setConfiguredTarget(owner)
-            .setConfiguration(
-                skyframeExecutor.getConfiguration(reporter, owner.getConfigurationKey()))
-            .build());
+        ConfiguredTargetKey.fromConfiguredTarget(owner));
   }
 
   /**
@@ -1602,13 +1594,9 @@ protected Artifact getGenfilesArtifact(String packageRelativePath, String owner)
    * be "foo.o".
    */
   protected Artifact getGenfilesArtifact(String packageRelativePath, ConfiguredTarget owner) {
+    ConfiguredTargetKey configKey = ConfiguredTargetKey.fromConfiguredTarget(owner);
     BuildConfigurationValue configuration =
-        skyframeExecutor.getConfiguration(reporter, owner.getConfigurationKey());
-    ConfiguredTargetKey configKey =
-        ConfiguredTargetKey.builder()
-            .setConfiguredTarget(owner)
-            .setConfiguration(configuration)
-            .build();
+        skyframeExecutor.getConfiguration(reporter, configKey.getConfigurationKey());
     return getGenfilesArtifact(packageRelativePath, configKey, configuration);
   }
 
@@ -1693,11 +1681,7 @@ protected Artifact getSharedArtifact(String rootRelativePath, ConfiguredTarget o
     return getDerivedArtifact(
         PathFragment.create(rootRelativePath),
         targetConfig.getBinDirectory(RepositoryName.MAIN),
-        ConfiguredTargetKey.builder()
-            .setConfiguredTarget(owner)
-            .setConfiguration(
-                skyframeExecutor.getConfiguration(reporter, owner.getConfigurationKey()))
-            .build());
+        ConfiguredTargetKey.fromConfiguredTarget(owner));
   }
 
   protected Action getGeneratingActionForLabel(String label) throws Exception {

diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java
@@ -691,10 +691,7 @@ public void testNoActionConflictWithInvalidatedTarget() throws Exception {
             getDerivedArtifact(
                 PathFragment.create("conflict/_objs/x/foo.o"),
                 root,
-                ConfiguredTargetKey.builder()
-                    .setConfiguredTarget(conflict.getConfiguredTarget())
-                    .setConfiguration(conflict.getConfiguration())
-                    .build()));
+                ConfiguredTargetKey.fromConfiguredTarget(conflict.getConfiguredTarget())));
     assertThat(oldAction.getOwner().getLabel().toString()).isEqualTo("//conflict:x");
     skyframeExecutor.handleAnalysisInvalidatingChange();
     ConfiguredTargetAndData objsConflict =
@@ -706,10 +703,7 @@ public void testNoActionConflictWithInvalidatedTarget() throws Exception {
             getDerivedArtifact(
                 PathFragment.create("conflict/_objs/x/foo.o"),
                 root,
-                ConfiguredTargetKey.builder()
-                    .setConfiguredTarget(objsConflict.getConfiguredTarget())
-                    .setConfiguration(objsConflict.getConfiguration())
-                    .build()));
+                ConfiguredTargetKey.fromConfiguredTarget(objsConflict.getConfiguredTarget())));
     assertThat(newAction.getOwner().getLabel().toString()).isEqualTo("//conflict:_objs/x/foo.o");
   }