Add logic for skylark options to all BuildOptions mechanics (construction, diff-ing, reconstruction, serialization, etc). Get the new skylark information from an OptionsProvider (the same way native options are retrieved). Rename the <code>getOptions()</code> method in BuildOptions to <code>getNativeOptions</code> to better reflect this new information being stored in BuildOptions.
PiperOrigin-RevId: 209841664
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
index 71d28c8..3f30da4 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
@@ -1096,7 +1096,7 @@
// "bazel-out/android-arm-linux-fastbuild/bin". That's pretty awkward to check here.
// && outputRoots.equals(other.outputRoots)
&& fragments.values().containsAll(other.fragments.values())
- && buildOptions.getOptions().containsAll(other.buildOptions.getOptions()));
+ && buildOptions.getNativeOptions().containsAll(other.buildOptions.getNativeOptions()));
}
/**
@@ -1117,7 +1117,7 @@
}
private int computeHashCode() {
- return Objects.hash(fragments, buildOptions.getOptions());
+ return Objects.hash(fragments, buildOptions.getNativeOptions());
}
public void describe(StringBuilder sb) {
@@ -1366,7 +1366,7 @@
}
return TransitiveOptionDetails.forOptionsWithDefaults(
- buildOptions.getOptions(), lateBoundDefaults);
+ buildOptions.getNativeOptions(), lateBoundDefaults);
}
private String buildMnemonic() {
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java
index 33dd455..6efa6ac 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java
@@ -24,6 +24,7 @@
import com.google.common.collect.ImmutableSortedMap;
import com.google.common.collect.Iterables;
import com.google.common.collect.Multimap;
+import com.google.common.collect.Ordering;
import com.google.common.collect.Sets;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy;
@@ -70,6 +71,7 @@
public final class BuildOptions implements Cloneable, Serializable {
private static final Comparator<Class<? extends FragmentOptions>>
lexicalFragmentOptionsComparator = Comparator.comparing(Class::getName);
+ private static final Comparator<String> skylarkOptionsComparator = Ordering.natural();
private static final Logger logger = Logger.getLogger(BuildOptions.class.getName());
/**
@@ -115,7 +117,7 @@
builder.add(options);
}
}
- return builder.build();
+ return builder.addSkylarkOptions(skylarkOptionsMap).build();
}
/**
@@ -128,10 +130,13 @@
for (Class<? extends FragmentOptions> optionsClass : optionsList) {
builder.add(provider.getOptions(optionsClass));
}
- return builder.build();
+ return builder.addSkylarkOptions(provider.getSkylarkOptions()).build();
}
- /** Creates a BuildOptions class by taking the option values from command-line arguments. */
+ /**
+ * Creates a BuildOptions class by taking the option values from command-line arguments. Returns a
+ * BuildOptions class that only has native options.
+ */
@VisibleForTesting
public static BuildOptions of(List<Class<? extends FragmentOptions>> optionsList, String... args)
throws OptionsParsingException {
@@ -146,6 +151,14 @@
return builder.build();
}
+ /*
+ * Returns a BuildOptions class that only has skylark options.
+ */
+ @VisibleForTesting
+ public static BuildOptions of(Map<String, Object> skylarkOptions) {
+ return builder().addSkylarkOptions(skylarkOptions).build();
+ }
+
/** Returns the actual instance of a FragmentOptions class. */
public <T extends FragmentOptions> T get(Class<T> optionsClass) {
FragmentOptions options = fragmentOptionsMap.get(optionsClass);
@@ -185,6 +198,7 @@
for (FragmentOptions options : fragmentOptionsMap.values()) {
keyBuilder.append(options.cacheKey());
}
+ keyBuilder.append(OptionsBase.mapToCacheKey(skylarkOptionsMap));
return keyBuilder.toString();
}
@@ -203,20 +217,29 @@
}
/** Returns the options contained in this collection. */
- public Collection<FragmentOptions> getOptions() {
+ public Collection<FragmentOptions> getNativeOptions() {
return fragmentOptionsMap.values();
}
- /** Creates a copy of the BuildOptions object that contains copies of the FragmentOptions. */
+ public ImmutableMap<String, Object> getSkylarkOptions() {
+ return skylarkOptionsMap;
+ }
+
+ /**
+ * Creates a copy of the BuildOptions object that contains copies of the FragmentOptions and
+ * skylark options.
+ */
@Override
public BuildOptions clone() {
- ImmutableMap.Builder<Class<? extends FragmentOptions>, FragmentOptions> builder =
+ ImmutableMap.Builder<Class<? extends FragmentOptions>, FragmentOptions> nativeOptionsBuilder =
ImmutableMap.builder();
for (Map.Entry<Class<? extends FragmentOptions>, FragmentOptions> entry :
fragmentOptionsMap.entrySet()) {
- builder.put(entry.getKey(), entry.getValue().clone());
+ nativeOptionsBuilder.put(entry.getKey(), entry.getValue().clone());
}
- return new BuildOptions(builder.build());
+ return new BuildOptions(
+ nativeOptionsBuilder.build(),
+ ImmutableMap.copyOf(skylarkOptionsMap));
}
/**
@@ -242,6 +265,10 @@
fingerprint.addString(entry.getKey().getName());
fingerprint.addString(entry.getValue().cacheKey());
}
+ for (Map.Entry<String, Object> entry : skylarkOptionsMap.entrySet()) {
+ fingerprint.addString(entry.getKey());
+ fingerprint.addString(entry.getValue().toString());
+ }
byte[] computedFingerprint = fingerprint.digestAndReset();
hashCode = Arrays.hashCode(computedFingerprint);
this.fingerprint = computedFingerprint;
@@ -274,10 +301,15 @@
/** Maps options class definitions to FragmentOptions objects. */
private final ImmutableMap<Class<? extends FragmentOptions>, FragmentOptions> fragmentOptionsMap;
+ /** Maps skylark options names to skylark options values. */
+ private final ImmutableMap<String, Object> skylarkOptionsMap;
@AutoCodec.VisibleForSerialization
- BuildOptions(ImmutableMap<Class<? extends FragmentOptions>, FragmentOptions> fragmentOptionsMap) {
+ BuildOptions(
+ ImmutableMap<Class<? extends FragmentOptions>, FragmentOptions> fragmentOptionsMap,
+ ImmutableMap<String, Object> skylarkOptionsMap) {
this.fragmentOptionsMap = fragmentOptionsMap;
+ this.skylarkOptionsMap = skylarkOptionsMap;
}
public BuildOptions applyDiff(OptionsDiffForReconstruction optionsDiff) {
@@ -298,6 +330,20 @@
for (FragmentOptions extraSecondFragment : optionsDiff.extraSecondFragments) {
builder.add(extraSecondFragment);
}
+
+ Map<String, Object> skylarkOptions = new HashMap<>();
+ for (Map.Entry<String, Object> buildSettingAndValue : skylarkOptionsMap.entrySet()) {
+ String buildSetting = buildSettingAndValue.getKey();
+ if (optionsDiff.extraFirstSkylarkOptions.contains(buildSetting)) {
+ continue;
+ } else if (optionsDiff.differingSkylarkOptions.containsKey(buildSetting)) {
+ skylarkOptions.put(buildSetting, optionsDiff.differingSkylarkOptions.get(buildSetting));
+ } else {
+ skylarkOptions.put(buildSetting, skylarkOptionsMap.get(buildSetting));
+ }
+ }
+ skylarkOptions.putAll(optionsDiff.extraSecondSkylarkOptions);
+ builder.addSkylarkOptions(skylarkOptions);
return builder.build();
}
@@ -317,15 +363,23 @@
return this;
}
+ Builder addSkylarkOptions(Map<String, Object> options) {
+ builderSkylarkOptionsMap.putAll(options);
+ return this;
+ }
+
public BuildOptions build() {
return new BuildOptions(
- ImmutableSortedMap.copyOf(builderMap, lexicalFragmentOptionsComparator));
+ ImmutableSortedMap.copyOf(builderMap, lexicalFragmentOptionsComparator),
+ ImmutableSortedMap.copyOf(builderSkylarkOptionsMap, skylarkOptionsComparator));
}
private Map<Class<? extends FragmentOptions>, FragmentOptions> builderMap;
+ private Map<String, Object> builderSkylarkOptionsMap;
private Builder() {
builderMap = new HashMap<>();
+ builderSkylarkOptionsMap = new HashMap<>();
}
}
@@ -347,6 +401,11 @@
*/
public static OptionsDiff diff(
OptionsDiff diff, @Nullable BuildOptions first, @Nullable BuildOptions second) {
+ if (diff.hasSkylarkOptions) {
+ throw new IllegalStateException(
+ "OptionsDiff cannot handle multiple 'second' BuildOptions with skylark options "
+ + "and is trying to diff against a second BuildOptions with skylark options.");
+ }
if (first == null || second == null) {
throw new IllegalArgumentException("Cannot diff null BuildOptions");
}
@@ -357,13 +416,13 @@
// other.
ImmutableSet<Class<? extends FragmentOptions>> firstOptionClasses =
first
- .getOptions()
+ .getNativeOptions()
.stream()
.map(FragmentOptions::getClass)
.collect(ImmutableSet.toImmutableSet());
ImmutableSet<Class<? extends FragmentOptions>> secondOptionClasses =
second
- .getOptions()
+ .getNativeOptions()
.stream()
.map(FragmentOptions::getClass)
.collect(ImmutableSet.toImmutableSet());
@@ -389,6 +448,21 @@
}
}
}
+
+ // Compare skylark options for the two classes
+ Map<String, Object> skylarkFirst = first.getSkylarkOptions();
+ Map<String, Object> skylarkSecond = second.getSkylarkOptions();
+ diff.setHasSkylarkOptions(!skylarkFirst.isEmpty() || !skylarkSecond.isEmpty());
+ for (String buildSetting : Sets.union(skylarkFirst.keySet(), skylarkSecond.keySet())) {
+ if (skylarkFirst.get(buildSetting) == null) {
+ diff.addExtraSecondSkylarkOption(buildSetting, skylarkSecond.get(buildSetting));
+ } else if (skylarkSecond.get(buildSetting) == null) {
+ diff.addExtraFirstSkylarkOption(buildSetting);
+ } else if (!skylarkFirst.get(buildSetting).equals(skylarkSecond.get(buildSetting))) {
+ diff.putSkylarkDiff(
+ buildSetting, skylarkFirst.get(buildSetting), skylarkSecond.get(buildSetting));
+ }
+ }
return diff;
}
@@ -444,7 +518,10 @@
.sorted(Comparator.comparing(o -> o.getClass().getName()))
.collect(ImmutableList.toImmutableList()),
first.fingerprint,
- second.computeChecksum());
+ second.computeChecksum(),
+ diff.skylarkSecond,
+ diff.extraSkylarkOptionsFirst,
+ diff.extraSkylarkOptionsSecond);
}
/**
@@ -465,13 +542,15 @@
private final Set<Class<? extends FragmentOptions>> extraFirstFragments = new HashSet<>();
private final Set<FragmentOptions> extraSecondFragments = new HashSet<>();
- private void addExtraFirstFragment(Class<? extends FragmentOptions> options) {
- extraFirstFragments.add(options);
- }
+ private final Map<String, Object> skylarkFirst = new LinkedHashMap<>();
+ // TODO(b/112041323): This should also be multimap but we don't diff multiple times with
+ // skylark options anywhere yet so add that feature when necessary.
+ private final Map<String, Object> skylarkSecond = new LinkedHashMap<>();
- private void addExtraSecondFragment(FragmentOptions options) {
- extraSecondFragments.add(options);
- }
+ private final List<String> extraSkylarkOptionsFirst = new ArrayList<>();
+ private final Map<String, Object> extraSkylarkOptionsSecond = new HashMap<>();
+
+ private boolean hasSkylarkOptions = false;
@VisibleForTesting
Set<Class<? extends FragmentOptions>> getExtraFirstFragmentClassesForTesting() {
@@ -501,6 +580,51 @@
second.put(option, secondValue);
}
+ private void addExtraFirstFragment(Class<? extends FragmentOptions> options) {
+ extraFirstFragments.add(options);
+ }
+
+ private void addExtraSecondFragment(FragmentOptions options) {
+ extraSecondFragments.add(options);
+ }
+
+ private void putSkylarkDiff(String buildSetting, Object firstValue, Object secondValue) {
+ skylarkFirst.put(buildSetting, firstValue);
+ skylarkSecond.put(buildSetting, secondValue);
+ }
+
+ private void addExtraFirstSkylarkOption(String buildSetting) {
+ extraSkylarkOptionsFirst.add(buildSetting);
+ }
+
+ private void addExtraSecondSkylarkOption(String buildSetting, Object value) {
+ extraSkylarkOptionsSecond.put(buildSetting, value);
+ }
+
+ private void setHasSkylarkOptions(boolean hasSkylarkOptions) {
+ this.hasSkylarkOptions = hasSkylarkOptions;
+ }
+
+ @VisibleForTesting
+ Map<String, Object> getSkylarkFirstForTesting() {
+ return skylarkFirst;
+ }
+
+ @VisibleForTesting
+ Map<String, Object> getSkylarkSecondForTesting() {
+ return skylarkSecond;
+ }
+
+ @VisibleForTesting
+ List<String> getExtraSkylarkOptionsFirstForTesting() {
+ return extraSkylarkOptionsFirst;
+ }
+
+ @VisibleForTesting
+ Map<String, Object> getExtraSkylarkOptionsSecondForTesting() {
+ return extraSkylarkOptionsSecond;
+ }
+
/**
* Note: it's not enough for first and second to be empty, with trimming, they must also contain
* the same options classes.
@@ -510,7 +634,11 @@
&& second.isEmpty()
&& extraSecondFragments.isEmpty()
&& extraFirstFragments.isEmpty()
- && differingOptions.isEmpty();
+ && differingOptions.isEmpty()
+ && skylarkFirst.isEmpty()
+ && skylarkSecond.isEmpty()
+ && extraSkylarkOptionsFirst.isEmpty()
+ && extraSkylarkOptionsSecond.isEmpty();
}
public String prettyPrint() {
@@ -526,6 +654,8 @@
first.forEach(
(option, value) ->
toReturn.add(option.getOptionName() + ":" + value + " -> " + second.get(option)));
+ skylarkFirst.forEach(
+ (option, value) -> toReturn.add(option + ":" + value + skylarkSecond.get(option)));
return toReturn;
}
}
@@ -535,29 +665,46 @@
* another: the full fragments of the second one, the fragment classes of the first that should be
* omitted, and the values of any fields that should be changed.
*/
- public static class OptionsDiffForReconstruction {
+ public static final class OptionsDiffForReconstruction {
private final Map<Class<? extends FragmentOptions>, Map<String, Object>> differingOptions;
private final ImmutableSet<Class<? extends FragmentOptions>> extraFirstFragmentClasses;
private final ImmutableList<FragmentOptions> extraSecondFragments;
private final byte[] baseFingerprint;
private final String checksum;
+ private final Map<String, Object> differingSkylarkOptions;
+ private final List<String> extraFirstSkylarkOptions;
+ private final Map<String, Object> extraSecondSkylarkOptions;
+
OptionsDiffForReconstruction(
Map<Class<? extends FragmentOptions>, Map<String, Object>> differingOptions,
ImmutableSet<Class<? extends FragmentOptions>> extraFirstFragmentClasses,
ImmutableList<FragmentOptions> extraSecondFragments,
byte[] baseFingerprint,
- String checksum) {
+ String checksum,
+ Map<String, Object> differingSkylarkOptions,
+ List<String> extraFirstSkylarkOptions,
+ Map<String, Object> extraSecondSkylarkOptions) {
this.differingOptions = differingOptions;
this.extraFirstFragmentClasses = extraFirstFragmentClasses;
this.extraSecondFragments = extraSecondFragments;
this.baseFingerprint = baseFingerprint;
this.checksum = checksum;
+ this.differingSkylarkOptions = differingSkylarkOptions;
+ this.extraFirstSkylarkOptions = extraFirstSkylarkOptions;
+ this.extraSecondSkylarkOptions = extraSecondSkylarkOptions;
}
private static OptionsDiffForReconstruction getEmpty(byte[] baseFingerprint, String checksum) {
return new OptionsDiffForReconstruction(
- ImmutableMap.of(), ImmutableSet.of(), ImmutableList.of(), baseFingerprint, checksum);
+ ImmutableMap.of(),
+ ImmutableSet.of(),
+ ImmutableList.of(),
+ baseFingerprint,
+ checksum,
+ ImmutableMap.of(),
+ ImmutableList.of(),
+ ImmutableMap.of());
}
@Nullable
@@ -589,7 +736,10 @@
private boolean isEmpty() {
return differingOptions.isEmpty()
&& extraFirstFragmentClasses.isEmpty()
- && extraSecondFragments.isEmpty();
+ && extraSecondFragments.isEmpty()
+ && differingSkylarkOptions.isEmpty()
+ && extraFirstSkylarkOptions.isEmpty()
+ && extraSecondSkylarkOptions.isEmpty();
}
@Override
@@ -611,6 +761,9 @@
.add("differingOptions", differingOptions)
.add("extraFirstFragmentClasses", extraFirstFragmentClasses)
.add("extraSecondFragments", extraSecondFragments)
+ .add("differingSkylarkOptions", differingSkylarkOptions)
+ .add("extraFirstSkylarkOptions", extraFirstSkylarkOptions)
+ .add("extraSecondSkylarkOptions", extraSecondSkylarkOptions)
.toString();
}
@@ -643,6 +796,9 @@
context.serialize(diff.extraSecondFragments, bytesOut);
bytesOut.writeByteArrayNoTag(diff.baseFingerprint);
context.serialize(diff.checksum, bytesOut);
+ context.serialize(diff.differingSkylarkOptions, bytesOut);
+ context.serialize(diff.extraFirstSkylarkOptions, bytesOut);
+ context.serialize(diff.extraSecondSkylarkOptions, bytesOut);
bytesOut.flush();
byteStringOut.flush();
int optionsDiffSize = byteStringOut.size();
@@ -675,13 +831,19 @@
ImmutableList<FragmentOptions> extraSecondFragments = context.deserialize(codedInput);
byte[] baseFingerprint = codedInput.readByteArray();
String checksum = context.deserialize(codedInput);
+ Map<String, Object> differingSkylarkOptions = context.deserialize(codedInput);
+ List<String> extraFirstSkylarkOptions = context.deserialize(codedInput);
+ Map<String, Object> extraSecondSkylarkOptions = context.deserialize(codedInput);
diff =
new OptionsDiffForReconstruction(
differingOptions,
extraFirstFragmentClasses,
extraSecondFragments,
baseFingerprint,
- checksum);
+ checksum,
+ differingSkylarkOptions,
+ extraFirstSkylarkOptions,
+ extraSecondSkylarkOptions);
cache.putBytesFromOptionsDiff(diff, bytes);
}
return diff;
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestTrimmingTransitionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestTrimmingTransitionFactory.java
index ed8f625..5a35c16 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestTrimmingTransitionFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestTrimmingTransitionFactory.java
@@ -52,7 +52,7 @@
return originalOptions;
}
BuildOptions.Builder builder = BuildOptions.builder();
- for (FragmentOptions options : originalOptions.getOptions()) {
+ for (FragmentOptions options : originalOptions.getNativeOptions()) {
if (!(options instanceof TestOptions)) {
builder.add(options);
}
diff --git a/src/main/java/com/google/devtools/common/options/OptionsBase.java b/src/main/java/com/google/devtools/common/options/OptionsBase.java
index 9496c65..ce66e66 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsBase.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsBase.java
@@ -91,8 +91,13 @@
*/
public final String cacheKey() {
StringBuilder result = new StringBuilder(getClass().getName()).append("{");
+ result.append(mapToCacheKey(asMap()));
+ return result.append("}").toString();
+ }
- for (Map.Entry<String, Object> entry : asMap().entrySet()) {
+ public static String mapToCacheKey(Map<String, Object> optionsMap) {
+ StringBuilder result = new StringBuilder();
+ for (Map.Entry<String, Object> entry : optionsMap.entrySet()) {
result.append(entry.getKey()).append("=");
Object value = entry.getValue();
@@ -110,15 +115,12 @@
}
result.append(", ");
}
-
- return result.append("}").toString();
+ return result.toString();
}
@Override
public final boolean equals(Object that) {
- return that != null &&
- this.getClass() == that.getClass() &&
- this.asMap().equals(((OptionsBase) that).asMap());
+ return that instanceof OptionsBase && this.asMap().equals(((OptionsBase) that).asMap());
}
@Override
