Verify that incompatible options are preserved in the exec cfg

Adding incompatible options is error-prone as adding them to their
corresponding `FragmentOption`'s `getHost` method is easily forgotten.
In that case, 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();
+  }
 }