Change config expansion application order, gated by startup flag --expand_configs_in_place.

--config options were expanded in a fix-point expansion, where in practice, the flags that --config values expanded to ended up between the normal bazelrc options and the command line's explicit options. Since the options parser has an order-based priority scheme and it accepts multiple mentions of a single-valued option, this conflicts with users' expectations of being able to override these config expansions by using the order in which they are mentioned.

This change makes it possible to expand the config values defined in your bazelrc (or blazerc) files to occur in-place: --stuff --config=something --laterstuff will interpret the options that --config=something expands to as if they had been mentioned explicitly between --stuff and --laterstuff.

In order to not break users relying on complex flag combinations to configure their builds, this behavior will not yet be turned on by default. Instead, use --expand_configs_in_place as a startup flag to test this feature. --announce_rc may be helpful for debugging any differences between the fixed point and in-place expansions. Once you've debugged your problems, add "startup --expand_configs_in_place" to your blazerc to stick to the new behavior.

RELNOTES: Use --expand_configs_in_place as a startup argument to change the order in which --config expansions are interpreted.
PiperOrigin-RevId: 176371289

3 files changed, 160 insertions, 5 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
index 2ed29f3613..7653fd892f 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
@@ -267,7 +267,11 @@ public class BlazeCommandDispatcher {
             commandAnnotation,
             // Provide the options parser so that we can cache OptionsData here.
             createOptionsParser(command),
-            invocationPolicy);
+            invocationPolicy,
+            runtime
+                .getStartupOptionsProvider()
+                .getOptions(BlazeServerStartupOptions.class)
+                .expandConfigsInPlace);
     ExitCode earlyExitCode = optionHandler.parseOptions(args, storedEventHandler);
     OptionsProvider options = optionHandler.getOptionsResult();
 
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java
index 248ba4578b..a2ff946635 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java
@@ -31,9 +31,11 @@ import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.common.options.InvocationPolicyEnforcer;
 import com.google.devtools.common.options.OptionDefinition;
 import com.google.devtools.common.options.OptionPriority.PriorityCategory;
+import com.google.devtools.common.options.OptionValueDescription;
 import com.google.devtools.common.options.OptionsParser;
 import com.google.devtools.common.options.OptionsParsingException;
 import com.google.devtools.common.options.OptionsProvider;
+import com.google.devtools.common.options.ParsedOptionDescription;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -90,9 +92,15 @@ public abstract class BlazeOptionHandler {
       BlazeCommand command,
       Command commandAnnotation,
       OptionsParser optionsParser,
-      InvocationPolicy invocationPolicy) {
-    return new FixPointConfigExpansionRcHandler(
-        runtime, workspace, command, commandAnnotation, optionsParser, invocationPolicy);
+      InvocationPolicy invocationPolicy,
+      boolean expandConfigsInPlace) {
+    if (expandConfigsInPlace) {
+      return new InPlaceConfigExpansionRcHandler(
+          runtime, workspace, command, commandAnnotation, optionsParser, invocationPolicy);
+    } else {
+      return new FixPointConfigExpansionRcHandler(
+          runtime, workspace, command, commandAnnotation, optionsParser, invocationPolicy);
+    }
   }
 
   // Return options as OptionsProvider so the options can't be easily modified after we've
@@ -321,7 +329,7 @@ public abstract class BlazeOptionHandler {
    * necessary, but this is not the case. This is why we are phasing out this behavior. We will
    * maintain the old behavior to avoid breaking users during a transition period.
    */
-  public static final class FixPointConfigExpansionRcHandler extends BlazeOptionHandler {
+  static final class FixPointConfigExpansionRcHandler extends BlazeOptionHandler {
 
     private FixPointConfigExpansionRcHandler(
         BlazeRuntime runtime,
@@ -401,6 +409,136 @@ public abstract class BlazeOptionHandler {
     }
   }
 
+  /**
+   * Expands the rc's options in {@code <rc options> <command line>} order, with config options
+   * expanded wherever the --config option was mentioned.
+   *
+   * <p>This requires a bit more care than the in-place expansion. We need to be wary of potential
+   * cycles in the definitions of config values, and should warn when a single --config expansion
+   * contradicts itself.
+   */
+  static final class InPlaceConfigExpansionRcHandler extends BlazeOptionHandler {
+
+    private InPlaceConfigExpansionRcHandler(
+        BlazeRuntime runtime,
+        BlazeWorkspace workspace,
+        BlazeCommand command,
+        Command commandAnnotation,
+        OptionsParser optionsParser,
+        InvocationPolicy invocationPolicy) {
+      super(runtime, workspace, command, commandAnnotation, optionsParser, invocationPolicy);
+    }
+
+    @Override
+    void expandConfigOptions(
+        EventHandler eventHandler, ListMultimap<String, RcChunkOfArgs> commandToRcArgs)
+        throws OptionsParsingException {
+
+      OptionValueDescription configValueDescription =
+          optionsParser.getOptionValueDescription("config");
+      if (configValueDescription == null
+          || configValueDescription.getCanonicalInstances() == null) {
+        // No --config values were set, we can avoid this whole thing.
+        return;
+      }
+
+      // Find the base set of configs. This does not include the config options that might be
+      // recursively incuded.
+      ImmutableList<ParsedOptionDescription> configInstances =
+          ImmutableList.copyOf(configValueDescription.getCanonicalInstances());
+
+      // Expand the configs that are mentioned in the input. Flatten these expansions before parsing
+      // them, to preserve order.
+      for (ParsedOptionDescription configInstance : configInstances) {
+        String configValueToExpand = (String) configInstance.getConvertedValue();
+        List<String> expansion = getExpansion(eventHandler, commandToRcArgs, configValueToExpand);
+        optionsParser.parseOptionsFixedAtSpecificPriority(
+            configInstance.getPriority(),
+            String.format("expanded from --%s", configValueToExpand),
+            expansion);
+      }
+    }
+
+    private List<String> getExpansion(
+        EventHandler eventHandler,
+        ListMultimap<String, RcChunkOfArgs> commandToRcArgs,
+        String configToExpand)
+        throws OptionsParsingException {
+      LinkedHashSet<String> configAncestorSet = new LinkedHashSet<>();
+      configAncestorSet.add(configToExpand);
+      return getExpansion(eventHandler, commandToRcArgs, configAncestorSet, configToExpand);
+    }
+
+    /**
+     * @param configAncestorSet is the chain of configs that have led to this one getting expanded.
+     *     This should only contain the configs that expanded, recursively, to this one, and should
+     *     not contain "siblings," as it is used to detect cycles. {@code build:foo --config=bar},
+     *     {@code build:bar --config=foo}, is a cycle, detected because this list will be [foo, bar]
+     *     when we find another 'foo' to expand. However, {@code build:foo --config=bar}, {@code
+     *     build:foo --config=bar} is not a cycle just because bar is expanded twice, and the 1st
+     *     bar should not be in the parents list of the second bar.
+     */
+    private List<String> getExpansion(
+        EventHandler eventHandler,
+        ListMultimap<String, RcChunkOfArgs> commandToRcArgs,
+        LinkedHashSet<String> configAncestorSet,
+        String configToExpand)
+        throws OptionsParsingException {
+      List<String> expansion = new ArrayList<>();
+      boolean foundDefinition = false;
+      // The expansion order of rc files is first by command priority, and then in the order the
+      // rc files were read, respecting import statement placement.
+      for (String commandToParse : getCommandNamesToParse(commandAnnotation)) {
+        String configDef = commandToParse + ":" + configToExpand;
+        for (RcChunkOfArgs rcArgs : commandToRcArgs.get(configDef)) {
+          foundDefinition = true;
+          rcfileNotes.add(
+              String.format(
+                  "Found applicable config definition %s in file %s: %s",
+                  configDef, rcArgs.rcFile, String.join(" ", rcArgs.args)));
+
+          // For each arg in the rcARgs chunk, we first check if it is a config, and if so, expand
+          // it in place. We avoid cycles by tracking the parents of this config.
+          for (String arg : rcArgs.args) {
+            expansion.add(arg);
+            if (arg.length() >= 8 && arg.substring(0, 8).equals("--config")) {
+              // We have a config. For sanity, because we don't want to worry about formatting,
+              // we will only accept --config=value, and will not accept value on a following line.
+              int charOfConfigValue = arg.indexOf('=');
+              if (charOfConfigValue < 0) {
+                throw new OptionsParsingException("Config flag was provided without a value.");
+              }
+              String newConfigValue = arg.substring(charOfConfigValue + 1);
+              LinkedHashSet<String> extendedConfigAncestorSet =
+                  new LinkedHashSet<>(configAncestorSet);
+              if (!extendedConfigAncestorSet.add(newConfigValue)) {
+                throw new OptionsParsingException(
+                    String.format(
+                        "Config expansion has a cycle: config value %s expands to itself, "
+                            + "see inheritance chain %s",
+                        newConfigValue, extendedConfigAncestorSet));
+              }
+              expansion.addAll(
+                  getExpansion(
+                      eventHandler, commandToRcArgs, extendedConfigAncestorSet, newConfigValue));
+            }
+          }
+        }
+      }
+
+      if (!foundDefinition) {
+        String warning = "Config value " + configToExpand + " is not defined in any .rc file";
+        CommonCommandOptions commonOptions = optionsParser.getOptions(CommonCommandOptions.class);
+        if (commonOptions.allowUndefinedConfigs) {
+          eventHandler.handle(Event.warn(warning));
+        } else {
+          throw new OptionsParsingException(warning);
+        }
+      }
+      return expansion;
+    }
+  }
+
   private static List<String> getCommandNamesToParse(Command commandAnnotation) {
     List<String> result = new ArrayList<>();
     result.add("common");
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java
index 6d96793d73..ce75d07d5c 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java
@@ -408,4 +408,17 @@ public class BlazeServerStartupOptions extends OptionsBase {
     help = "The amount of time the client waits for each attempt to connect to the server"
   )
   public int connectTimeoutSecs;
+
+  @Option(
+    name = "expand_configs_in_place",
+    defaultValue = "false",
+    category = "server startup",
+    documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS,
+    effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION, OptionEffectTag.CHANGES_INPUTS},
+    metadataTags = {OptionMetadataTag.EXPERIMENTAL},
+    help =
+        "Changes the expansion of --config flags to be done in-place, as opposed to in a fixed "
+            + "point expansion between normal rc options and command-line specified options."
+  )
+  public boolean expandConfigsInPlace;
 }