Remove alphabetical sorting of options in the canonical list.

This was broken for --config. Doing this properly requires keeping the order in which the options were given, which could be done either by filtering the ordered list according to which values affect the final outcome or by tracking the order correctly. I picked the later: the option order was not explicitly tracked for expansions before but now it is.

RELNOTES: canonicalize-flags no longer reorders the flags
PiperOrigin-RevId: 192132260

2 files changed, 22 insertions, 16 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/runtime/CommandLineEventTest.java b/src/test/java/com/google/devtools/build/lib/runtime/CommandLineEventTest.java
index fc51f94f9a..6c7f2929f1 100644
--- a/src/test/java/com/google/devtools/build/lib/runtime/CommandLineEventTest.java
+++ b/src/test/java/com/google/devtools/build/lib/runtime/CommandLineEventTest.java
@@ -283,16 +283,16 @@ public class CommandLineEventTest {
     assertThat(line.getSections(0).getChunkList().getChunk(0)).isEqualTo("testblaze");
     assertThat(line.getSections(1).getOptionList().getOptionCount()).isEqualTo(2);
     assertThat(line.getSections(2).getChunkList().getChunk(0)).isEqualTo("someCommandName");
-    // In the canonical line, expect the options in alphabetical order.
+    // In the canonical line, expect the options in priority order.
     assertThat(line.getSections(3).getOptionList().getOptionCount()).isEqualTo(4);
     assertThat(line.getSections(3).getOptionList().getOption(0).getCombinedForm())
-        .isEqualTo("--expanded_c=2");
-    assertThat(line.getSections(3).getOptionList().getOption(1).getCombinedForm())
         .isEqualTo("--test_multiple_string=baz");
+    assertThat(line.getSections(3).getOptionList().getOption(1).getCombinedForm())
+        .isEqualTo("--test_string=foo");
     assertThat(line.getSections(3).getOptionList().getOption(2).getCombinedForm())
         .isEqualTo("--test_multiple_string=bar");
     assertThat(line.getSections(3).getOptionList().getOption(3).getCombinedForm())
-        .isEqualTo("--test_string=foo");
+        .isEqualTo("--expanded_c=2");
     assertThat(line.getSections(4).getChunkList().getChunkCount()).isEqualTo(0);
   }
 
diff --git a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java
index 33baef25d3..b0e37586d3 100644
--- a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java
+++ b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java
@@ -1710,31 +1710,37 @@ public class OptionsParserTest {
   }
 
   @Test
-  public void canonicalizeSorts() throws Exception {
-    assertThat(canonicalize(Yesterday.class, "--b=y", "--a=x"))
-        .containsExactly("--a=x", "--b=y").inOrder();
+  public void canonicalizeDoesNotReorder() throws Exception {
+    assertThat(canonicalize(Yesterday.class, "--b=y", "--d=x", "--a=z"))
+        .containsExactly("--b=y", "--d=x", "--a=z")
+        .inOrder();
   }
 
   @Test
   public void canonicalizeImplicitDepsNotListed() throws Exception {
     // e's requirement overrides the explicit "a" here, so the "a" value is not in the canonical
     // form - the effective value is implied and the overridden value is lost.
-    assertThat(canonicalize(Yesterday.class, "--a=x", "--e=y"))
-        .isEqualTo(Arrays.asList("--e=y"));
+    assertThat(canonicalize(Yesterday.class, "--a=x", "--e=y")).containsExactly("--e=y");
   }
 
   @Test
-  public void canonicalizeImplicitDepsSkipsDuplicate() throws Exception {
+  public void canonicalizeSkipsDuplicateAndStillOmmitsImplicitDeps() throws Exception {
     assertThat(canonicalize(Yesterday.class, "--e=x", "--e=y")).containsExactly("--e=y");
   }
 
   @Test
-  public void implicitDepsDoNotAffectCanonicalOrder() throws Exception {
-    // e requires a value of a that is overridden and should therefore be absent.
-    // f requires a value of b, that is absent because it is implied. Neither of these affects
-    // the order of the canonical list.
-    assertThat(canonicalize(Yesterday.class, "--f=z", "--e=y", "--a=x"))
-        .containsExactly("--a=x", "--e=y", "--f=z").inOrder();
+  public void implicitDepsAreNotInTheCanonicalOrderWhenTheyAreOverridden() throws Exception {
+    assertThat(canonicalize(Yesterday.class, "--e=y", "--a=x"))
+        .containsExactly("--e=y", "--a=x")
+        .inOrder();
+  }
+
+  @Test
+  public void implicitDepsAreNotInTheCanonicalOrder() throws Exception {
+    // f requires a value of b, that is absent because it is implied.
+    assertThat(canonicalize(Yesterday.class, "--f=z", "--a=x"))
+        .containsExactly("--f=z", "--a=x")
+        .inOrder();
   }
 
   @Test