Fix cache leak when applying transitions when only a rule's attribute…

…s change. Fixes bazelbuild#13997 PiperOrigin-RevId: 406886706
brentleyjones · Nov 1, 2021 · fe644be · fe644be
1 parent 216b2b6
commit fe644be
Show file tree

Hide file tree

Showing 2 changed files with 70 additions and 5 deletions.
diff --git a/.../java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleTransitionProvider.java b/.../java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleTransitionProvider.java
@@ -25,7 +25,6 @@
 import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory;
-import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.Attribute;
@@ -71,13 +70,13 @@ public StarlarkDefinedConfigTransition getStarlarkDefinedConfigTransitionForTest
    */
   private static class CacheKey {
     private final StarlarkDefinedConfigTransition starlarkDefinedConfigTransition;
-    private final Label ruleLabel;
+    private final Rule rule;
     private final int hashCode;
 
     CacheKey(StarlarkDefinedConfigTransition starlarkDefinedConfigTransition, Rule rule) {
       this.starlarkDefinedConfigTransition = starlarkDefinedConfigTransition;
-      this.ruleLabel = rule.getLabel();
-      this.hashCode = Objects.hash(starlarkDefinedConfigTransition, ruleLabel);
+      this.rule = rule;
+      this.hashCode = Objects.hash(starlarkDefinedConfigTransition, rule);
     }
 
     @Override
@@ -90,7 +89,7 @@ public boolean equals(Object other) {
       }
       return (this.starlarkDefinedConfigTransition.equals(
               ((CacheKey) other).starlarkDefinedConfigTransition)
-          && this.ruleLabel.equals(((CacheKey) other).ruleLabel));
+          && this.rule.equals(((CacheKey) other).rule));
     }
 
     @Override

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java
@@ -30,6 +30,9 @@
 import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
 import com.google.devtools.build.lib.testutil.TestConstants;
 import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
+import com.google.devtools.build.lib.vfs.ModifiedFileSet;
+import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.Root;
 import com.google.testing.junit.testparameterinjector.TestParameterInjector;
 import com.google.testing.junit.testparameterinjector.TestParameters;
 import java.util.List;
@@ -1692,4 +1695,67 @@ public void testNoPlatformChange() throws Exception {
                 .platforms)
         .containsExactly(Label.parseAbsoluteUnchecked("//platforms:my_platform"));
   }
+
+  @Test
+  public void testTransitionsStillTriggerWhenOnlyRuleAttributesChange() throws Exception {
+    scratch.file(
+        "test/defs.bzl",
+        "def _transition_impl(settings, attr):",
+        "  return {",
+        "    '//command_line_option:foo': attr.my_attr,",
+        "  }",
+        "_my_transition = transition(",
+        "  implementation = _transition_impl,",
+        "  inputs = [],",
+        "  outputs = [",
+        "    '//command_line_option:foo',",
+        "  ]",
+        ")",
+        "def _rule_impl(ctx):",
+        "  return []",
+        "my_rule = rule(",
+        "  implementation = _rule_impl,",
+        "  cfg = _my_transition,",
+        "  attrs = {",
+        "    'my_attr': attr.string(),",
+        "    '_allowlist_function_transition': attr.label(",
+        "        default = '//tools/allowlists/function_transition_allowlist',",
+        "    ),",
+        "  },",
+        ")");
+    writeAllowlistFile();
+
+    scratch.file(
+        "test/BUILD",
+        "load('//test:defs.bzl', 'my_rule')",
+        "my_rule(",
+        "  name = 'buildme',",
+        "  my_attr = 'first build',",
+        ")");
+    assertThat(
+            getConfiguration(getConfiguredTarget("//test:buildme"))
+                .getOptions()
+                .get(DummyTestOptions.class)
+                .foo)
+        .isEqualTo("first build");
+
+    scratch.overwriteFile(
+        "test/BUILD",
+        "load('//test:defs.bzl', 'my_rule')",
+        "my_rule(",
+        "  name = 'buildme',",
+        "  my_attr = 'second build',",
+        ")");
+    skyframeExecutor.invalidateFilesUnderPathForTesting(
+        reporter,
+        ModifiedFileSet.builder().modify(PathFragment.create("test/BUILD")).build(),
+        Root.fromPath(rootDirectory));
+
+    assertThat(
+            getConfiguration(getConfiguredTarget("//test:buildme"))
+                .getOptions()
+                .get(DummyTestOptions.class)
+                .foo)
+        .isEqualTo("second build");
+  }
 }