Move --collapse_duplicate_defines to the graveyard.

It has been enabled for quite a while and there is no reason to disable it.

PiperOrigin-RevId: 546235453
Change-Id: I897056960da937b7f43e1a3b6265fcd537076182

src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java

@@ -119,20 +119,6 @@ public class CoreOptions extends FragmentOptions implements Cloneable {
       help = "Each --define option specifies an assignment for a build variable.")
   public List<Map.Entry<String, String>> commandLineBuildVariables;
 
-  @Option(
-      name = "collapse_duplicate_defines",
-      defaultValue = "true",
-      documentationCategory = OptionDocumentationCategory.BUILD_TIME_OPTIMIZATION,
-      effectTags = {
-        OptionEffectTag.LOADING_AND_ANALYSIS,
-        OptionEffectTag.LOSES_INCREMENTAL_STATE,
-      },
-      help =
-          "When enabled, redundant --defines will be removed early in the build. This avoids"
-              + " unnecessary loss of the analysis cache for certain types of equivalent"
-              + " builds.")
-  public boolean collapseDuplicateDefines;
-
   @Option(
       name = "cpu",
       defaultValue = "",
@@ -1114,20 +1100,18 @@ private static List<String> getNormalizedFeatures(List<String> features) {
   @Override
   public CoreOptions getNormalized() {
     CoreOptions result = (CoreOptions) clone();
-
-    if (collapseDuplicateDefines) {
-      LinkedHashMap<String, String> flagValueByName = getNormalizedCommandLineBuildVariables();
-
-      // This check is an optimization to avoid creating a new list if the normalization was a
-      // no-op.
-      if (flagValueByName.size() != result.commandLineBuildVariables.size()) {
-        result.commandLineBuildVariables =
-            flagValueByName.entrySet().stream()
-                // The entries in the transformed list must be serializable.
-                .map(SimpleEntry::new)
-                .collect(toImmutableList());
-      }
+    LinkedHashMap<String, String> flagValueByName = getNormalizedCommandLineBuildVariables();
+
+    // This check is an optimization to avoid creating a new list if the normalization was a
+    // no-op.
+    if (flagValueByName.size() != result.commandLineBuildVariables.size()) {
+      result.commandLineBuildVariables =
+          flagValueByName.entrySet().stream()
+              // The entries in the transformed list must be serializable.
+              .map(SimpleEntry::new)
+              .collect(toImmutableList());
     }
+
     // Normalize features.
     result.defaultFeatures = getNormalizedFeatures(defaultFeatures);
 

src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java

@@ -503,6 +503,17 @@ public static class BuildGraveyardOptions extends OptionsBase {
         effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION},
         deprecationWarning = "This flag is a no-op and skyframe-native-filesets is always true.")
     public boolean skyframeNativeFileset;
+
+    @Option(
+        name = "collapse_duplicate_defines",
+        defaultValue = "true",
+        documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+        effectTags = {
+          OptionEffectTag.LOADING_AND_ANALYSIS,
+          OptionEffectTag.LOSES_INCREMENTAL_STATE,
+        },
+        help = "no-op")
+    public boolean collapseDuplicateDefines;
   }
 
   /** This is where deprecated Bazel-specific options only used by the build command go to die. */

src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java

@@ -877,26 +877,13 @@ public void cacheNotClearedWhenAllowedOptionsChange() throws Exception {
             .build());
   }
 
-  @Test
-  public void cacheClearedWhenRedundantDefinesChange_collapseDuplicateDefinesDisabled()
-      throws Exception {
-    setupDiffResetTesting();
-    scratch.file("test/BUILD", "load(':lib.bzl', 'normal_lib')", "normal_lib(name='top')");
-    useConfiguration("--nocollapse_duplicate_defines", "--define=a=1", "--define=a=2");
-    update("//test:top");
-    assertNumberOfAnalyzedConfigurationsOfTargets(ImmutableMap.of("//test:top", 2));
-    useConfiguration("--nocollapse_duplicate_defines", "--define=a=2");
-    update("//test:top");
-    assertNumberOfAnalyzedConfigurationsOfTargets(ImmutableMap.of("//test:top", 2));
-  }
-
   @Test
   public void cacheNotClearedWhenRedundantDefinesChange() throws Exception {
     setupDiffResetTesting();
     scratch.file("test/BUILD", "load(':lib.bzl', 'normal_lib')", "normal_lib(name='top')");
-    useConfiguration("--collapse_duplicate_defines", "--define=a=1", "--define=a=2");
+    useConfiguration("--define=a=1", "--define=a=2");
     update("//test:top");
-    useConfiguration("--collapse_duplicate_defines", "--define=a=2");
+    useConfiguration("--define=a=2");
     update("//test:top");
     assertNumberOfAnalyzedConfigurationsOfTargets(ImmutableMap.of("//test:top", 0));
   }

src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValueTest.java

@@ -17,7 +17,6 @@
 import static org.junit.Assert.assertThrows;
 
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.testing.EqualsTester;
 import com.google.devtools.build.lib.analysis.config.BuildOptions.MapBackedChecksumCache;
@@ -182,34 +181,20 @@ public void testCommandLineVariablesOverride() throws Exception {
     assertThat(config.getCommandLineBuildVariables().get("a")).isEqualTo("c");
   }
 
-  @Test
-  public void testNormalization_definesWithSameName_collapseDuplicateDefinesDisabled()
-      throws Exception {
-    BuildConfigurationValue config =
-        create("--nocollapse_duplicate_defines", "--define", "a=1", "--define", "a=2");
-    CoreOptions options = config.getOptions().get(CoreOptions.class);
-    assertThat(ImmutableListMultimap.copyOf(options.commandLineBuildVariables))
-        .containsExactly("a", "1", "a", "2")
-        .inOrder();
-    assertThat(config).isNotEqualTo(create("--nocollapse_duplicate_defines", "--define", "a=2"));
-  }
-
   @Test
   public void testNormalization_definesWithDifferentNames() throws Exception {
-    BuildConfigurationValue config =
-        create("--collapse_duplicate_defines", "--define", "a=1", "--define", "b=2");
+    BuildConfigurationValue config = create("--define", "a=1", "--define", "b=2");
     CoreOptions options = config.getOptions().get(CoreOptions.class);
     assertThat(ImmutableMap.copyOf(options.commandLineBuildVariables))
         .containsExactly("a", "1", "b", "2");
   }
 
   @Test
   public void testNormalization_definesWithSameName() throws Exception {
-    BuildConfigurationValue config =
-        create("--collapse_duplicate_defines", "--define", "a=1", "--define", "a=2");
+    BuildConfigurationValue config = create("--define", "a=1", "--define", "a=2");
     CoreOptions options = config.getOptions().get(CoreOptions.class);
     assertThat(ImmutableMap.copyOf(options.commandLineBuildVariables)).containsExactly("a", "2");
-    assertThat(config).isEqualTo(create("--collapse_duplicate_defines", "--define", "a=2"));
+    assertThat(config).isEqualTo(create("--define", "a=2"));
   }
 
   // This is really a test of option parsing, not command-line variable

src/test/java/com/google/devtools/build/lib/analysis/config/CoreOptionsTest.java

@@ -13,7 +13,6 @@
 // limitations under the License.
 package com.google.devtools.build.lib.analysis.config;
 
-import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.analysis.util.OptionsTestCase;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -26,11 +25,6 @@ public final class CoreOptionsTest extends OptionsTestCase<CoreOptions> {
   private static final String FEATURES_PREFIX = "--features=";
   private static final String DEFINE_PREFIX = "--define=";
 
-  private static final ImmutableList<String> NO_COLLAPSE_DEFINES =
-      ImmutableList.of("--nocollapse_duplicate_defines");
-  private static final ImmutableList<String> COLLAPSE_DEFINES =
-      ImmutableList.of("--collapse_duplicate_defines");
-
   @Test
   public void testFeatures_orderingOfPositiveFeatures() throws Exception {
     CoreOptions one = createWithPrefix(FEATURES_PREFIX, "foo", "bar");
@@ -52,26 +46,10 @@ public void testFeatures_disablingWins() throws Exception {
     assertSame(one, two);
   }
 
-  @Test
-  public void testDefines_ordering_effectOnCacheKey() throws Exception {
-    CoreOptions one = createWithPrefix(NO_COLLAPSE_DEFINES, DEFINE_PREFIX, "a=1", "b=2");
-    CoreOptions two = createWithPrefix(NO_COLLAPSE_DEFINES, DEFINE_PREFIX, "b=2", "a=1");
-    // Due to NO_COLLAPSE_DEFINES, the two configurations are not equivalent.
-    assertDifferent(one, two);
-  }
-
-  @Test
-  public void testDefines_duplicates_effectOnCacheKey() throws Exception {
-    CoreOptions one = createWithPrefix(NO_COLLAPSE_DEFINES, DEFINE_PREFIX, "a=2", "a=2");
-    CoreOptions two = createWithPrefix(NO_COLLAPSE_DEFINES, DEFINE_PREFIX, "a=2");
-    // Due to NO_COLLAPSE_DEFINES, the two configurations are not equivalent.
-    assertDifferent(one, two);
-  }
-
   @Test
   public void testDefines_duplicatesWithCollapsingEnabled_effectOnCacheKey() throws Exception {
-    CoreOptions one = createWithPrefix(COLLAPSE_DEFINES, DEFINE_PREFIX, "a=1", "a=2");
-    CoreOptions two = createWithPrefix(COLLAPSE_DEFINES, DEFINE_PREFIX, "a=2");
+    CoreOptions one = createWithPrefix(DEFINE_PREFIX, "a=1", "a=2");
+    CoreOptions two = createWithPrefix(DEFINE_PREFIX, "a=2");
     assertSame(one, two);
   }