Skip to content

Commit

Permalink
Make no-op starlark transition not affect the output directory.
Browse files Browse the repository at this point in the history
    Not having this logic in was leading to c++ action conflicts. Before this CL we naively hashed the map of options->values returned by a transition into the output directory fragment. Now we only update the output directory fragment if values actually change. In order to do this, we also need to add the default values of all the output options to the starlark options map (if they don't already have a non-default value in the map) to take care of the set-to-default case also being a no-op. Add a bunch of tests and delete tests that showed the undesirable behavior before.

    Also fixes bazelbuild/bazel#11196

    TESTED: patch unknown commit && devblaze build //googlex/koi/...
    PiperOrigin-RevId: 327116054
  • Loading branch information
Luca Di Grazia committed Sep 4, 2022
1 parent 2e04fe5 commit eb65f0e
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 75 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ public static Map<String, BuildOptions> applyTransition(
ConfigurationTransition transition,
Map<PackageValue.Key, PackageValue> buildSettingPackages,
ExtendedEventHandler eventHandler)
throws TransitionException, InterruptedException {
throws TransitionException {
boolean doesStarlarkTransition = StarlarkTransition.doesStarlarkTransition(transition);
if (doesStarlarkTransition) {
fromOptions =
Expand Down Expand Up @@ -534,7 +534,7 @@ public static TopLevelTargetsAndConfigsResult getConfigurationsFromExecutor(
Multimap<BuildConfiguration, DependencyKey> targetsToEvaluate,
ExtendedEventHandler eventHandler,
ConfigurationsCollector configurationsCollector)
throws InvalidConfigurationException, InterruptedException {
throws InvalidConfigurationException {

Map<Label, Target> labelsToTargets = new HashMap<>();
for (TargetAndConfiguration targetAndConfig : defaultContext) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,25 +28,25 @@
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.packages.StructImpl;
import com.google.devtools.build.lib.syntax.Dict;
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.Location;
import com.google.devtools.build.lib.syntax.Mutability;
import com.google.devtools.build.lib.syntax.NoneType;
import com.google.devtools.build.lib.syntax.Starlark;
import com.google.devtools.build.lib.util.Fingerprint;
import com.google.devtools.common.options.OptionDefinition;
import com.google.devtools.common.options.OptionsParser;
import com.google.devtools.common.options.OptionsParsingException;
import java.lang.reflect.Field;
import java.util.Collection;
import java.util.HashSet;
import java.util.HashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.TreeMap;
import java.util.TreeSet;
import net.starlark.java.eval.Dict;
import net.starlark.java.eval.EvalException;
import net.starlark.java.eval.Mutability;
import net.starlark.java.eval.NoneType;
import net.starlark.java.eval.Starlark;
import net.starlark.java.syntax.Location;

/**
* Utility class for common work done across {@link StarlarkAttributeTransitionProvider} and {@link
Expand Down Expand Up @@ -239,7 +239,7 @@ private static BuildOptions applyTransition(
throws EvalException {
BuildOptions buildOptions = buildOptionsToTransition.clone();
// The names and values of options that are different after this transition.
Set<String> convertedNewValues = new HashSet<>();
HashMap<String, Object> convertedNewValues = new HashMap<>();
for (Map.Entry<String, Object> entry : newValues.entrySet()) {
String optionName = entry.getKey();
Object optionValue = entry.getValue();
Expand All @@ -258,7 +258,7 @@ private static BuildOptions applyTransition(
.merge(buildOptions)
.addStarlarkOption(Label.parseAbsoluteUnchecked(optionName), optionValue)
.build();
convertedNewValues.add(optionName);
convertedNewValues.put(optionName, optionValue);
}
} else {
optionName = optionName.substring(COMMAND_LINE_OPTION_PREFIX.length());
Expand Down Expand Up @@ -314,7 +314,7 @@ private static BuildOptions applyTransition(
|| (oldValue != null && convertedValue == null)
|| (oldValue != null && !oldValue.equals(convertedValue))) {
field.set(options, convertedValue);
convertedNewValues.add(entry.getKey());
convertedNewValues.put(entry.getKey(), convertedValue);
}

} catch (IllegalArgumentException e) {
Expand All @@ -334,12 +334,9 @@ private static BuildOptions applyTransition(
buildConfigOptions = buildOptions.get(CoreOptions.class);

if (starlarkTransition.isForAnalysisTesting()) {
// We need to record every time we change a configuration option.
// see {@link #updateOutputDirectoryNameFragment} for usage.
convertedNewValues.add("//command_line_option:evaluating for analysis test");
buildConfigOptions.evaluatingForAnalysisTest = true;
}
updateOutputDirectoryNameFragment(convertedNewValues, optionInfoMap, buildOptions);
updateOutputDirectoryNameFragment(convertedNewValues.keySet(), optionInfoMap, buildOptions);

return buildOptions;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -405,23 +405,22 @@ private static BuildOptions unalias(
public static ImmutableMap<Label, Object> getDefaultValues(
Map<PackageValue.Key, PackageValue> buildSettingPackages, ConfigurationTransition root)
throws TransitionException {
HashMap<Label, Object> defaultValues = new HashMap<>();
ImmutableMap.Builder<Label, Object> defaultValues = new ImmutableMap.Builder<>();
root.visit(
(StarlarkTransitionVisitor)
transition -> {
ImmutableSet<Label> settings =
getRelevantStarlarkSettingsFromTransition(
transition, Settings.INPUTS_AND_OUTPUTS);
for (Label setting : settings) {
defaultValues.computeIfAbsent(
defaultValues.put(
setting,
(Label settingLabel) ->
getActual(buildSettingPackages, settingLabel)
.getAssociatedRule()
.getAttr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME));
getActual(buildSettingPackages, setting)
.getAssociatedRule()
.getAttr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME));
}
});
return ImmutableMap.copyOf(defaultValues);
return defaultValues.build();
}

/**
Expand Down
Loading

0 comments on commit eb65f0e

Please sign in to comment.