From e133e66f715bac17bf5848e4440c089a8c8d3fd9 Mon Sep 17 00:00:00 2001 From: Chenchu Kolli Date: Thu, 12 May 2022 07:03:39 -0500 Subject: [PATCH] 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 --- .../analysis/config/BuildConfiguration.java | 8 ++---- .../lib/analysis/config/CoreOptions.java | 19 ++++++++++---- .../lib/runtime/commands/ConfigCommand.java | 6 ++++- .../runtime/commands/ConfigCommandTest.java | 25 +++++++++++++++++++ 4 files changed, 46 insertions(+), 12 deletions(-) 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(); + } }