Skip to content

Commit

Permalink
config doesn't error on duplicate --define values (#15473)
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
ckolli5 and illicitonion authored May 12, 2022
1 parent 1651b3b commit e133e66
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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<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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -557,7 +557,11 @@ private static ImmutableSortedMap<String, String> getOrderedUserDefinedOptions(

// --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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}

0 comments on commit e133e66

Please sign in to comment.