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
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 5629c83a..21fbab7 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.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 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 c7a068b..277552a 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.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.Queue;
import java.util.Set;
import java.util.TreeMap;
-
import javax.annotation.Nullable;
/**
@@ -165,13 +163,6 @@
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 @@
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 @@
host.compilationMode = CompilationMode.OPT;
host.isHost = true;
host.useDynamicConfigurations = useDynamicConfigurations;
+ host.commandLineBuildVariables = commandLineBuildVariables;
host.enforceConstraints = enforceConstraints;
if (fallback) {
@@ -1041,6 +1043,7 @@
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 @@
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 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 @@
* (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 1e14df9..a02c7a9 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 @@
// 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 @@
}
}
- // 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 @@
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 2195e3f..47cd385 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.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 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 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) {
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationTest.java b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationTest.java
index 873b6c4..fc60ff6 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationTest.java
@@ -30,14 +30,12 @@
import com.google.devtools.build.lib.rules.java.JavaConfiguration;
import com.google.devtools.build.lib.rules.objc.J2ObjcConfiguration;
import com.google.devtools.common.options.Options;
-
+import java.util.Map;
+import java.util.regex.Pattern;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
-import java.util.Map;
-import java.util.regex.Pattern;
-
/**
* Tests for {@link BuildConfiguration}.
*/
@@ -334,4 +332,46 @@
assertThat(config1.getFragment(J2ObjcConfiguration.class))
.isNotSameAs(config3.getFragment(J2ObjcConfiguration.class));
}
+
+ @Test
+ public void testCommandLineVariables() throws Exception {
+ BuildConfiguration config = create(
+ "--define", "a=b/c:d", "--define", "b=FOO", "--define", "DEFUN=Nope");
+ assertThat(config.getCommandLineBuildVariables().get("a")).isEqualTo("b/c:d");
+ assertThat(config.getCommandLineBuildVariables().get("b")).isEqualTo("FOO");
+ assertThat(config.getCommandLineBuildVariables().get("DEFUN")).isEqualTo("Nope");
+ }
+
+ // Regression test for bug #2518997:
+ // "--define in blazerc overrides --define from command line"
+ @Test
+ public void testCommandLineVariablesOverride() throws Exception {
+ BuildConfiguration config = create("--define", "a=b", "--define", "a=c");
+ assertThat(config.getCommandLineBuildVariables().get("a")).isEqualTo("c");
+ }
+
+ // This is really a test of option parsing, not command-line variable
+ // semantics.
+ @Test
+ public void testCommandLineVariablesWithFunnyCharacters() throws Exception {
+ BuildConfiguration config = create(
+ "--define", "foo=#foo",
+ "--define", "comma=a,b",
+ "--define", "space=foo bar",
+ "--define", "thing=a \"quoted\" thing",
+ "--define", "qspace=a\\ quoted\\ space",
+ "--define", "#a=pounda");
+ assertThat(config.getCommandLineBuildVariables().get("foo")).isEqualTo("#foo");
+ assertThat(config.getCommandLineBuildVariables().get("comma")).isEqualTo("a,b");
+ assertThat(config.getCommandLineBuildVariables().get("space")).isEqualTo("foo bar");
+ assertThat(config.getCommandLineBuildVariables().get("thing")).isEqualTo("a \"quoted\" thing");
+ assertThat(config.getCommandLineBuildVariables().get("qspace")).isEqualTo("a\\ quoted\\ space");
+ assertThat(config.getCommandLineBuildVariables().get("#a")).isEqualTo("pounda");
+ }
+
+ @Test
+ public void testHostDefine() throws Exception {
+ BuildConfiguration cfg = createHost("--define=foo=bar");
+ assertThat(cfg.getCommandLineBuildVariables().get("foo")).isEqualTo("bar");
+ }
}
