Skip to content

Commit

Permalink
Fix cache leak when applying transitions when only a rule's attribute…
Browse files Browse the repository at this point in the history
…s change.

Fixes #13997

PiperOrigin-RevId: 406886706
  • Loading branch information
gregestren authored and copybara-github committed Nov 1, 2021
1 parent 216b2b6 commit fe644be
Show file tree
Hide file tree
Showing 2 changed files with 70 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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");
}
}

0 comments on commit fe644be

Please sign in to comment.