Same transition dirs when only cmd line options differs
Ignore user defined build settings only set on
command line and not affected by any transition,
when calculating transitionDirectoryNameFragment
output directory.
Native options on command line already have this
behavior, and this commit provides the same also
for user defined build settings.
This allows reducing transition scalability issues,
by combining transitions with command line options:
a) Setting a limited number of common options via
transitions, which affects large parts of the
code base, but with limited number of variants.
b) When needed, *also* setting a few of many
specific options, each of them affecting only
their parts of the code base, but with many
variants.
Resolves #12731
Closes #13580.
PiperOrigin-RevId: 396689438
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java
index bb77ae6..6a13252 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java
@@ -261,9 +261,11 @@
}
/**
- * This internal option is a *set* of names (e.g. "cpu") of *native* options that have been
- * changed by starlark transitions at any point in the build at the time of accessing. This is
- * used to regenerate {@code transitionDirectoryNameFragment} after each starlark transition.
+ * This internal option is a *set* of names of options that have been changed by starlark
+ * transitions at any point in the build at the time of accessing. It contains both native and
+ * starlark options in label form. e.g. "//command_line_option:cpu" for native options and
+ * "//myapp:foo" for starlark options. This is used to regenerate {@code
+ * transitionDirectoryNameFragment} after each starlark transition.
*/
@Option(
name = "affected by starlark transition",
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 72f3acc..4e4d2a1 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
@@ -19,6 +19,7 @@
import static java.util.stream.Collectors.joining;
import com.google.common.base.Joiner;
+import com.google.common.base.VerifyException;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
@@ -289,13 +290,13 @@
if (!optionName.startsWith(COMMAND_LINE_OPTION_PREFIX)) {
// The transition changes a Starlark option.
- Object oldValue =
- fromOptions.getStarlarkOptions().get(Label.parseAbsoluteUnchecked(optionName));
+ Label optionLabel = Label.parseAbsoluteUnchecked(optionName);
+ Object oldValue = fromOptions.getStarlarkOptions().get(optionLabel);
if ((oldValue == null && optionValue != null)
|| (oldValue != null && optionValue == null)
|| (oldValue != null && !oldValue.equals(optionValue))) {
- changedStarlarkOptions.put(Label.parseAbsoluteUnchecked(optionName), optionValue);
- convertedNewValues.add(optionName);
+ changedStarlarkOptions.put(optionLabel, optionValue);
+ convertedNewValues.add(optionLabel.toString());
}
} else {
// The transition changes a native option.
@@ -398,9 +399,10 @@
}
/**
- * Compute the output directory name fragment corresponding to the new BuildOptions based on (1)
- * the names and values of all native options previously transitioned anywhere in the build by
- * starlark transitions, (2) names and values of all entries in the starlark options map.
+ * Compute the output directory name fragment corresponding to the new BuildOptions based on the
+ * names and values of all options (both native and Starlark) previously transitioned anywhere in
+ * the build by Starlark transitions. Options only set on command line are not affecting the
+ * computation.
*
* @param changedOptions the names of all options changed by this transition in label form e.g.
* "//command_line_option:cpu" for native options and "//myapp:foo" for starlark options.
@@ -421,38 +423,34 @@
}
CoreOptions buildConfigOptions = toOptions.get(CoreOptions.class);
- Set<String> updatedAffectedByStarlarkTransition =
- new TreeSet<>(buildConfigOptions.affectedByStarlarkTransition);
- // Add newly changed native options to overall list of changed native options
- for (String option : changedOptions) {
- if (option.startsWith(COMMAND_LINE_OPTION_PREFIX)) {
- updatedAffectedByStarlarkTransition.add(
- option.substring(COMMAND_LINE_OPTION_PREFIX.length()));
- }
- }
- buildConfigOptions.affectedByStarlarkTransition =
- ImmutableList.sortedCopyOf(updatedAffectedByStarlarkTransition);
- // hash all relevant native option values;
+ updateAffectedByStarlarkTransition(buildConfigOptions, changedOptions);
+
TreeMap<String, Object> toHash = new TreeMap<>();
- for (String nativeOption : updatedAffectedByStarlarkTransition) {
- Object value;
- try {
- value =
- optionInfoMap
- .get(nativeOption)
- .getDefinition()
- .getField()
- .get(toOptions.get(optionInfoMap.get(nativeOption).getOptionClass()));
- } catch (IllegalAccessException e) {
- throw new RuntimeException(
- "IllegalAccess for option " + nativeOption + ": " + e.getMessage());
+ for (String optionName : buildConfigOptions.affectedByStarlarkTransition) {
+ if (optionName.startsWith(COMMAND_LINE_OPTION_PREFIX)) {
+ String nativeOptionName = optionName.substring(COMMAND_LINE_OPTION_PREFIX.length());
+ Object value;
+ try {
+ value =
+ optionInfoMap
+ .get(nativeOptionName)
+ .getDefinition()
+ .getField()
+ .get(toOptions.get(optionInfoMap.get(nativeOptionName).getOptionClass()));
+ } catch (IllegalAccessException e) {
+ throw new VerifyException(
+ "IllegalAccess for option " + nativeOptionName + ": " + e.getMessage());
+ }
+ toHash.put(optionName, value);
+ } else {
+ Object value = toOptions.getStarlarkOptions().get(Label.parseAbsoluteUnchecked(optionName));
+ if (value != null) {
+ toHash.put(optionName, value);
+ }
}
- toHash.put(nativeOption, value);
}
- // hash all starlark options in map.
- toOptions.getStarlarkOptions().forEach((opt, value) -> toHash.put(opt.toString(), value));
ImmutableList.Builder<String> hashStrs = ImmutableList.builderWithExpectedSize(toHash.size());
for (Map.Entry<String, Object> singleOptionAndValue : toHash.entrySet()) {
String toAdd = singleOptionAndValue.getKey() + "=" + singleOptionAndValue.getValue();
@@ -462,6 +460,19 @@
transitionDirectoryNameFragment(hashStrs.build());
}
+ /**
+ * Extend the global build config affectedByStarlarkTransition, by adding any new option names
+ * from changedOptions
+ */
+ private static void updateAffectedByStarlarkTransition(
+ CoreOptions buildConfigOptions, Set<String> changedOptions) {
+ Set<String> mutableCopyToUpdate =
+ new TreeSet<>(buildConfigOptions.affectedByStarlarkTransition);
+ mutableCopyToUpdate.addAll(changedOptions);
+ buildConfigOptions.affectedByStarlarkTransition =
+ ImmutableList.sortedCopyOf(mutableCopyToUpdate);
+ }
+
public static String transitionDirectoryNameFragment(Iterable<String> opts) {
Fingerprint fp = new Fingerprint();
for (String opt : opts) {
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 268e03b..6e9106e 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
@@ -1137,10 +1137,59 @@
.isEqualTo(StarlarkInt.of(42));
}
+ @Test
+ public void testTransitionOnBuildSetting_onlyTransitionsAffectsDirectory() throws Exception {
+ writeAllowlistFile();
+ writeBuildSettingsBzl();
+ writeRulesWithAttrTransitionBzl();
+ 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 = ':dep')",
+ "my_rule(name = 'dep')",
+ "int_flag(name = 'the-answer', build_setting_default = 0)",
+ "int_flag(name = 'cmd-line-option', build_setting_default = 0)");
+
+ useConfiguration(ImmutableMap.of("//test/starlark:cmd-line-option", 100), "--cpu=FOO");
+
+ ConfiguredTarget test = getConfiguredTarget("//test/starlark:test");
+
+ @SuppressWarnings("unchecked")
+ ConfiguredTarget dep =
+ Iterables.getOnlyElement(
+ (List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep"));
+
+ // Assert starlark option set via transition.
+ assertThat(getStarlarkOption(dep, "//test/starlark:the-answer")).isEqualTo(StarlarkInt.of(42));
+
+ // Assert starlark option set via command line.
+ assertThat(getStarlarkOption(dep, "//test/starlark:cmd-line-option")).isEqualTo(100);
+
+ // Assert native option set via command line.
+ assertThat(getCoreOptions(dep).cpu).isEqualTo("FOO");
+
+ // Assert that transitionDirectoryNameFragment is only affected by options
+ // set via transitions. Not by native or starlark options set via command line,
+ // never touched by any transition.
+ assertThat(getCoreOptions(dep).transitionDirectoryNameFragment)
+ .isEqualTo(
+ FunctionTransitionUtil.transitionDirectoryNameFragment(
+ ImmutableList.of("//test/starlark:the-answer=42")));
+ }
+
private CoreOptions getCoreOptions(ConfiguredTarget target) {
return getConfiguration(target).getOptions().get(CoreOptions.class);
}
+ private ImmutableMap<Label, Object> getStarlarkOptions(ConfiguredTarget target) {
+ return getConfiguration(target).getOptions().getStarlarkOptions();
+ }
+
+ private Object getStarlarkOption(ConfiguredTarget target, String absName) {
+ return getStarlarkOptions(target).get(Label.parseAbsoluteUnchecked(absName));
+ }
+
@Test
public void testOutputDirHash_multipleNativeOptionTransitions() throws Exception {
writeAllowlistFile();
@@ -1182,7 +1231,7 @@
List<String> affectedOptions = getCoreOptions(test).affectedByStarlarkTransition;
- assertThat(affectedOptions).containsExactly("foo");
+ assertThat(affectedOptions).containsExactly("//command_line_option:foo");
@SuppressWarnings("unchecked")
ConfiguredTarget dep =
@@ -1191,17 +1240,19 @@
affectedOptions = getCoreOptions(dep).affectedByStarlarkTransition;
- assertThat(affectedOptions).containsExactly("foo", "bar");
+ assertThat(affectedOptions)
+ .containsExactly("//command_line_option:foo", "//command_line_option:bar");
assertThat(getCoreOptions(test).transitionDirectoryNameFragment)
.isEqualTo(
FunctionTransitionUtil.transitionDirectoryNameFragment(
- ImmutableList.of("foo=foosball")));
+ ImmutableList.of("//command_line_option:foo=foosball")));
assertThat(getCoreOptions(dep).transitionDirectoryNameFragment)
.isEqualTo(
FunctionTransitionUtil.transitionDirectoryNameFragment(
- ImmutableList.of("bar=barsball", "foo=foosball")));
+ ImmutableList.of(
+ "//command_line_option:bar=barsball", "//command_line_option:foo=foosball")));
}
// Test that a no-op starlark transition to an already starlark transitioned configuration
@@ -1523,8 +1574,7 @@
List<String> affectedOptions =
getConfiguration(dep).getOptions().get(CoreOptions.class).affectedByStarlarkTransition;
- // Assert that affectedOptions is empty but final fragment is still different.
- assertThat(affectedOptions).isEmpty();
+ assertThat(affectedOptions).containsExactly("//test:bar", "//test:foo");
assertThat(getCoreOptions(test).transitionDirectoryNameFragment)
.isEqualTo(
FunctionTransitionUtil.transitionDirectoryNameFragment(
@@ -1611,12 +1661,12 @@
List<String> affectedOptionsTop =
getConfiguration(top).getOptions().get(CoreOptions.class).affectedByStarlarkTransition;
- assertThat(affectedOptionsTop).containsExactly("foo");
+ assertThat(affectedOptionsTop).containsExactly("//command_line_option:foo");
assertThat(getConfiguration(top).getOptions().getStarlarkOptions()).isEmpty();
assertThat(getCoreOptions(top).transitionDirectoryNameFragment)
.isEqualTo(
FunctionTransitionUtil.transitionDirectoryNameFragment(
- ImmutableList.of("foo=foosball")));
+ ImmutableList.of("//command_line_option:foo=foosball")));
// test:middle (foo_transition, zee_transition, bar_transition)
@SuppressWarnings("unchecked")
@@ -1626,14 +1676,20 @@
List<String> affectedOptionsMiddle =
getConfiguration(middle).getOptions().get(CoreOptions.class).affectedByStarlarkTransition;
- assertThat(affectedOptionsMiddle).containsExactly("foo", "bar");
+ assertThat(affectedOptionsMiddle)
+ .containsExactly("//command_line_option:foo", "//command_line_option:bar", "//test:zee");
+
assertThat(getConfiguration(middle).getOptions().getStarlarkOptions().entrySet())
.containsExactly(
Maps.immutableEntry(Label.parseAbsoluteUnchecked("//test:zee"), "zeesball"));
+
assertThat(getCoreOptions(middle).transitionDirectoryNameFragment)
.isEqualTo(
FunctionTransitionUtil.transitionDirectoryNameFragment(
- ImmutableList.of("//test:zee=zeesball", "bar=barsball", "foo=foosball")));
+ ImmutableList.of(
+ "//command_line_option:bar=barsball",
+ "//command_line_option:foo=foosball",
+ "//test:zee=zeesball")));
// test:bottom (foo_transition, zee_transition, bar_transition, xan_transition)
@SuppressWarnings("unchecked")
@@ -1644,7 +1700,10 @@
List<String> affectedOptionsBottom =
getConfiguration(bottom).getOptions().get(CoreOptions.class).affectedByStarlarkTransition;
- assertThat(affectedOptionsBottom).containsExactly("foo", "bar");
+ assertThat(affectedOptionsBottom)
+ .containsExactly(
+ "//command_line_option:foo", "//command_line_option:bar", "//test:xan", "//test:zee");
+
assertThat(getConfiguration(bottom).getOptions().getStarlarkOptions().entrySet())
.containsExactly(
Maps.immutableEntry(Label.parseAbsoluteUnchecked("//test:zee"), "zeesball"),
@@ -1653,7 +1712,8 @@
.isEqualTo(
FunctionTransitionUtil.transitionDirectoryNameFragment(
ImmutableList.of(
- "//test:xan=xansball", "//test:zee=zeesball", "bar=barsball", "foo=foosball")));
+ "//command_line_option:bar=barsball", "//command_line_option:foo=foosball",
+ "//test:xan=xansball", "//test:zee=zeesball")));
}
@Test
