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 af8bc0183e9091..34be910e4b40c7 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 @@ public BuildConfiguration( // 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 commandLineDefinesBuilder = new TreeMap<>(); - for (Map.Entry 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 5203a883b811f6..c2d17a49d8ff6b 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 @@ public class CoreOptions extends FragmentOptions implements Cloneable { 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 @@ public FragmentOptions getHost() { return host; } + /// Normalizes --define flags, preserving the last one to appear in the event of conflicts. + public LinkedHashMap getNormalizedCommandLineBuildVariables() { + LinkedHashMap flagValueByName = new LinkedHashMap<>(); + for (Map.Entry 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 flagValueByName = new LinkedHashMap<>(); - for (Map.Entry entry : result.commandLineBuildVariables) { - // If the same --define flag is passed multiple times we keep the last value. - flagValueByName.put(entry.getKey(), entry.getValue()); - } + LinkedHashMap 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 38f73612c0a111..3e848d2391abab 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 @@ private static ImmutableSortedMap getOrderedUserDefinedOptions( // --define: for (Map.Entry 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 bc7e069a9c0bfe..2c4a0b69026682 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 @@ public void defineFlagsIndividuallyListedInUserDefinedFragment() throws Exceptio .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(); + } }