Properly account for StarlarkOptions at their default (=null) when calculating ST-hash
Previously, the hash calculation was skipping including StarlarkOptions that happened to be at their default values.
This is wrong since those values may still be in "affected by Starlark transition" (because either the commandline set them and the Starlark transition reset them to their Starlark defaults thus still requiring a hash change OR the commandline did not set them but a series of Starlark transitions did an default->B->default anyways causing the Starlark option to still be 'stuck' in "affected by Starlark transition").
Resolves https://github.com/bazelbuild/bazel/issues/14239
PiperOrigin-RevId: 408701552
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java
index b0ce7bc..13ebd99 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java
@@ -440,16 +440,19 @@
toHash.put(optionName, value);
} else {
Object value = toOptions.getStarlarkOptions().get(Label.parseAbsoluteUnchecked(optionName));
- if (value != null) {
- toHash.put(optionName, value);
- }
+ toHash.put(optionName, value);
}
}
ImmutableList.Builder<String> hashStrs = ImmutableList.builderWithExpectedSize(toHash.size());
for (Map.Entry<String, Object> singleOptionAndValue : toHash.entrySet()) {
- String toAdd = singleOptionAndValue.getKey() + "=" + singleOptionAndValue.getValue();
- hashStrs.add(toAdd);
+ Object value = singleOptionAndValue.getValue();
+ if (value != null) {
+ hashStrs.add(singleOptionAndValue.getKey() + "=" + value);
+ } else {
+ // Avoid using =null to different from value being the non-null String "null"
+ hashStrs.add(singleOptionAndValue.getKey() + "@null");
+ }
}
return transitionDirectoryNameFragment(hashStrs.build());
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java
index 4ba6086..fe1cb2f 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java
@@ -1182,6 +1182,81 @@
}
@Test
+ public void testTransitionBackToStarlarkDefaultOK() throws Exception {
+ writeAllowlistFile();
+ writeBuildSettingsBzl();
+ scratch.file(
+ "test/starlark/rules.bzl",
+ "load('//myinfo:myinfo.bzl', 'MyInfo')",
+ "def _transition_impl(settings, attr):",
+ " return {",
+ " '//test/starlark:the-answer': attr.answer_for_dep,",
+ " '//test/starlark:did-transition': 1,",
+ "}",
+ "my_transition = transition(",
+ " implementation = _transition_impl,",
+ " inputs = [],",
+ " outputs = ['//test/starlark:the-answer', '//test/starlark:did-transition']",
+ ")",
+ "def _rule_impl(ctx):",
+ " return MyInfo(dep = ctx.attr.dep)",
+ "my_rule = rule(",
+ " implementation = _rule_impl,",
+ " attrs = {",
+ " 'dep': attr.label(cfg = my_transition),",
+ " 'answer_for_dep': attr.int(),",
+ " '_allowlist_function_transition': attr.label(",
+ " default = '//tools/allowlists/function_transition_allowlist'),",
+ " }",
+ ")");
+ scratch.file(
+ "test/starlark/BUILD",
+ "load('//test/starlark:rules.bzl', 'my_rule')",
+ "load('//test/starlark:build_settings.bzl', 'int_flag')",
+ "my_rule(name = 'test', dep = ':dep1', answer_for_dep=0)",
+ "my_rule(name = 'dep1', dep = ':dep2', answer_for_dep=42)",
+ "my_rule(name = 'dep2', dep = ':dep3', answer_for_dep=0)",
+ "my_rule(name = 'dep3')",
+ "int_flag(name = 'the-answer', build_setting_default=0)",
+ "int_flag(name = 'did-transition', build_setting_default=0)");
+ useConfiguration(ImmutableMap.of(), "--cpu=FOO");
+
+ ConfiguredTarget test = getConfiguredTarget("//test/starlark:test");
+
+ // '//test/starlark:did-transition ensures ST-hash is 'turned on' since :test has no ST-hash
+ // and thus will trivially have a unique getTransitionDirectoryNameFragment
+
+ @SuppressWarnings("unchecked")
+ ConfiguredTarget dep1 =
+ Iterables.getOnlyElement(
+ (List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep"));
+
+ @SuppressWarnings("unchecked")
+ ConfiguredTarget dep2 =
+ Iterables.getOnlyElement(
+ (List<ConfiguredTarget>) getMyInfoFromTarget(dep1).getValue("dep"));
+
+ @SuppressWarnings("unchecked")
+ ConfiguredTarget dep3 =
+ Iterables.getOnlyElement(
+ (List<ConfiguredTarget>) getMyInfoFromTarget(dep2).getValue("dep"));
+
+ // These must be true
+ assertThat(getTransitionDirectoryNameFragment(dep1))
+ .isNotEqualTo(getTransitionDirectoryNameFragment(dep2));
+
+ assertThat(getTransitionDirectoryNameFragment(dep2))
+ .isNotEqualTo(getTransitionDirectoryNameFragment(dep3));
+
+ // TODO(blaze-configurability-team): When "affected by starlark transition" is gone,
+ // will be equal and thus getTransitionDirectoryNameFragment can be equal.
+ if (!getConfiguration(dep1).equals(getConfiguration(dep3))) {
+ assertThat(getTransitionDirectoryNameFragment(dep1))
+ .isNotEqualTo(getTransitionDirectoryNameFragment(dep3));
+ }
+ }
+
+ @Test
public void testTransitionOnBuildSetting_onlyTransitionsAffectsDirectory() throws Exception {
writeAllowlistFile();
writeBuildSettingsBzl();
