diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java index 3e90ffa937e7b1..c275fa438a8562 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java @@ -419,8 +419,7 @@ public static Map applyTransition( if (doesStarlarkTransition) { fromOptions = addDefaultStarlarkOptions( - fromOptions, - StarlarkTransition.getDefaultInputValues(buildSettingPackages, transition)); + fromOptions, StarlarkTransition.getDefaultValues(buildSettingPackages, transition)); } // TODO(bazel-team): Add safety-check that this never mutates fromOptions. diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java index 3e795c405761ae..4611892f06d41d 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java @@ -238,18 +238,28 @@ private static BuildOptions applyTransition( StarlarkDefinedConfigTransition starlarkTransition) throws EvalException { BuildOptions buildOptions = buildOptionsToTransition.clone(); + // The names and values of options that are different after this transition. HashMap convertedNewValues = new HashMap<>(); for (Map.Entry entry : newValues.entrySet()) { String optionName = entry.getKey(); Object optionValue = entry.getValue(); if (!optionName.startsWith(COMMAND_LINE_OPTION_PREFIX)) { - buildOptions = - BuildOptions.builder() - .merge(buildOptions) - .addStarlarkOption(Label.parseAbsoluteUnchecked(optionName), optionValue) - .build(); - convertedNewValues.put(optionName, optionValue); + Object oldValue = + buildOptions.getStarlarkOptions().get(Label.parseAbsoluteUnchecked(optionName)); + if ((oldValue == null && optionValue != null) + || (oldValue != null && optionValue == null) + || (oldValue != null && !oldValue.equals(optionValue))) { + // TODO(bazel-team): Figure out if we need to create a whole new build options every + // time. Can we just keep track of the running changes and actually build a new build + // options after this loop? + buildOptions = + BuildOptions.builder() + .merge(buildOptions) + .addStarlarkOption(Label.parseAbsoluteUnchecked(optionName), optionValue) + .build(); + convertedNewValues.put(optionName, optionValue); + } } else { optionName = optionName.substring(COMMAND_LINE_OPTION_PREFIX.length()); @@ -298,8 +308,15 @@ private static BuildOptions applyTransition( } else { throw Starlark.errorf("Invalid value type for option '%s'", optionName); } - field.set(options, convertedValue); - convertedNewValues.put(entry.getKey(), convertedValue); + + Object oldValue = field.get(options); + if ((oldValue == null && convertedValue != null) + || (oldValue != null && convertedValue == null) + || (oldValue != null && !oldValue.equals(convertedValue))) { + field.set(options, convertedValue); + convertedNewValues.put(entry.getKey(), convertedValue); + } + } catch (IllegalArgumentException e) { throw Starlark.errorf( "IllegalArgumentError for option '%s': %s", optionName, e.getMessage()); @@ -342,6 +359,11 @@ private static BuildOptions applyTransition( // makes it so that two configurations that are the same in value may hash differently. private static void updateOutputDirectoryNameFragment( Set changedOptions, Map optionInfoMap, BuildOptions toOptions) { + // Return without doing anything if this transition hasn't changed any option values. + if (changedOptions.isEmpty()) { + return; + } + CoreOptions buildConfigOptions = toOptions.get(CoreOptions.class); Set updatedAffectedByStarlarkTransition = new TreeSet<>(buildConfigOptions.affectedByStarlarkTransition); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkTransition.java index f3e62413fa1ed3..316be455f1c753 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkTransition.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkTransition.java @@ -391,8 +391,8 @@ private static BuildOptions unalias( } /** - * For a given transition, find all Starlark build settings that are read while applying it, then - * return a map of their label to their default values. + * For a given transition, find all Starlark build settings that are input/output while applying + * it, then return a map of their label to their default values. * *

If the build setting is referenced by an {@link com.google.devtools.build.lib.rules.Alias}, * the returned map entry is still keyed by the alias. @@ -402,7 +402,7 @@ private static BuildOptions unalias( * build settings *written* by relevant transitions) so do not iterate over for input * packages. */ - public static ImmutableMap getDefaultInputValues( + public static ImmutableMap getDefaultValues( Map buildSettingPackages, ConfigurationTransition root) throws TransitionException { ImmutableMap.Builder defaultValues = new ImmutableMap.Builder<>(); @@ -410,7 +410,8 @@ public static ImmutableMap getDefaultInputValues( (StarlarkTransitionVisitor) transition -> { ImmutableSet