config doesn't error on duplicate `--define` values (#15473)
When `--define` flags are interpreted by other commands, the last one
wins. Currently `bazel config` causes a server crash when interpreting
duplicate `--define` values.
This change makes `config` consistent with the other commands, and
re-uses the same deduplication code across all call-sites.
Closes #14760.
PiperOrigin-RevId: 427266039
Co-authored-by: Daniel Wagner-Hall <dwagnerhall@apple.com>
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 af8bc01..34be910 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
@@ -46,7 +46,6 @@
import java.util.Map;
import java.util.Objects;
import java.util.Set;
-import java.util.TreeMap;
import java.util.function.Supplier;
import net.starlark.java.annot.StarlarkAnnotations;
import net.starlark.java.annot.StarlarkBuiltin;
@@ -215,11 +214,8 @@
// 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);
+ commandLineBuildVariables =
+ ImmutableMap.copyOf(options.getNormalizedCommandLineBuildVariables());
this.actionEnv = actionEnvironment;
this.testEnv = setupTestEnvironment();
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java
index 5203a88..c2d17a4 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java
@@ -87,6 +87,9 @@
help = "If true, constraint settings from @bazel_tools are removed.")
public boolean usePlatformsRepoForConstraints;
+ // Note: This value may contain conflicting duplicate values for the same define.
+ // Use `getNormalizedCommandLineBuildVariables` if you wish for these to be deduplicated
+ // (last-wins).
@Option(
name = "define",
converter = Converters.AssignmentConverter.class,
@@ -878,16 +881,22 @@
return host;
}
+ /// Normalizes --define flags, preserving the last one to appear in the event of conflicts.
+ public LinkedHashMap<String, String> getNormalizedCommandLineBuildVariables() {
+ LinkedHashMap<String, String> flagValueByName = new LinkedHashMap<>();
+ for (Map.Entry<String, String> entry : commandLineBuildVariables) {
+ // If the same --define flag is passed multiple times we keep the last value.
+ flagValueByName.put(entry.getKey(), entry.getValue());
+ }
+ return flagValueByName;
+ }
+
@Override
public CoreOptions getNormalized() {
CoreOptions result = (CoreOptions) clone();
if (collapseDuplicateDefines) {
- LinkedHashMap<String, String> flagValueByName = new LinkedHashMap<>();
- for (Map.Entry<String, String> entry : result.commandLineBuildVariables) {
- // If the same --define flag is passed multiple times we keep the last value.
- flagValueByName.put(entry.getKey(), entry.getValue());
- }
+ LinkedHashMap<String, String> flagValueByName = getNormalizedCommandLineBuildVariables();
// This check is an optimization to avoid creating a new list if the normalization was a
// no-op.
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/ConfigCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/ConfigCommand.java
index 38f7361..3e848d2 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/ConfigCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/ConfigCommand.java
@@ -557,7 +557,11 @@
// --define:
for (Map.Entry<String, String> entry :
- config.getOptions().get(CoreOptions.class).commandLineBuildVariables) {
+ config
+ .getOptions()
+ .get(CoreOptions.class)
+ .getNormalizedCommandLineBuildVariables()
+ .entrySet()) {
ans.put("--define:" + entry.getKey(), Verify.verifyNotNull(entry.getValue()));
}
return ans.build();
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/commands/ConfigCommandTest.java b/src/test/java/com/google/devtools/build/lib/runtime/commands/ConfigCommandTest.java
index bc7e069..2c4a0b6 100644
--- a/src/test/java/com/google/devtools/build/lib/runtime/commands/ConfigCommandTest.java
+++ b/src/test/java/com/google/devtools/build/lib/runtime/commands/ConfigCommandTest.java
@@ -427,4 +427,29 @@
.collect(Collectors.toList()))
.isEmpty();
}
+
+ @Test
+ public void conflictingDefinesLastWins() throws Exception {
+ analyzeTarget("--define", "a=1", "--define", "a=2");
+
+ ConfigurationForOutput targetConfig = null;
+ for (JsonElement configJson :
+ JsonParser.parseString(callConfigCommand("--dump_all").outAsLatin1()).getAsJsonArray()) {
+ ConfigurationForOutput config = new Gson().fromJson(configJson, ConfigurationForOutput.class);
+ if (isTargetConfig(config)) {
+ targetConfig = config;
+ break;
+ }
+ }
+
+ assertThat(targetConfig).isNotNull();
+ assertThat(getOptionValue(targetConfig, "user-defined", "--define:a")).isEqualTo("2");
+ assertThat(
+ targetConfig.fragmentOptions.stream()
+ .filter(fragment -> fragment.name.endsWith("CoreOptions"))
+ .flatMap(fragment -> fragment.options.keySet().stream())
+ .filter(name -> name.equals("define"))
+ .collect(Collectors.toList()))
+ .isEmpty();
+ }
}
