Skip to content

Commit

Permalink
Same transition dirs when only cmd line options differs
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@ulrfa @copybara-github
ulrfa authored and copybara-github committed Sep 14, 2021
1 parent 331e19a commit 82d4db2
Show file tree
Hide file tree
Showing 3 changed files with 122 additions and 49 deletions.
8 changes: 5 additions & 3 deletions src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -261,9 +261,11 @@ public String getTypeDescription() {
}

/**
* 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",
Expand Down
79 changes: 45 additions & 34 deletions src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -289,13 +290,13 @@ private static BuildOptions applyTransition(

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.
Expand Down Expand Up @@ -398,9 +399,10 @@ private static BuildOptions applyTransition(
}

/**
* 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.
Expand All @@ -421,38 +423,34 @@ private static void updateOutputDirectoryNameFragment(
}

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();
Expand All @@ -462,6 +460,19 @@ private static void updateOutputDirectoryNameFragment(
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) {
Expand Down
84 changes: 72 additions & 12 deletions src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1137,10 +1137,59 @@ public void testTransitionOnBuildSetting_fromCommandLine() throws Exception {
.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();
Expand Down Expand Up @@ -1182,7 +1231,7 @@ public void testOutputDirHash_multipleNativeOptionTransitions() throws Exception

List<String> affectedOptions = getCoreOptions(test).affectedByStarlarkTransition;

assertThat(affectedOptions).containsExactly("foo");
assertThat(affectedOptions).containsExactly("//command_line_option:foo");

@SuppressWarnings("unchecked")
ConfiguredTarget dep =
Expand All @@ -1191,17 +1240,19 @@ public void testOutputDirHash_multipleNativeOptionTransitions() throws Exception

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
Expand Down Expand Up @@ -1523,8 +1574,7 @@ public void testOutputDirHash_multipleStarlarkTransitions() throws Exception {
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(
Expand Down Expand Up @@ -1611,12 +1661,12 @@ public void testOutputDirHash_multipleMixedTransitions() throws Exception {
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")
Expand All @@ -1626,14 +1676,20 @@ public void testOutputDirHash_multipleMixedTransitions() throws Exception {
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")
Expand All @@ -1644,7 +1700,10 @@ public void testOutputDirHash_multipleMixedTransitions() throws Exception {
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"),
Expand All @@ -1653,7 +1712,8 @@ public void testOutputDirHash_multipleMixedTransitions() throws Exception {
.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
Expand Down

0 comments on commit 82d4db2

Please sign in to comment.