Move the --define command line option from CppConfiguration to BuildConfiguration.

It makes much more sense there because it does *not* specify C++ #defines but BUILD variables. Also rename the getter method to make that clearer.

This allows us to remove BuildConfiguration.Fragment#getCommandLineDefines().

--
MOS_MIGRATED_REVID=129080014

4 files changed, 25 insertions, 60 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfigurationMakeVariableContext.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfigurationMakeVariableContext.java
index 5629c83a60..21fbab7537 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfigurationMakeVariableContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfigurationMakeVariableContext.java
@@ -18,7 +18,6 @@ import com.google.devtools.build.lib.analysis.MakeVariableExpander.ExpansionExce
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.syntax.SkylarkDict;
-
 import java.util.LinkedHashMap;
 import java.util.Map;
 
@@ -35,7 +34,7 @@ public class ConfigurationMakeVariableContext implements MakeVariableExpander.Co
 
   public ConfigurationMakeVariableContext(Package pkg, BuildConfiguration configuration) {
     this.pkg = pkg;
-    commandLineEnv = configuration.getCommandLineDefines();
+    commandLineEnv = configuration.getCommandLineBuildVariables();
     globalEnv = configuration.getGlobalMakeEnvironment();
     platform = configuration.getPlatformName();
   }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
index c7a068b5a1..277552a353 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
@@ -68,7 +68,6 @@ import com.google.devtools.common.options.Option;
 import com.google.devtools.common.options.OptionsBase;
 import com.google.devtools.common.options.OptionsParsingException;
 import com.google.devtools.common.options.TriState;
-
 import java.io.Serializable;
 import java.lang.reflect.Field;
 import java.util.ArrayList;
@@ -85,7 +84,6 @@ import java.util.Objects;
 import java.util.Queue;
 import java.util.Set;
 import java.util.TreeMap;
-
 import javax.annotation.Nullable;
 
 /**
@@ -165,13 +163,6 @@ public final class BuildConfiguration {
       return implicitLabels;
     }
 
-    /*
-     * Returns the command-line "Make" variable overrides.
-     */
-    public ImmutableMap<String, String> getCommandLineDefines() {
-      return ImmutableMap.of();
-    }
-
     /**
      * Returns a fragment of the output directory name for this configuration. The output
      * directory for the whole configuration contains all the short names by all fragments.
@@ -451,6 +442,16 @@ public final class BuildConfiguration {
       return cpu;
     }
 
+    @Option(
+      name = "define",
+      converter = Converters.AssignmentConverter.class,
+      defaultValue = "",
+      category = "semantics",
+      allowMultiple = true,
+      help = "Each --define option specifies an assignment for a build variable."
+    )
+    public List<Map.Entry<String, String>> commandLineBuildVariables;
+
     @Option(name = "cpu",
         defaultValue = "null",
         category = "semantics",
@@ -862,6 +863,7 @@ public final class BuildConfiguration {
       host.compilationMode = CompilationMode.OPT;
       host.isHost = true;
       host.useDynamicConfigurations = useDynamicConfigurations;
+      host.commandLineBuildVariables = commandLineBuildVariables;
       host.enforceConstraints = enforceConstraints;
 
       if (fallback) {
@@ -1041,6 +1043,7 @@ public final class BuildConfiguration {
   private final String platformName;
 
   private final ImmutableMap<String, String> testEnvironment;
+  private final ImmutableMap<String, String> commandLineBuildVariables;
 
   /**
    * Helper container for {@link #transitiveOptionsMap} below.
@@ -1203,6 +1206,15 @@ public final class BuildConfiguration {
 
     this.testEnvironment = ImmutableMap.copyOf(testEnv);
 
+    // We can't use an ImmutableMap.Builder here; we need the ability to add entries with keys that
+    // are already in the map so that the same define can be specified on the command line twice,
+    // and ImmutableMap.Builder does not support that.
+    Map<String, String> commandLineDefinesBuilder = new TreeMap<>();
+    for (Map.Entry<String, String> define : options.commandLineBuildVariables) {
+      commandLineDefinesBuilder.put(define.getKey(), define.getValue());
+    }
+    commandLineBuildVariables = ImmutableMap.copyOf(commandLineDefinesBuilder);
+
     this.mnemonic = buildMnemonic();
     String outputDirName = (options.outputDirectoryName != null)
         ? options.outputDirectoryName : mnemonic;
@@ -2057,9 +2069,7 @@ public final class BuildConfiguration {
   public Map<String, String> getMakeEnvironment() {
     Map<String, String> makeEnvironment = new HashMap<>();
     makeEnvironment.putAll(globalMakeEnv);
-    for (Fragment fragment : fragments.values()) {
-      makeEnvironment.putAll(fragment.getCommandLineDefines());
-    }
+    makeEnvironment.putAll(commandLineBuildVariables);
     return ImmutableMap.copyOf(makeEnvironment);
   }
 
@@ -2068,12 +2078,8 @@ public final class BuildConfiguration {
    * (Fragments, in particular the Google C++ support, can set variables through the
    * command line.)
    */
-  public Map<String, String> getCommandLineDefines() {
-    ImmutableMap.Builder<String, String> builder = ImmutableMap.builder();
-    for (Fragment fragment : fragments.values()) {
-      builder.putAll(fragment.getCommandLineDefines());
-    }
-    return builder.build();
+  public Map<String, String> getCommandLineBuildVariables() {
+    return commandLineBuildVariables;
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
index 1e14df9dc6..a02c7a92e5 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
@@ -331,7 +331,6 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
   // The dynamic mode for linking.
   private final DynamicMode dynamicMode;
   private final boolean stripBinaries;
-  private final ImmutableMap<String, String> commandLineDefines;
   private final String solibDirectory;
   private final CompilationMode compilationMode;
 
@@ -457,15 +456,6 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
       }
     }
 
-    // We can't use an ImmutableMap.Builder here; we need the ability (at least
-    // in tests) to add entries with keys that are already in the map, and only
-    // HashMap supports this (by replacing the existing entry under the key).
-    Map<String, String> commandLineDefinesBuilder = new HashMap<>();
-    for (Map.Entry<String, String> define : cppOptions.commandLineDefinedVariables) {
-      commandLineDefinesBuilder.put(define.getKey(), define.getValue());
-    }
-    commandLineDefines = ImmutableMap.copyOf(commandLineDefinesBuilder);
-
     ListMultimap<CompilationMode, String> cFlags = ArrayListMultimap.create();
     ListMultimap<CompilationMode, String> cxxFlags = ArrayListMultimap.create();
     linkOptionsFromCompilationMode = ArrayListMultimap.create();
@@ -1576,22 +1566,6 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
     return cppOptions.stl;
   }
 
-  /*
-   * Returns the command-line "Make" variable overrides.
-   */
-  @Override
-  public ImmutableMap<String, String> getCommandLineDefines() {
-    return commandLineDefines;
-  }
-
-  /**
-   * Returns the command-line override value for the specified "Make" variable
-   * for this configuration, or null if none.
-   */
-  public String getMakeVariableOverride(String var) {
-    return commandLineDefines.get(var);
-  }
-
   /**
    * Returns the currently active LIPO compilation mode.
    */
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java
index 2195e3ff2d..47cd385670 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java
@@ -31,11 +31,9 @@ import com.google.devtools.build.lib.util.OptionsUtils;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.LipoMode;
 import com.google.devtools.common.options.Converter;
-import com.google.devtools.common.options.Converters;
 import com.google.devtools.common.options.EnumConverter;
 import com.google.devtools.common.options.Option;
 import com.google.devtools.common.options.OptionsParsingException;
-
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
@@ -497,16 +495,6 @@ public class CppOptions extends FragmentOptions {
   public List<String> hostCoptList;
 
   @Option(
-    name = "define",
-    converter = Converters.AssignmentConverter.class,
-    defaultValue = "",
-    category = "semantics",
-    allowMultiple = true,
-    help = "Each --define option specifies an assignment for a build variable."
-  )
-  public List<Map.Entry<String, String>> commandLineDefinedVariables;
-
-  @Option(
     name = "grte_top",
     defaultValue = "null", // The default value is chosen by the toolchain.
     category = "version",
@@ -572,8 +560,6 @@ public class CppOptions extends FragmentOptions {
   public FragmentOptions getHost(boolean fallback) {
     CppOptions host = (CppOptions) getDefault();
 
-    host.commandLineDefinedVariables = commandLineDefinedVariables;
-
     // The crosstool options are partially copied from the target configuration.
     if (!fallback) {
       if (hostCrosstoolTop == null) {