Verify that incompatible options are preserved in the exec cfg

Adding incompatible options is error-prone as it is easy to forget to
add them to their corresponding `FragmentOption`'s `getHost` method. If
this is not done, the flag will have no effect in the exec
configuration, which has already caused very surprising, buggy behavior
in numerous cases.

This commit adds a test to verify that all non-deprecated incompatible
flags:

1. are tagged with the `INCOMPATIBLE_CHANGE` metadata tag;
2. are preserved in the exec configuration.

Fixes bazelbuild#12238
Fixes bazelbuild#16388

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

@@ -481,6 +481,7 @@ public ExecConfigurationDistinguisherSchemeConverter() {
       defaultValue = "false",
       documentationCategory = OptionDocumentationCategory.INPUT_STRICTNESS,
       effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
+      metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
       help =
           "If enabled, check testonly for prerequisite targets that are output files by"
               + " looking up the testonly of the generating rule. This matches visibility"
@@ -921,6 +922,7 @@ public FragmentOptions getHost() {
     host.cpu = hostCpu;
     host.includeRequiredConfigFragmentsProvider = includeRequiredConfigFragmentsProvider;
     host.debugSelectsAlwaysSucceed = debugSelectsAlwaysSucceed;
+    host.checkTestonlyForOutputFiles = checkTestonlyForOutputFiles;
 
     // === Runfiles ===
     host.buildRunfilesManifests = buildRunfilesManifests;

src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java

@@ -1030,6 +1030,7 @@ public FragmentOptions getHost() {
       host.persistentBusyboxTools = persistentBusyboxTools;
       host.experimentalPersistentMultiplexBusyboxTools =
           experimentalPersistentMultiplexBusyboxTools;
+      host.disableNativeAndroidRules = disableNativeAndroidRules;
 
       // Unless the build was started from an Android device, host means MAIN.
       host.configurationDistinguisher = ConfigurationDistinguisher.MAIN;

src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagOptions.java

@@ -36,7 +36,6 @@ public final class ConfigFeatureFlagOptions extends FragmentOptions {
         OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION,
         OptionEffectTag.LOADING_AND_ANALYSIS
       },
-      metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
       defaultValue = "false")
   public boolean enforceTransitiveConfigsForConfigFeatureFlag = false;
 

src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java

@@ -1227,6 +1227,10 @@ public FragmentOptions getHost() {
     host.strictSystemIncludes = strictSystemIncludes;
     host.useArgsParamsFile = useArgsParamsFile;
     host.experimentalIncludeScanning = experimentalIncludeScanning;
+    host.renameDLL = renameDLL;
+    host.enableCcTestFeature = enableCcTestFeature;
+    host.forceStrictHeaderCheckFromStarlark = forceStrictHeaderCheckFromStarlark;
+    host.useCppCompileHeaderMnemonic = useCppCompileHeaderMnemonic;
 
     // Save host options for further use.
     host.hostCoptList = hostCoptList;

src/main/java/com/google/devtools/build/lib/rules/java/JavaOptions.java

@@ -569,6 +569,7 @@ public ImportDepsCheckingLevelConverter() {
       help = "The Java language version used to execute the tools that are needed during a build")
   public String hostJavaLanguageVersion;
 
+  @Deprecated
   @Option(
       name = "incompatible_dont_collect_native_libraries_in_data",
       defaultValue = "false",

src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCommandLineOptions.java

@@ -181,4 +181,12 @@ public class ObjcCommandLineOptions extends FragmentOptions {
       metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
       help = "No-op. Kept here for backwards compatibility.")
   public boolean incompatibleDisableNativeAppleBinaryRule;
+
+  @Override
+  public FragmentOptions getHost() {
+    ObjcCommandLineOptions host = (ObjcCommandLineOptions) getDefault();
+    host.enableCcDeps = enableCcDeps;
+    host.incompatibleAvoidHardcodedObjcCompilationFlags = incompatibleAvoidHardcodedObjcCompilationFlags;
+    return host;
+  }
 }

src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java

@@ -327,6 +327,11 @@ public FragmentOptions getHost() {
     hostPythonOptions.incompatiblePy2OutputsAreSuffixed = incompatiblePy2OutputsAreSuffixed;
     hostPythonOptions.buildPythonZip = buildPythonZip;
     hostPythonOptions.incompatibleUsePythonToolchains = incompatibleUsePythonToolchains;
+    hostPythonOptions.buildTransitiveRunfilesTrees = buildTransitiveRunfilesTrees;
+    hostPythonOptions.incompatibleAllowPythonVersionTransitions = incompatibleAllowPythonVersionTransitions;
+    hostPythonOptions.incompatibleDefaultToExplicitInitPy = incompatibleDefaultToExplicitInitPy;
+    hostPythonOptions.incompatibleDisallowLegacyPyProvider = incompatibleDisallowLegacyPyProvider;
+    hostPythonOptions.incompatibleRemoveOldPythonVersionApi = incompatibleRemoveOldPythonVersionApi;
 
     // Save host options in case of a further exec->host transition.
     hostPythonOptions.hostForcePython = hostForcePython;

src/test/java/com/google/devtools/build/lib/skyframe/packages/BUILD

@@ -35,13 +35,15 @@ java_test(
     srcs = ["BazelPackageLoaderTest.java"],
     deps = [
         ":AbstractPackageLoaderTest",
+        "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options",
         "//src/main/java/com/google/devtools/build/lib/analysis:server_directories",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/packages",
         "//src/main/java/com/google/devtools/build/lib/skyframe/packages:BazelPackageLoader",
         "//src/main/java/com/google/devtools/build/lib/skyframe/packages:PackageLoader",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
+        "//src/main/java/com/google/devtools/common/options:options_internal",
         "//src/test/java/com/google/devtools/build/lib/testutil:JunitUtils",
         "//third_party:junit4",
         "//third_party:truth",

src/test/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoaderTest.java

@@ -17,7 +17,11 @@
 import static com.google.devtools.build.lib.testutil.MoreAsserts.assertNoEvents;
 import static org.junit.Assert.assertThrows;
 
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMultimap;
+import com.google.common.collect.ImmutableSortedSet;
 import com.google.devtools.build.lib.analysis.ServerDirectories;
+import com.google.devtools.build.lib.analysis.config.FragmentOptions;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
@@ -27,7 +31,13 @@
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.Root;
+import com.google.devtools.common.options.OptionDefinition;
+import com.google.devtools.common.options.OptionMetadataTag;
+import com.google.devtools.common.options.Options;
+import com.google.devtools.common.options.OptionsParser;
 import java.io.IOException;
+import java.lang.reflect.Field;
+import java.util.Arrays;
 import java.util.concurrent.ForkJoinPool;
 import org.junit.Before;
 import org.junit.Test;
@@ -186,4 +196,49 @@ public void buildDotBazelForSubpackageCheckDuringGlobbing() throws Exception {
     assertThrows(NoSuchTargetException.class, () -> aPkg.getTarget("sub/a.txt"));
     assertNoEvents(handler.getEvents());
   }
+
+  @Test
+  public void incompatibleOptionsPreservedInExec() throws IllegalAccessException {
+    ImmutableMultimap.Builder<Class<? extends FragmentOptions>, OptionDefinition> missingMetadataTagOptions =
+        new ImmutableMultimap.Builder<>();
+    ImmutableMultimap.Builder<Class<? extends FragmentOptions>, OptionDefinition> unpreservedOptions =
+        new ImmutableMultimap.Builder<>();
+    ImmutableSortedSet<Class<? extends FragmentOptions>> allFragmentOptions =
+        newPackageLoaderBuilder().ruleClassProvider.getFragmentRegistry().getOptionsClasses();
+    for (Class<? extends FragmentOptions> optionsClass : allFragmentOptions) {
+      ImmutableList<OptionDefinition> incompatibleOptions = OptionsParser.getOptionDefinitions(
+              optionsClass).stream()
+          .filter(option -> Arrays.asList(option.getOptionMetadataTags())
+              .contains(OptionMetadataTag.INCOMPATIBLE_CHANGE) || option.getOptionName()
+              .startsWith("incompatible_"))
+          .filter(option -> option.getField().getType().isAssignableFrom(boolean.class))
+          .filter(option -> option.getField().getAnnotation(Deprecated.class) == null)
+          .collect(ImmutableList.toImmutableList());
+
+      // Verify that all --incompatible_* options have the INCOMPATIBLE_CHANGE metadata tag.
+      incompatibleOptions.stream()
+          .filter(option -> !Arrays.asList(option.getOptionMetadataTags())
+              .contains(OptionMetadataTag.INCOMPATIBLE_CHANGE))
+          .forEach(option -> missingMetadataTagOptions.put(optionsClass, option));
+
+      // Flip all incompatible (boolean) options to their non-default value.
+      FragmentOptions flipped = Options.getDefaults(optionsClass);
+      for (OptionDefinition incompatibleOption : incompatibleOptions) {
+        Field field = incompatibleOption.getField();
+        field.setBoolean(flipped, !field.getBoolean(flipped));
+      }
+
+      // Verify that the flipped value is preserved under an exec transition.
+      FragmentOptions flippedAfterExec = flipped.getHost();
+      for (OptionDefinition incompatibleOption : incompatibleOptions) {
+        Field field = incompatibleOption.getField();
+        if (field.getBoolean(flippedAfterExec) != field.getBoolean(flipped)) {
+          unpreservedOptions.put(optionsClass, incompatibleOption);
+        }
+      }
+    }
+
+    assertThat(missingMetadataTagOptions.build()).isEmpty();
+    assertThat(unpreservedOptions.build()).isEmpty();
+  }
 }