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