Make BuildOptions$OptionsDiffForReconstruction deterministic in its construction. Also restrict visibility of some test-only OptionsDiff methods, and remove a server-specific part of the ConfiguredTargetKey #toString representation.
PiperOrigin-RevId: 197830577
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 a91649b..33cd98af 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
@@ -426,10 +426,19 @@
}
LinkedHashMap<Class<? extends FragmentOptions>, Map<String, Object>> differingOptions =
new LinkedHashMap<>(diff.differingOptions.keySet().size());
- for (Class<? extends FragmentOptions> clazz : diff.differingOptions.keySet()) {
+ for (Class<? extends FragmentOptions> clazz :
+ diff.differingOptions
+ .keySet()
+ .stream()
+ .sorted(lexicalFragmentOptionsComparator)
+ .collect(Collectors.toList())) {
Collection<OptionDefinition> fields = diff.differingOptions.get(clazz);
LinkedHashMap<String, Object> valueMap = new LinkedHashMap<>(fields.size());
- for (OptionDefinition optionDefinition : fields) {
+ for (OptionDefinition optionDefinition :
+ fields
+ .stream()
+ .sorted(Comparator.comparing(o -> o.getField().getName()))
+ .collect(Collectors.toList())) {
Object secondValue;
try {
secondValue = Iterables.getOnlyElement(diff.second.get(optionDefinition));
@@ -448,8 +457,14 @@
first.maybeInitializeFingerprintAndHashCode();
return new OptionsDiffForReconstruction(
differingOptions,
- ImmutableSet.copyOf(diff.extraFirstFragments),
- ImmutableList.copyOf(diff.extraSecondFragments),
+ diff.extraFirstFragments
+ .stream()
+ .sorted(lexicalFragmentOptionsComparator)
+ .collect(ImmutableSet.toImmutableSet()),
+ diff.extraSecondFragments
+ .stream()
+ .sorted(Comparator.comparing(o -> o.getClass().getName()))
+ .collect(ImmutableList.toImmutableList()),
first.fingerprint,
second.computeChecksum());
}
@@ -480,14 +495,14 @@
extraSecondFragments.add(options);
}
- /** Return the extra fragments classes from the first configuration. */
- public Set<Class<? extends FragmentOptions>> getExtraFirstFragmentClasses() {
+ @VisibleForTesting
+ Set<Class<? extends FragmentOptions>> getExtraFirstFragmentClassesForTesting() {
return extraFirstFragments;
}
- /** Return the extra fragments classes from the second configuration. */
- public Set<Class<?>> getExtraSecondFragmentClasses() {
- return extraSecondFragments.stream().map(Object::getClass).collect(Collectors.toSet());
+ @VisibleForTesting
+ Set<FragmentOptions> getExtraSecondFragmentsForTesting() {
+ return extraSecondFragments;
}
public Map<OptionDefinition, Object> getFirst() {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKey.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKey.java
index f9ac4b5..fa9bbd7 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKey.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKey.java
@@ -170,12 +170,7 @@
@Override
public String toString() {
- return String.format(
- "%s %s %s (%s)",
- label,
- configurationKey,
- isHostConfiguration(),
- System.identityHashCode(this));
+ return String.format("%s %s %s", label, configurationKey, isHostConfiguration());
}
static class HostConfiguredTargetKey extends ConfiguredTargetKey {
diff --git a/src/test/java/com/google/devtools/build/lib/BUILD b/src/test/java/com/google/devtools/build/lib/BUILD
index 8876204..e8d568d 100644
--- a/src/test/java/com/google/devtools/build/lib/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/BUILD
@@ -699,6 +699,7 @@
":guava_junit_truth",
":test_runner",
":testutil",
+ "//src/main/java/com/google/devtools/build/lib:android-rules",
"//src/main/java/com/google/devtools/build/lib:bazel-main",
"//src/main/java/com/google/devtools/build/lib:bazel-rules",
"//src/main/java/com/google/devtools/build/lib:build-base",
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildOptionsTest.java b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildOptionsTest.java
index a8a120a..3f0f93a 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildOptionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildOptionsTest.java
@@ -17,10 +17,17 @@
import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.analysis.config.BuildOptions.OptionsDiff;
import com.google.devtools.build.lib.analysis.config.BuildOptions.OptionsDiffForReconstruction;
+import com.google.devtools.build.lib.rules.android.AndroidConfiguration;
import com.google.devtools.build.lib.rules.cpp.CppOptions;
+import com.google.devtools.build.lib.rules.java.JavaOptions;
+import com.google.devtools.build.lib.rules.proto.ProtoConfiguration;
+import com.google.devtools.build.lib.rules.python.PythonOptions;
+import com.google.devtools.build.lib.skyframe.serialization.testutils.TestUtils;
import com.google.devtools.common.options.OptionsParser;
+import java.util.stream.Collectors;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
@@ -100,8 +107,13 @@
OptionsDiff diff = BuildOptions.diff(one, two);
assertThat(diff.areSame()).isFalse();
- assertThat(diff.getExtraFirstFragmentClasses()).containsExactly(CppOptions.class);
- assertThat(diff.getExtraSecondFragmentClasses()).containsExactlyElementsIn(TEST_OPTIONS);
+ assertThat(diff.getExtraFirstFragmentClassesForTesting()).containsExactly(CppOptions.class);
+ assertThat(
+ diff.getExtraSecondFragmentsForTesting()
+ .stream()
+ .map(Object::getClass)
+ .collect(Collectors.toSet()))
+ .containsExactlyElementsIn(TEST_OPTIONS);
}
@Test
@@ -165,4 +177,40 @@
assertThat(otherFragment.applyDiff(BuildOptions.diffForReconstruction(otherFragment, one)))
.isEqualTo(one);
}
+
+ private static ImmutableList.Builder<Class<? extends FragmentOptions>> makeOptionsClassBuilder() {
+ return ImmutableList.<Class<? extends FragmentOptions>>builder()
+ .addAll(TEST_OPTIONS)
+ .add(CppOptions.class);
+ }
+
+ /**
+ * Tests that an {@link OptionsDiffForReconstruction} serializes stably. Unfortunately, still
+ * passes without fixes! (Perhaps more classes and diffs are needed?)
+ */
+ @Test
+ public void codecStability() throws Exception {
+ BuildOptions one =
+ BuildOptions.of(
+ makeOptionsClassBuilder()
+ .add(AndroidConfiguration.Options.class)
+ .add(ProtoConfiguration.Options.class)
+ .build());
+ BuildOptions two =
+ BuildOptions.of(
+ makeOptionsClassBuilder().add(JavaOptions.class).add(PythonOptions.class).build(),
+ "--cpu=k8",
+ "--compilation_mode=opt",
+ "--compiler=gcc",
+ "--copt=-Dfoo",
+ "--javacopt=--javacoption",
+ "--experimental_fix_deps_tool=fake",
+ "--build_python_zip=no",
+ "--force_python=py2");
+ OptionsDiffForReconstruction diff1 = BuildOptions.diffForReconstruction(one, two);
+ OptionsDiffForReconstruction diff2 = BuildOptions.diffForReconstruction(one, two);
+ assertThat(diff2).isEqualTo(diff1);
+ assertThat(TestUtils.toBytes(diff2, ImmutableMap.of()))
+ .isEqualTo(TestUtils.toBytes(diff1, ImmutableMap.of()));
+ }
}
