Transform options by parsing result.

Allows an existing FragmentOptions to be the base for a new one which is distinguished by a set of flags provided through an option parser (for example from the command line or a file).

Step 1/N towards the platforms mapping functionality for #6426

RELNOTES: None.
PiperOrigin-RevId: 238022069

Author: aragos

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

@@ -39,7 +39,9 @@
 import com.google.devtools.common.options.OptionsBase;
 import com.google.devtools.common.options.OptionsParser;
 import com.google.devtools.common.options.OptionsParsingException;
+import com.google.devtools.common.options.OptionsParsingResult;
 import com.google.devtools.common.options.OptionsProvider;
+import com.google.devtools.common.options.ParsedOptionDescription;
 import com.google.protobuf.ByteString;
 import com.google.protobuf.CodedInputStream;
 import com.google.protobuf.CodedOutputStream;
@@ -312,6 +314,76 @@ public BuildOptions applyDiff(OptionsDiffForReconstruction optionsDiff) {
     return builder.build();
   }
 
+  /**
+   * Applies any options set in the parsing result on top of these options, returning the resulting
+   * build options.
+   *
+   * <p>To preserve fragment trimming, this method will not expand the set of included native
+   * fragments. If the parsing result contains native options whose owning fragment is not part of
+   * these options they will be ignored (i.e. not set on the resulting options). Starlark options
+   * are not affected by this restriction.
+   *
+   * @param parsingResult any options that are being modified
+   * @return the new options after applying the parsing result to the original options
+   * @throws OptionsParsingException if a value in the parsing result cannot in fact be parsed
+   */
+  public BuildOptions applyParsingResult(OptionsParsingResult parsingResult)
+      throws OptionsParsingException {
+    Map<Class<? extends FragmentOptions>, FragmentOptions> modifiedFragments =
+        toModifiedFragments(parsingResult);
+
+    BuildOptions.Builder builder = builder();
+    for (Map.Entry<Class<? extends FragmentOptions>, FragmentOptions> classAndFragment :
+        fragmentOptionsMap.entrySet()) {
+      Class<? extends FragmentOptions> fragmentClass = classAndFragment.getKey();
+      if (modifiedFragments.containsKey(fragmentClass)) {
+        builder.addFragmentOptions(modifiedFragments.get(fragmentClass));
+      } else {
+        builder.addFragmentOptions(classAndFragment.getValue());
+      }
+    }
+
+    Map<Label, Object> starlarkOptions = new HashMap<>(skylarkOptionsMap);
+    Map<Label, Object> parsedStarlarkOptions =
+        labelizeStarlarkOptions(parsingResult.getStarlarkOptions());
+    for (Map.Entry<Label, Object> starlarkOption : parsedStarlarkOptions.entrySet()) {
+      starlarkOptions.put(starlarkOption.getKey(), starlarkOption.getValue());
+    }
+    builder.addStarlarkOptions(starlarkOptions);
+    return builder.build();
+  }
+
+  private Map<Class<? extends FragmentOptions>, FragmentOptions> toModifiedFragments(
+      OptionsParsingResult parsingResult) throws OptionsParsingException {
+    Map<Class<? extends FragmentOptions>, FragmentOptions> replacedOptions = new HashMap<>();
+    for (ParsedOptionDescription parsedOption : parsingResult.asListOfExplicitOptions()) {
+      OptionDefinition optionDefinition = parsedOption.getOptionDefinition();
+
+      // All options obtained from an options parser are guaranteed to have been defined in an
+      // FragmentOptions class.
+      @SuppressWarnings("unchecked")
+      Class<? extends FragmentOptions> fragmentOptionClass =
+          (Class<? extends FragmentOptions>) optionDefinition.getField().getDeclaringClass();
+
+      FragmentOptions originalFragment = fragmentOptionsMap.get(fragmentOptionClass);
+      if (originalFragment == null) {
+        // Preserve trimming by ignoring fragments not present in the original options.
+        continue;
+      }
+      FragmentOptions newOptions =
+          replacedOptions.computeIfAbsent(
+              fragmentOptionClass,
+              (Class<? extends FragmentOptions> k) -> originalFragment.clone());
+      try {
+        optionDefinition.getField().set(newOptions, parsedOption.getConvertedValue());
+      } catch (IllegalAccessException e) {
+        throw new IllegalStateException("Couldn't set " + optionDefinition.getField(), e);
+      }
+    }
+
+    return replacedOptions;
+  }
+
   /** Creates a builder object for BuildOptions */
   public static Builder builder() {
     return new Builder();

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

@@ -39,20 +39,21 @@
 /**
  * A test for {@link BuildOptions}.
  *
- * Currently this tests native options and skylark options completely
- * separately since these two types of options do not interact. In the future when we begin to
- * migrate native options to skylark options, the format of this test class will need to accommodate
- * that overlap.
+ * <p>Currently this tests native options and skylark options completely separately since these two
+ * types of options do not interact. In the future when we begin to migrate native options to
+ * skylark options, the format of this test class will need to accommodate that overlap.
  */
 @RunWith(JUnit4.class)
 public class BuildOptionsTest {
-  private static final ImmutableList<Class<? extends FragmentOptions>> TEST_OPTIONS =
+  private static final ImmutableList<Class<? extends FragmentOptions>> BUILD_CONFIG_OPTIONS =
       ImmutableList.of(BuildConfiguration.Options.class);
 
   @Test
   public void optionSetCaching() {
-    BuildOptions a = BuildOptions.of(TEST_OPTIONS, OptionsParser.newOptionsParser(TEST_OPTIONS));
-    BuildOptions b = BuildOptions.of(TEST_OPTIONS, OptionsParser.newOptionsParser(TEST_OPTIONS));
+    BuildOptions a =
+        BuildOptions.of(BUILD_CONFIG_OPTIONS, OptionsParser.newOptionsParser(BUILD_CONFIG_OPTIONS));
+    BuildOptions b =
+        BuildOptions.of(BUILD_CONFIG_OPTIONS, OptionsParser.newOptionsParser(BUILD_CONFIG_OPTIONS));
     // The cache keys of the OptionSets must be equal even if these are
     // different objects, if they were created with the same options (no options in this case).
     assertThat(b.toString()).isEqualTo(a.toString());
@@ -62,26 +63,27 @@ public void optionSetCaching() {
   @Test
   public void cachingSpecialCases() throws Exception {
     // You can add options here to test that their string representations are good.
-    String[] options = new String[] { "--run_under=//run_under" };
-    BuildOptions a = BuildOptions.of(TEST_OPTIONS, options);
-    BuildOptions b = BuildOptions.of(TEST_OPTIONS, options);
+    String[] options = new String[] {"--run_under=//run_under"};
+    BuildOptions a = BuildOptions.of(BUILD_CONFIG_OPTIONS, options);
+    BuildOptions b = BuildOptions.of(BUILD_CONFIG_OPTIONS, options);
     assertThat(b.toString()).isEqualTo(a.toString());
   }
 
   @Test
   public void optionsEquality() throws Exception {
-    String[] options1 = new String[] { "--compilation_mode=opt" };
-    String[] options2 = new String[] { "--compilation_mode=dbg" };
+    String[] options1 = new String[] {"--compilation_mode=opt"};
+    String[] options2 = new String[] {"--compilation_mode=dbg"};
     // Distinct instances with the same values are equal:
-    assertThat(BuildOptions.of(TEST_OPTIONS, options1))
-        .isEqualTo(BuildOptions.of(TEST_OPTIONS, options1));
+    assertThat(BuildOptions.of(BUILD_CONFIG_OPTIONS, options1))
+        .isEqualTo(BuildOptions.of(BUILD_CONFIG_OPTIONS, options1));
     // Same fragments, different values aren't equal:
     assertThat(
-            BuildOptions.of(TEST_OPTIONS, options1).equals(BuildOptions.of(TEST_OPTIONS, options2)))
+            BuildOptions.of(BUILD_CONFIG_OPTIONS, options1)
+                .equals(BuildOptions.of(BUILD_CONFIG_OPTIONS, options2)))
         .isFalse();
     // Same values, different fragments aren't equal:
     assertThat(
-            BuildOptions.of(TEST_OPTIONS, options1)
+            BuildOptions.of(BUILD_CONFIG_OPTIONS, options1)
                 .equals(
                     BuildOptions.of(
                         ImmutableList.<Class<? extends FragmentOptions>>of(
@@ -92,9 +94,9 @@ public void optionsEquality() throws Exception {
 
   @Test
   public void optionsDiff() throws Exception {
-    BuildOptions one = BuildOptions.of(TEST_OPTIONS, "--compilation_mode=opt", "cpu=k8");
-    BuildOptions two = BuildOptions.of(TEST_OPTIONS, "--compilation_mode=dbg", "cpu=k8");
-    BuildOptions three = BuildOptions.of(TEST_OPTIONS, "--compilation_mode=dbg", "cpu=k8");
+    BuildOptions one = BuildOptions.of(BUILD_CONFIG_OPTIONS, "--compilation_mode=opt", "cpu=k8");
+    BuildOptions two = BuildOptions.of(BUILD_CONFIG_OPTIONS, "--compilation_mode=dbg", "cpu=k8");
+    BuildOptions three = BuildOptions.of(BUILD_CONFIG_OPTIONS, "--compilation_mode=dbg", "cpu=k8");
 
     OptionsDiff diffOneTwo = BuildOptions.diff(one, two);
     OptionsDiff diffTwoThree = BuildOptions.diff(two, three);
@@ -111,23 +113,22 @@ public void optionsDiff() throws Exception {
   public void optionsDiff_differentFragments() throws Exception {
     BuildOptions one =
         BuildOptions.of(ImmutableList.<Class<? extends FragmentOptions>>of(CppOptions.class));
-    BuildOptions two = BuildOptions.of(TEST_OPTIONS);
+    BuildOptions two = BuildOptions.of(BUILD_CONFIG_OPTIONS);
 
     OptionsDiff diff = BuildOptions.diff(one, two);
 
     assertThat(diff.areSame()).isFalse();
     assertThat(diff.getExtraFirstFragmentClassesForTesting()).containsExactly(CppOptions.class);
     assertThat(
-            diff.getExtraSecondFragmentsForTesting()
-                .stream()
+            diff.getExtraSecondFragmentsForTesting().stream()
                 .map(Object::getClass)
                 .collect(Collectors.toSet()))
-        .containsExactlyElementsIn(TEST_OPTIONS);
+        .containsExactlyElementsIn(BUILD_CONFIG_OPTIONS);
   }
 
   @Test
   public void optionsDiff_nullOptionsThrow() throws Exception {
-    BuildOptions one = BuildOptions.of(TEST_OPTIONS, "--compilation_mode=opt", "cpu=k8");
+    BuildOptions one = BuildOptions.of(BUILD_CONFIG_OPTIONS, "--compilation_mode=opt", "cpu=k8");
     BuildOptions two = null;
     IllegalArgumentException e =
         assertThrows(IllegalArgumentException.class, () -> BuildOptions.diff(one, two));
@@ -149,8 +150,8 @@ public void optionsDiff_nullSecondValue() throws Exception {
 
   @Test
   public void optionsDiff_differentBaseThrowException() throws Exception {
-    BuildOptions one = BuildOptions.of(TEST_OPTIONS, "--compilation_mode=opt", "cpu=k8");
-    BuildOptions two = BuildOptions.of(TEST_OPTIONS, "--compilation_mode=dbg", "cpu=k8");
+    BuildOptions one = BuildOptions.of(BUILD_CONFIG_OPTIONS, "--compilation_mode=opt", "cpu=k8");
+    BuildOptions two = BuildOptions.of(BUILD_CONFIG_OPTIONS, "--compilation_mode=dbg", "cpu=k8");
     BuildOptions three = BuildOptions.of(ImmutableList.of(CppOptions.class), "--compiler=gcc");
     OptionsDiffForReconstruction diffForReconstruction =
         BuildOptions.diffForReconstruction(one, two);
@@ -163,8 +164,8 @@ public void optionsDiff_differentBaseThrowException() throws Exception {
 
   @Test
   public void optionsDiff_getEmptyAndApplyEmpty() throws Exception {
-    BuildOptions one = BuildOptions.of(TEST_OPTIONS, "--compilation_mode=opt", "cpu=k8");
-    BuildOptions two = BuildOptions.of(TEST_OPTIONS, "--compilation_mode=opt", "cpu=k8");
+    BuildOptions one = BuildOptions.of(BUILD_CONFIG_OPTIONS, "--compilation_mode=opt", "cpu=k8");
+    BuildOptions two = BuildOptions.of(BUILD_CONFIG_OPTIONS, "--compilation_mode=opt", "cpu=k8");
     OptionsDiffForReconstruction diffForReconstruction =
         BuildOptions.diffForReconstruction(one, two);
     BuildOptions reconstructed = one.applyDiff(diffForReconstruction);
@@ -173,8 +174,8 @@ public void optionsDiff_getEmptyAndApplyEmpty() throws Exception {
 
   @Test
   public void applyDiff_nativeOptions() throws Exception {
-    BuildOptions one = BuildOptions.of(TEST_OPTIONS, "--compilation_mode=opt", "cpu=k8");
-    BuildOptions two = BuildOptions.of(TEST_OPTIONS, "--compilation_mode=dbg", "cpu=k8");
+    BuildOptions one = BuildOptions.of(BUILD_CONFIG_OPTIONS, "--compilation_mode=opt", "cpu=k8");
+    BuildOptions two = BuildOptions.of(BUILD_CONFIG_OPTIONS, "--compilation_mode=dbg", "cpu=k8");
     BuildOptions reconstructedTwo = one.applyDiff(BuildOptions.diffForReconstruction(one, two));
     assertThat(reconstructedTwo).isEqualTo(two);
     assertThat(reconstructedTwo).isNotSameAs(two);
@@ -271,14 +272,14 @@ public void applyDiff_extraStarlarkOptions() throws Exception {
     BuildOptions two = BuildOptions.of(ImmutableMap.of(flagNameTwo, flagValue));
 
     BuildOptions reconstructedTwo = one.applyDiff(BuildOptions.diffForReconstruction(one, two));
-    
+
     assertThat(reconstructedTwo).isEqualTo(two);
     assertThat(reconstructedTwo).isNotSameAs(two);
   }
 
   private static ImmutableList.Builder<Class<? extends FragmentOptions>> makeOptionsClassBuilder() {
     return ImmutableList.<Class<? extends FragmentOptions>>builder()
-        .addAll(TEST_OPTIONS)
+        .addAll(BUILD_CONFIG_OPTIONS)
         .add(CppOptions.class);
   }
 
@@ -322,8 +323,8 @@ public void codecStability() throws Exception {
 
   @Test
   public void repeatedCodec() throws Exception {
-    BuildOptions one = BuildOptions.of(TEST_OPTIONS, "--compilation_mode=opt", "cpu=k8");
-    BuildOptions two = BuildOptions.of(TEST_OPTIONS, "--compilation_mode=dbg", "cpu=k8");
+    BuildOptions one = BuildOptions.of(BUILD_CONFIG_OPTIONS, "--compilation_mode=opt", "cpu=k8");
+    BuildOptions two = BuildOptions.of(BUILD_CONFIG_OPTIONS, "--compilation_mode=dbg", "cpu=k8");
     OptionsDiffForReconstruction diff = BuildOptions.diffForReconstruction(one, two);
     BuildOptions.OptionsDiffCache cache = new BuildOptions.FingerprintingKDiffToByteStringCache();
     assertThat(TestUtils.toBytes(diff, ImmutableMap.of(BuildOptions.OptionsDiffCache.class, cache)))
@@ -334,11 +335,56 @@ public void repeatedCodec() throws Exception {
   @Test
   public void testMultiValueOptionImmutability() throws Exception {
     BuildOptions options =
-        BuildOptions.of(TEST_OPTIONS, OptionsParser.newOptionsParser(TEST_OPTIONS));
+        BuildOptions.of(BUILD_CONFIG_OPTIONS, OptionsParser.newOptionsParser(BUILD_CONFIG_OPTIONS));
     BuildConfiguration.Options coreOptions = options.get(Options.class);
     assertThrows(
         UnsupportedOperationException.class,
         () ->
             coreOptions.commandLineBuildVariables.add(new AbstractMap.SimpleEntry<>("foo", "bar")));
   }
+
+  @Test
+  public void parsingResultTransform() throws Exception {
+    BuildOptions original = BuildOptions.of(BUILD_CONFIG_OPTIONS, "--cpu=foo");
+
+    OptionsParser parser = OptionsParser.newOptionsParser(BUILD_CONFIG_OPTIONS);
+    parser.parse("--cpu=bar");
+    parser.setStarlarkOptions(ImmutableMap.of("//custom:flag", "hello"));
+
+    BuildOptions modified = original.applyParsingResult(parser);
+
+    assertThat(original.get(BuildConfiguration.Options.class).cpu)
+        .isNotEqualTo(modified.get(BuildConfiguration.Options.class).cpu);
+    assertThat(modified.get(BuildConfiguration.Options.class).cpu).isEqualTo("bar");
+    assertThat(modified.getStarlarkOptions().get(Label.parseAbsoluteUnchecked("//custom:flag")))
+        .isEqualTo("hello");
+  }
+
+  @Test
+  public void parsingResultTransformNativeIgnored() throws Exception {
+    ImmutableList.Builder<Class<? extends FragmentOptions>> fragmentClassesBuilder =
+        ImmutableList.<Class<? extends FragmentOptions>>builder()
+            .add(BuildConfiguration.Options.class);
+
+    BuildOptions original = BuildOptions.of(fragmentClassesBuilder.build());
+
+    fragmentClassesBuilder.add(CppOptions.class);
+
+    OptionsParser parser = OptionsParser.newOptionsParser(fragmentClassesBuilder.build());
+    parser.parse("--cxxopt=bar");
+
+    BuildOptions modified = original.applyParsingResult(parser);
+
+    assertThat(modified.contains(CppOptions.class)).isFalse();
+  }
+
+  @Test
+  public void parsingResultTransformIllegalStarlarkLabel() throws Exception {
+    BuildOptions original = BuildOptions.of(BUILD_CONFIG_OPTIONS);
+
+    OptionsParser parser = OptionsParser.newOptionsParser(BUILD_CONFIG_OPTIONS);
+    parser.setStarlarkOptions(ImmutableMap.of("@@@", "hello"));
+
+    assertThrows(IllegalArgumentException.class, () -> original.applyParsingResult(parser));
+  }
 }