Skip to content

Commit

Permalink
Have Starlark transition output call FragmentOptions.getNormalized().
Browse files Browse the repository at this point in the history 
This makes Starlark transitions more consistent with native transitions.

**Motivation**: fix a small difference between the Starlark and native exec transition. While they produce the same outputs, lack of normalization means the Starlark verion's `--define`, `--features`, `--host_features` order can be different.

This doesn't matter for most Bazel code. But it can break [BaselineOptionsFunction](https://github.com/bazelbuild/bazel/blob/c7d8d1e3f16ac6db37b134358b6cfdb5e3c8f6b0/src/main/java/com/google/devtools/build/lib/skyframe/config/BaselineOptionsFunction.java#L43).

**Also**: update `ComparingTransition` to catch ordering differences and add some unit tests.

PiperOrigin-RevId: 579185062
Change-Id: I13f19b7eb6c9067426a35ca403b4ba45949c04e3
  • Loading branch information
@gregestren @copybara-github
gregestren authored and copybara-github committed Nov 3, 2023
1 parent 9469d5a commit e9f0296
Show file tree
Hide file tree
Showing 4 changed files with 358 additions and 38 deletions.
75 changes: 45 additions & 30 deletions ...n/java/com/google/devtools/build/lib/analysis/config/transitions/ComparingTransition.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

package com.google.devtools.build.lib.analysis.config.transitions;

import static com.google.common.collect.ImmutableList.toImmutableList;
import static java.util.stream.Collectors.joining;

import com.google.common.collect.ImmutableMap;
Expand All @@ -24,11 +25,9 @@
import com.google.devtools.build.lib.analysis.config.BuildOptionsView;
import com.google.devtools.build.lib.analysis.config.CoreOptions;
import com.google.devtools.build.lib.analysis.config.FragmentOptions;
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.util.Pair;
import java.util.HashMap;
import java.util.Map;
import java.util.StringJoiner;
import java.util.TreeMap;
Expand Down Expand Up @@ -160,10 +159,11 @@ private void compare(
String activeVal = combined.getValue().getFirst();
String altVal = combined.getValue().getSecond();
if (activeVal.equals("DOES NOT EXIST")) {
s2.add(String.format(" only in %s mode: --%s=%s", activeTransitionDesc, option, altVal));
s2.add(String.format(" only in %s mode: --%s=%s", altTransitionDesc, option, altVal));
diffs++;
} else if (altVal.equals("DOES NOT EXIST")) {
s2.add(String.format(" only in %s mode: --%s=%s", altTransitionDesc, option, activeVal));
s2.add(
String.format(" only in %s mode: --%s=%s", activeTransitionDesc, option, activeVal));
diffs++;
} else if (!activeVal.equals(altVal)) {
s2.add(
Expand Down Expand Up @@ -194,39 +194,54 @@ private static ImmutableMap<String, String> serialize(BuildOptions o) {
for (FragmentOptions f : o.getNativeOptions()) {
for (Map.Entry<String, Object> op : f.asMap().entrySet()) {
if (op.getKey().equals("define")) {
for (Map.Entry<String, String> define :
o.get(CoreOptions.class).getNormalizedCommandLineBuildVariables().entrySet()) {
ans.put("user-defined define=" + define.getKey(), define.getValue());
}
ans.putAll(
serializeUserDefinedOption(
o.get(CoreOptions.class).commandLineBuildVariables.stream()
.map(d -> Map.entry(d.getKey(), d.getValue()))
.collect(toImmutableList()),
"define"));
} else if (op.getKey().equals("features")) {
var seen = new HashMap<String, Integer>();
for (String feature : o.get(CoreOptions.class).defaultFeatures) {
String suffix = "";
if (seen.containsKey(feature)) {
int dupeNum = seen.get(feature) + 1;
suffix = String.format(" (%d)", dupeNum);
seen.put(feature, dupeNum);
}
ans.put(String.format("user-defined feature=%s%s", feature, suffix), "");
}
ans.putAll(
serializeUserDefinedOption(
o.get(CoreOptions.class).defaultFeatures.stream()
.map(d -> Map.entry(d, ""))
.collect(toImmutableList()),
"feature"));
} else if (op.getKey().equals("host_features")) {
var seen = new HashMap<String, Integer>();
for (String feature : o.get(CoreOptions.class).hostFeatures) {
String suffix = "";
if (seen.containsKey(feature)) {
int dupeNum = seen.get(feature) + 1;
suffix = String.format(" (%d)", dupeNum);
seen.put(feature, dupeNum);
}
ans.put(String.format("user-defined host_feature=%s%s", feature, suffix), "");
}
ans.putAll(
serializeUserDefinedOption(
o.get(CoreOptions.class).hostFeatures.stream()
.map(d -> Map.entry(d, ""))
.collect(toImmutableList()),
"host feature"));
} else {
ans.put(prettyClassName(f.getClass()) + " " + op.getKey(), String.valueOf(op.getValue()));
}
}
}
for (Map.Entry<Label, Object> starlarkOption : o.getStarlarkOptions().entrySet()) {
ans.put("user-defined " + starlarkOption.getKey(), starlarkOption.getValue().toString());
ans.putAll(
serializeUserDefinedOption(
o.getStarlarkOptions().entrySet().stream()
.map(d -> Map.entry(d.getKey().toString(), d.getValue().toString()))
.collect(toImmutableList()),
""));
return ans.buildOrThrow();
}

/**
* Expands a {@link BuildOptions} native flag that represents a set of user-defined options.
*
* <p>For example: <code>--define</code> or <code>--features</code>.
*/
private static ImmutableMap<String, String> serializeUserDefinedOption(
Iterable<Map.Entry<String, String>> userDefinedOption, String desc) {
ImmutableMap.Builder<String, String> ans = ImmutableMap.builder();
int index = 0;
for (Map.Entry<String, String> entry : userDefinedOption) {
ans.put(
String.format("user-defined %s %s (index %d)", desc, entry.getKey(), index),
entry.getValue());
index++;
}
return ans.buildOrThrow();
}
Expand Down
16 changes: 8 additions & 8 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 @@ -449,16 +449,16 @@ private static BuildOptions applyTransition(
}
}

if (!changedStarlarkOptions.isEmpty()) {
toOptions =
BuildOptions.builder()
.merge(toOptions == null ? fromOptions.clone() : toOptions)
.addStarlarkOptions(changedStarlarkOptions)
.build();
}
if (toOptions == null) {
if (toOptions == null && changedStarlarkOptions.isEmpty()) {
return fromOptions;
}
// Note that rebuilding also calls FragmentOptions.getNormalized() to guarantee --define,
// --features, and similar flags are consistently ordered.
toOptions =
BuildOptions.builder()
.merge(toOptions == null ? fromOptions.clone() : toOptions)
.addStarlarkOptions(changedStarlarkOptions)
.build();
if (starlarkTransition.isForAnalysisTesting()) {
// We need to record every time we change a configuration option.
// see {@link #updateOutputDirectoryNameFragment} for usage.
Expand Down
1 change: 1 addition & 0 deletions src/test/java/com/google/devtools/build/lib/analysis/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/analysis:config/run_under_converter",
"//src/main/java/com/google/devtools/build/lib/analysis:config/toolchain_type_requirement",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transition_factories",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/comparing_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/composing_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/composing_transition_factory",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/configuration_transition",
Expand Down
Loading

0 comments on commit e9f0296

Please sign in to comment.