Add `--experimental_output_directory_naming_scheme` with two modes. The `legacy` mode retains pre-existing behavior for determining the ST hash value by tracking an `affected by starlark transition` list and consuming it. The new `diff_against_baseline` mode determines the ST hash value by diffing the current configuration against the build's baseline configuration (currently, the configuration used to prime all top-level targets). Note that this mode specifically turns off tracking the `affected by starlark transition` for performance reasons (see linked bug for details). Related github issue: https://github.com/bazelbuild/bazel/issues/14023 PiperOrigin-RevId: 433882310
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index 40db26d..908a2c5 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
@@ -1536,7 +1536,6 @@ ":config/invalid_configuration_exception", ":config/run_under", ":platform_options", - ":starlark/function_transition_util", "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/buildeventstream", @@ -1867,6 +1866,17 @@ ], ) +java_library( + name = "config/optioninfo", + srcs = ["config/OptionInfo.java"], + deps = [ + ":config/build_options", + ":config/fragment_options", + "//src/main/java/com/google/devtools/common/options", + "//third_party:guava", + ], +) + # TODO(b/144899336): This should be config/transitions/BUILD java_library( name = "config/transitions/composing_transition", @@ -2172,12 +2182,12 @@ ":config/build_options", ":config/core_options", ":config/fragment_options", + ":config/optioninfo", ":config/starlark_defined_config_transition", ":config/transitions/configuration_transition", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/packages", - "//src/main/java/com/google/devtools/build/lib/util", "//src/main/java/com/google/devtools/common/options", "//src/main/java/net/starlark/java/eval", "//third_party:guava",
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java index 4145642..1495aee 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java
@@ -29,7 +29,6 @@ import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.PlatformOptions; import com.google.devtools.build.lib.analysis.config.OutputDirectories.InvalidMnemonicException; -import com.google.devtools.build.lib.analysis.starlark.FunctionTransitionUtil; import com.google.devtools.build.lib.buildeventstream.BuildEventIdUtil; import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos; import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildEventId; @@ -161,6 +160,7 @@ BuildOptions buildOptions, RepositoryName mainRepositoryName, boolean siblingRepositoryLayout, + String transitionDirectoryNameFragment, // Arguments below this are server-global. BlazeDirectories directories, GlobalStateProvider globalProvider, @@ -175,6 +175,7 @@ buildOptions, mainRepositoryName, siblingRepositoryLayout, + transitionDirectoryNameFragment, directories, fragments, globalProvider.getReservedActionMnemonics(), @@ -200,6 +201,7 @@ BuildOptions buildOptions, RepositoryName mainRepositoryName, boolean siblingRepositoryLayout, + String transitionDirectoryNameFragment, // Arguments below this are either server-global and constant or completely dependent values. BlazeDirectories directories, ImmutableMap<Class<? extends Fragment>, Fragment> fragments, @@ -216,8 +218,7 @@ if (buildOptions.contains(PlatformOptions.class)) { platformOptions = buildOptions.get(PlatformOptions.class); } - this.transitionDirectoryNameFragment = - FunctionTransitionUtil.computeOutputDirectoryNameFragment(buildOptions); + this.transitionDirectoryNameFragment = transitionDirectoryNameFragment; this.outputDirectories = new OutputDirectories( directories, @@ -272,12 +273,14 @@ BuildConfigurationValue otherVal = (BuildConfigurationValue) other; return this.buildOptions.equals(otherVal.buildOptions) && this.mainRepositoryName.equals(otherVal.mainRepositoryName) - && this.siblingRepositoryLayout == otherVal.siblingRepositoryLayout; + && this.siblingRepositoryLayout == otherVal.siblingRepositoryLayout + && this.transitionDirectoryNameFragment.equals(otherVal.transitionDirectoryNameFragment); } @Override public int hashCode() { - return Objects.hash(buildOptions, mainRepositoryName, siblingRepositoryLayout); + return Objects.hash( + buildOptions, mainRepositoryName, siblingRepositoryLayout, transitionDirectoryNameFragment); } private ImmutableMap<String, Class<? extends Fragment>> buildIndexOfStarlarkVisibleFragments() {
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 f9c3917..49f6213 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
@@ -536,11 +536,11 @@ + " existing build actions.") public List<Label> actionListeners; - /** Values for the --experimental_output_directory_naming_scheme options * */ + /** Values for the --experimental_output_directory_naming_scheme options */ public enum OutputDirectoryNamingScheme { - /** Use `affected by starlark transition` to track configuration changes * */ + /** Use `affected by starlark transition` to track configuration changes */ LEGACY, - /** Produce name based on diff from some baseline BuildOptions (usually top-level) * */ + /** Produce name based on diff from some baseline BuildOptions (usually top-level) */ DIFF_AGAINST_BASELINE } @@ -559,7 +559,12 @@ documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, effectTags = {OptionEffectTag.AFFECTS_OUTPUTS}, metadataTags = {OptionMetadataTag.EXPERIMENTAL}, - help = "Please only use this flag as part of a suggested migration or testing strategy.") + help = + "Please only use this flag as part of a suggested migration or testing strategy. In" + + " legacy mode, transitions (generally only Starlark) set and use `affected by" + + " Starlark transition` to determine the ST hash. In diff_against_baseline mode," + + " `affected by Starlark transition` is ignored and instead ST hash is determined," + + " for all configuration, by diffing against the top-level configuration.") public OutputDirectoryNamingScheme outputDirectoryNamingScheme; @Option(
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/OptionInfo.java b/src/main/java/com/google/devtools/build/lib/analysis/config/OptionInfo.java new file mode 100644 index 0000000..64bed42 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/OptionInfo.java
@@ -0,0 +1,69 @@ +// Copyright 2022 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.analysis.config; + +import static com.google.common.collect.ImmutableSet.toImmutableSet; +import static java.util.Arrays.stream; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import com.google.devtools.common.options.OptionDefinition; +import com.google.devtools.common.options.OptionMetadataTag; +import com.google.devtools.common.options.OptionsParser; + +/** Stores information about a build option gathered via reflection. */ +public final class OptionInfo { + private final Class<? extends FragmentOptions> optionClass; + private final OptionDefinition definition; + + private OptionInfo(Class<? extends FragmentOptions> optionClass, OptionDefinition definition) { + this.optionClass = optionClass; + this.definition = definition; + } + + public Class<? extends FragmentOptions> getOptionClass() { + return optionClass; + } + + public OptionDefinition getDefinition() { + return definition; + } + + public boolean hasOptionMetadataTag(OptionMetadataTag tag) { + return stream(getDefinition().getOptionMetadataTags()).anyMatch(tag::equals); + } + + /** For all the options in the BuildOptions, build a map from option name to its information. */ + public static ImmutableMap<String, OptionInfo> buildMapFrom(BuildOptions buildOptions) { + ImmutableMap.Builder<String, OptionInfo> builder = new ImmutableMap.Builder<>(); + + ImmutableSet<Class<? extends FragmentOptions>> optionClasses = + buildOptions.getNativeOptions().stream() + .map(FragmentOptions::getClass) + .collect(toImmutableSet()); + + for (Class<? extends FragmentOptions> optionClass : optionClasses) { + ImmutableList<OptionDefinition> optionDefinitions = + OptionsParser.getOptionDefinitions(optionClass); + for (OptionDefinition def : optionDefinitions) { + String optionName = def.getOptionName(); + builder.put(optionName, new OptionInfo(optionClass, def)); + } + } + + return builder.build(); + } +}
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 1fba765..57d8cb0 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
@@ -17,11 +17,9 @@ import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition.COMMAND_LINE_OPTION_PREFIX; import static com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition.PATCH_TRANSITION_KEY; -import static java.util.Arrays.stream; import static java.util.stream.Collectors.joining; import com.google.common.base.Joiner; -import com.google.common.base.VerifyException; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; @@ -30,16 +28,15 @@ import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.analysis.config.FragmentOptions; +import com.google.devtools.build.lib.analysis.config.OptionInfo; import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition; import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition.ValidationException; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.packages.StructImpl; -import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.common.options.OptionDefinition; import com.google.devtools.common.options.OptionMetadataTag; -import com.google.devtools.common.options.OptionsParser; import com.google.devtools.common.options.OptionsParsingException; import java.lang.reflect.Field; import java.util.HashSet; @@ -48,7 +45,6 @@ import java.util.List; import java.util.Map; import java.util.Set; -import java.util.TreeMap; import java.util.TreeSet; import java.util.stream.Stream; import javax.annotation.Nullable; @@ -64,10 +60,6 @@ */ public final class FunctionTransitionUtil { - // The length of the hash of the config tacked onto the end of the output path. - // Limited for ergonomics and MAX_PATH reasons. - private static final int HASH_LENGTH = 12; - /** * Figure out what build settings the given transition changes and apply those changes to the * incoming {@link BuildOptions}. For native options, this involves a preprocess step of @@ -92,7 +84,7 @@ // TODO(waltl): Consider building this once and using it across different split transitions, // or reusing BuildOptionDetails. - Map<String, OptionInfo> optionInfoMap = buildOptionInfo(buildOptions); + ImmutableMap<String, OptionInfo> optionInfoMap = OptionInfo.buildMapFrom(buildOptions); Dict<String, Object> settings = buildSettings(buildOptions, optionInfoMap, starlarkTransition); @@ -113,7 +105,7 @@ applyTransition(buildOptions, newValues, optionInfoMap, starlarkTransition); splitBuildOptions.put(entry.getKey(), transitionedOptions); } - return splitBuildOptions.buildOrThrow(); + return splitBuildOptions.build(); } catch (ValidationException ex) { handler.handle(Event.error(starlarkTransition.getLocation(), ex.getMessage())); @@ -135,7 +127,8 @@ * * <p>Transitions can also explicitly set --platforms to be clear what platform they set. * - * <p>Platform mappings: https://bazel.build/concepts/platform-intro#platform-mappings. + * <p>Platform mappings: + * https://docs.bazel.build/versions/main/platforms-intro.html#platform-mappings. * * <p>This doesn't check that the changed value is actually different than the source (i.e. * setting {@code --cpu=foo} when {@code --cpu} is already {@code foo}). That could unnecessarily @@ -149,7 +142,7 @@ ? ImmutableMap.<String, Object>builder() .putAll(originalOutput) .put(COMMAND_LINE_OPTION_PREFIX + "platforms", ImmutableList.<Label>of()) - .buildOrThrow() + .build() : originalOutput; } @@ -158,31 +151,10 @@ if (transition.getOutputs().contains("//command_line_option:define")) { throw new ValidationException( "Starlark transition on --define not supported - try using build settings" - + " (https://bazel.build/rules/config#user-defined-build-settings)."); + + " (https://docs.bazel.build/skylark/config.html#user-defined-build-settings)."); } } - /** For all the options in the BuildOptions, build a map from option name to its information. */ - private static ImmutableMap<String, OptionInfo> buildOptionInfo(BuildOptions buildOptions) { - ImmutableMap.Builder<String, OptionInfo> builder = new ImmutableMap.Builder<>(); - - ImmutableSet<Class<? extends FragmentOptions>> optionClasses = - buildOptions.getNativeOptions().stream() - .map(FragmentOptions::getClass) - .collect(toImmutableSet()); - - for (Class<? extends FragmentOptions> optionClass : optionClasses) { - ImmutableList<OptionDefinition> optionDefinitions = - OptionsParser.getOptionDefinitions(optionClass); - for (OptionDefinition def : optionDefinitions) { - String optionName = def.getOptionName(); - builder.put(optionName, new OptionInfo(optionClass, def)); - } - } - - return builder.buildOrThrow(); - } - /** * Enter the options in buildOptions into a Starlark dictionary, and return the dictionary. * @@ -199,7 +171,7 @@ Map<String, OptionInfo> optionInfoMap, StarlarkDefinedConfigTransition starlarkTransition) throws ValidationException { - Map<String, String> inputsCanonicalizedToGiven = + ImmutableMap<String, String> inputsCanonicalizedToGiven = starlarkTransition.getInputsCanonicalizedToGiven(); LinkedHashSet<String> remainingInputs = Sets.newLinkedHashSet(inputsCanonicalizedToGiven.keySet()); @@ -368,7 +340,7 @@ } field.set(toOptions.get(optionInfo.getOptionClass()), convertedValue); - if (!optionInfo.hasMetadataTag(OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH)) { + if (!optionInfo.hasOptionMetadataTag(OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH)) { convertedAffectedOptions.add(optionKey); } } @@ -403,73 +375,17 @@ toOptions.get(CoreOptions.class).evaluatingForAnalysisTest = true; } - updateAffectedByStarlarkTransition(toOptions.get(CoreOptions.class), convertedAffectedOptions); + CoreOptions coreOptions = toOptions.get(CoreOptions.class); + if (coreOptions.outputDirectoryNamingScheme.equals( + CoreOptions.OutputDirectoryNamingScheme.LEGACY)) { + updateAffectedByStarlarkTransition(coreOptions, convertedAffectedOptions); + } return toOptions; } - /** - * Compute the output directory name fragment corresponding to the new BuildOptions based on the - * names and values of all options (both native and Starlark) previously transitioned anywhere in - * the build by Starlark transitions. Options only set on command line are not affecting the - * computation. - * - * @param toOptions the {@link BuildOptions} to use to calculate which we need to compute {@code - * transitionDirectoryNameFragment}. - */ - // TODO(bazel-team): This hashes different forms of equivalent values differently though they - // should be the same configuration. Starlark transitions are flexible about the values they - // take (e.g. bool-typed options can take 0/1, True/False, "0"/"1", or "True"/"False") which - // makes it so that two configurations that are the same in value may hash differently. - public static String computeOutputDirectoryNameFragment(BuildOptions toOptions) { - CoreOptions buildConfigOptions = toOptions.get(CoreOptions.class); - if (buildConfigOptions.affectedByStarlarkTransition.isEmpty()) { - return ""; - } - // TODO(blaze-configurability-team): A mild performance optimization would have this be global. - Map<String, OptionInfo> optionInfoMap = buildOptionInfo(toOptions); - - TreeMap<String, Object> toHash = new TreeMap<>(); - for (String optionName : buildConfigOptions.affectedByStarlarkTransition) { - if (optionName.startsWith(COMMAND_LINE_OPTION_PREFIX)) { - String nativeOptionName = optionName.substring(COMMAND_LINE_OPTION_PREFIX.length()); - Object value; - try { - OptionInfo optionInfo = optionInfoMap.get(nativeOptionName); - if (optionInfo == null) { - // This can occur if toOptions has been trimmed but the supplied chosen native options - // includes that trimmed options. - // (e.g. legacy naming mode, using --trim_test_configuration and --test_arg transition). - continue; - } - value = - optionInfo - .getDefinition() - .getField() - .get(toOptions.get(optionInfoMap.get(nativeOptionName).getOptionClass())); - } catch (IllegalAccessException e) { - throw new VerifyException( - "IllegalAccess for option " + nativeOptionName + ": " + e.getMessage()); - } - toHash.put(optionName, value); - } else { - Object value = toOptions.getStarlarkOptions().get(Label.parseAbsoluteUnchecked(optionName)); - toHash.put(optionName, value); - } - } - - ImmutableList.Builder<String> hashStrs = ImmutableList.builderWithExpectedSize(toHash.size()); - for (Map.Entry<String, Object> singleOptionAndValue : toHash.entrySet()) { - Object value = singleOptionAndValue.getValue(); - if (value != null) { - hashStrs.add(singleOptionAndValue.getKey() + "=" + value); - } else { - // Avoid using =null to different from value being the non-null String "null" - hashStrs.add(singleOptionAndValue.getKey() + "@null"); - } - } - return transitionDirectoryNameFragment(hashStrs.build()); - } - + /** Return different options in "affected by Starlark transition" form */ + // TODO(blaze-configurability-team):This only exists for pseudo-legacy fixups of native + // transitions. Remove once those fixups are removed in favor of the global fixup. public static ImmutableSet<String> getAffectedByStarlarkTransitionViaDiff( BuildOptions toOptions, BuildOptions baselineOptions) { if (toOptions.equals(baselineOptions)) { @@ -492,7 +408,7 @@ /** * Extend the global build config affectedByStarlarkTransition, by adding any new option names - * from changedOptions + * from changedOptions. Does nothing if output directory naming scheme is not in legacy mode. */ public static void updateAffectedByStarlarkTransition( CoreOptions buildConfigOptions, Set<String> changedOptions) { @@ -506,40 +422,5 @@ ImmutableList.sortedCopyOf(mutableCopyToUpdate); } - public static String transitionDirectoryNameFragment(Iterable<String> opts) { - Fingerprint fp = new Fingerprint(); - for (String opt : opts) { - fp.addString(opt); - } - // Shorten the hash to 48 bits. This should provide sufficient collision avoidance - // (that is, we don't expect anyone to experience a collision ever). - // Shortening the hash is important for Windows paths that tend to be short. - String suffix = fp.hexDigestAndReset().substring(0, HASH_LENGTH); - return "ST-" + suffix; - } - - /** Stores option info useful to a FunctionSplitTransition. */ - private static final class OptionInfo { - private final Class<? extends FragmentOptions> optionClass; - private final OptionDefinition definition; - - OptionInfo(Class<? extends FragmentOptions> optionClass, OptionDefinition definition) { - this.optionClass = optionClass; - this.definition = definition; - } - - Class<? extends FragmentOptions> getOptionClass() { - return optionClass; - } - - OptionDefinition getDefinition() { - return definition; - } - - boolean hasMetadataTag(OptionMetadataTag tag) { - return stream(getDefinition().getOptionMetadataTags()).anyMatch(tag::equals); - } - } - private FunctionTransitionUtil() {} }
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/BUILD b/src/main/java/com/google/devtools/build/lib/runtime/commands/BUILD index 3937792..df72494 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/commands/BUILD +++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/BUILD
@@ -34,6 +34,7 @@ "//src/main/java/com/google/devtools/build/lib/analysis:analysis_options", "//src/main/java/com/google/devtools/build/lib/analysis:blaze_version_info", "//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration", + "//src/main/java/com/google/devtools/build/lib/analysis:config/build_options", "//src/main/java/com/google/devtools/build/lib/analysis:config/core_options", "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment", "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_class_set",
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/InfoCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/InfoCommand.java index 6fd3459..05e6b67 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/commands/InfoCommand.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/InfoCommand.java
@@ -20,6 +20,7 @@ import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.analysis.NoBuildEvent; import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; +import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.profiler.Profiler; @@ -162,11 +163,10 @@ // information is available here. env.syncPackageLoading(optionsParsingResult); // TODO(bazel-team): What if there are multiple configurations? [multi-config] + BuildOptions buildOptions = runtime.createBuildOptions(optionsParsingResult); + env.getSkyframeExecutor().setBaselineConfiguration(buildOptions); return env.getSkyframeExecutor() - .getConfiguration( - env.getReporter(), - runtime.createBuildOptions(optionsParsingResult), - /*keepGoing=*/ true); + .getConfiguration(env.getReporter(), buildOptions, /*keepGoing=*/ true); } catch (InvalidConfigurationException e) { env.getReporter().handle(Event.error(e.getMessage())); throw new AbruptExitRuntimeException(e.getDetailedExitCode());
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index 8ed9f6c..4b430e2 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -252,6 +252,8 @@ "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options", "//src/main/java/com/google/devtools/build/lib/analysis:config/host_transition", "//src/main/java/com/google/devtools/build/lib/analysis:config/invalid_configuration_exception", + "//src/main/java/com/google/devtools/build/lib/analysis:config/optioninfo", + "//src/main/java/com/google/devtools/build/lib/analysis:config/starlark_defined_config_transition", "//src/main/java/com/google/devtools/build/lib/analysis:config/toolchain_type_requirement", "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/configuration_transition", "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/no_transition",
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java index 0f53eb1..fde86b8 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java
@@ -13,25 +13,46 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe; +import static com.google.common.collect.ImmutableSet.toImmutableSet; +import static com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition.COMMAND_LINE_OPTION_PREFIX; + +import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.VerifyException; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; +import com.google.devtools.build.lib.analysis.PlatformOptions; import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.analysis.config.FragmentFactory; import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException; +import com.google.devtools.build.lib.analysis.config.OptionInfo; +import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.packages.RuleClassProvider; import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; +import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunctionException; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; +import com.google.devtools.common.options.OptionDefinition; +import com.google.devtools.common.options.OptionMetadataTag; +import com.google.devtools.common.options.OptionsParsingException; +import java.util.Map; +import java.util.TreeMap; import net.starlark.java.eval.StarlarkSemantics; /** A builder for {@link BuildConfigurationValue} instances. */ public final class BuildConfigurationFunction implements SkyFunction { + // The length of the hash of the config tacked onto the end of the output path. + // Limited for ergonomics and MAX_PATH reasons. + private static final int HASH_LENGTH = 12; + private final BlazeDirectories directories; private final ConfiguredRuleClassProvider ruleClassProvider; private final FragmentFactory fragmentFactory = new FragmentFactory(); @@ -58,13 +79,36 @@ BuildConfigurationKey key = (BuildConfigurationKey) skyKey.argument(); BuildOptions targetOptions = key.getOptions(); - // TODO(blaze-configurability-team): Actually use this rather than just simulate getting it. - // (currently just testing that Skyframe setup is correct for when option is used) + + String transitionDirectoryNameFragment; if (targetOptions .get(CoreOptions.class) .outputDirectoryNamingScheme .equals(CoreOptions.OutputDirectoryNamingScheme.DIFF_AGAINST_BASELINE)) { - PrecomputedValue.BASELINE_CONFIGURATION.get(env); + // Herein lies a hack to apply platform mappings to the baseline options. + // TODO(blaze-configurability-team): this should become unnecessary once --platforms is marked + // as EXPLICIT_IN_OUTPUT_PATH + PlatformMappingValue platformMappingValue = + (PlatformMappingValue) + env.getValue( + PlatformMappingValue.Key.create( + targetOptions.get(PlatformOptions.class).platformMappings)); + if (platformMappingValue == null) { + return null; + } + BuildOptions baselineOptions = PrecomputedValue.BASELINE_CONFIGURATION.get(env); + try { + BuildOptions mappedBaselineOptions = + BuildConfigurationKey.withPlatformMapping(platformMappingValue, baselineOptions) + .getOptions(); + transitionDirectoryNameFragment = + computeNameFragmentWithDiff(targetOptions, mappedBaselineOptions); + } catch (OptionsParsingException e) { + throw new BuildConfigurationFunctionException(e); + } + } else { + transitionDirectoryNameFragment = + computeNameFragmentWithAffectedByStarlarkTransition(targetOptions); } try { @@ -72,6 +116,7 @@ targetOptions, RepositoryName.createFromValidStrippedName(workspaceNameValue.getName()), starlarkSemantics.getBool(BuildLanguageOptions.EXPERIMENTAL_SIBLING_REPOSITORY_LAYOUT), + transitionDirectoryNameFragment, // Arguments below this are server-global. directories, ruleClassProvider, @@ -82,8 +127,152 @@ } private static final class BuildConfigurationFunctionException extends SkyFunctionException { - BuildConfigurationFunctionException(InvalidConfigurationException e) { + BuildConfigurationFunctionException(Exception e) { super(e, Transience.PERSISTENT); } } + + /** + * Compute the hash for the new BuildOptions based on the names and values of all options (both + * native and Starlark) that are different from some supplied baseline configuration. + */ + private static String computeNameFragmentWithDiff( + BuildOptions toOptions, BuildOptions baselineOptions) { + // Quick short-circuit for trivial case. + if (toOptions.equals(baselineOptions)) { + return ""; + } + + // TODO(blaze-configurability-team): As a mild performance update, getFirst already includes + // details of the corresponding option. Could incorporate this instead of hashChosenOptions + // regenerating the OptionDefinitions and values. + BuildOptions.OptionsDiff diff = BuildOptions.diff(toOptions, baselineOptions); + // Note: getFirst only excludes options trimmed between baselineOptions to toOptions and this is + // considered OK as a given Rule should not be being built with options of different + // trimmings. See longform note in {@link ConfiguredTargetKey} for details. + ImmutableSet<String> chosenNativeOptions = + diff.getFirst().keySet().stream() + .filter( + optionDef -> + !optionDef.hasOptionMetadataTag(OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH)) + .map(OptionDefinition::getOptionName) + .collect(toImmutableSet()); + // Note: getChangedStarlarkOptions includes all changed options, added options and removed + // options between baselineOptions and toOptions. This is necessary since there is no current + // notion of trimming a Starlark option: 'null' or non-existent justs means set to default. + ImmutableSet<String> chosenStarlarkOptions = + diff.getChangedStarlarkOptions().stream().map(Label::toString).collect(toImmutableSet()); + return hashChosenOptions(toOptions, chosenNativeOptions, chosenStarlarkOptions); + } + + /** + * Compute the output directory name fragment corresponding to the new BuildOptions based on the + * names and values of all options (both native and Starlark) previously transitioned anywhere in + * the build by Starlark transitions. Options only set on command line are not affecting the + * computation. + * + * @param toOptions the {@link BuildOptions} to use to calculate which we need to compute {@code + * transitionDirectoryNameFragment}. + */ + private static String computeNameFragmentWithAffectedByStarlarkTransition( + BuildOptions toOptions) { + CoreOptions buildConfigOptions = toOptions.get(CoreOptions.class); + if (buildConfigOptions.affectedByStarlarkTransition.isEmpty()) { + return ""; + } + + ImmutableList.Builder<String> affectedNativeOptions = ImmutableList.builder(); + ImmutableList.Builder<String> affectedStarlarkOptions = ImmutableList.builder(); + + for (String optionName : buildConfigOptions.affectedByStarlarkTransition) { + if (optionName.startsWith(COMMAND_LINE_OPTION_PREFIX)) { + String nativeOptionName = optionName.substring(COMMAND_LINE_OPTION_PREFIX.length()); + affectedNativeOptions.add(nativeOptionName); + } else { + affectedStarlarkOptions.add(optionName); + } + } + + return hashChosenOptions( + toOptions, affectedNativeOptions.build(), affectedStarlarkOptions.build()); + } + + /** + * Compute a hash of the given BuildOptions by hashing only the options referenced in both + * chosenNative and chosenStarlark. The order of the chosen order does not matter (as this + * function will effectively sort them into a canonical order) and the pre-hash for each option + * will be of the form (//command_line_option:[native option]|[Starlark option label])=[value]. + * + * <p>If a supplied native option does not exist, it is skipped (as it is presumed non-existence + * is due to trimming). + * + * <p>If a supplied Starlark option does exist, the pre-hash will be [Starlark option label]@null + * (as it is presumed non-existence is due to being set to default value). + */ + private static String hashChosenOptions( + BuildOptions toOptions, Iterable<String> chosenNative, Iterable<String> chosenStarlark) { + // TODO(blaze-configurability-team): A mild performance optimization would have this be global. + ImmutableMap<String, OptionInfo> optionInfoMap = OptionInfo.buildMapFrom(toOptions); + + // Note that the TreeMap guarantees a stable ordering of keys and thus + // it is okay if chosenNative or chosenStarlark do not have a stable iteration order + TreeMap<String, Object> toHash = new TreeMap<>(); + for (String nativeOptionName : chosenNative) { + Object value; + try { + OptionInfo optionInfo = optionInfoMap.get(nativeOptionName); + if (optionInfo == null) { + // This can occur if toOptions has been trimmed but the supplied chosen native options + // includes that trimmed options. + // (e.g. legacy naming mode, using --trim_test_configuration and --test_arg transition). + continue; + } + value = + optionInfo + .getDefinition() + .getField() + .get(toOptions.get(optionInfoMap.get(nativeOptionName).getOptionClass())); + } catch (IllegalAccessException e) { + throw new VerifyException( + "IllegalAccess for option " + nativeOptionName + ": " + e.getMessage()); + } + // TODO(blaze-configurability-team): The commandline option is legacy and can be removed + // after fixing up all the associated tests. + toHash.put("//command_line_option:" + nativeOptionName, value); + } + for (String starlarkOptionName : chosenStarlark) { + Object value = + toOptions.getStarlarkOptions().get(Label.parseAbsoluteUnchecked(starlarkOptionName)); + toHash.put(starlarkOptionName, value); + } + + if (toHash.isEmpty()) { + return ""; + } else { + ImmutableList.Builder<String> hashStrs = ImmutableList.builderWithExpectedSize(toHash.size()); + for (Map.Entry<String, Object> singleOptionAndValue : toHash.entrySet()) { + Object value = singleOptionAndValue.getValue(); + if (value != null) { + hashStrs.add(singleOptionAndValue.getKey() + "=" + value); + } else { + // Avoid using =null to different from value being the non-null String "null" + hashStrs.add(singleOptionAndValue.getKey() + "@null"); + } + } + return transitionDirectoryNameFragment(hashStrs.build()); + } + } + + @VisibleForTesting + public static String transitionDirectoryNameFragment(Iterable<String> opts) { + Fingerprint fp = new Fingerprint(); + for (String opt : opts) { + fp.addString(opt); + } + // Shorten the hash to 48 bits. This should provide sufficient collision avoidance + // (that is, we don't expect anyone to experience a collision ever). + // Shortening the hash is important for Windows paths that tend to be short. + String suffix = fp.hexDigestAndReset().substring(0, HASH_LENGTH); + return "ST-" + suffix; + } }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java index 5a95446..a32ed6f 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java
@@ -26,7 +26,6 @@ import com.google.common.eventbus.EventBus; import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition; -import com.google.devtools.build.lib.analysis.starlark.FunctionTransitionUtil; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.analysis.util.DummyTestFragment; import com.google.devtools.build.lib.analysis.util.DummyTestFragment.DummyTestOptions; @@ -40,6 +39,7 @@ import com.google.devtools.build.lib.packages.util.BazelMockAndroidSupport; import com.google.devtools.build.lib.rules.cpp.CppConfiguration; import com.google.devtools.build.lib.rules.cpp.CppOptions; +import com.google.devtools.build.lib.skyframe.BuildConfigurationFunction; import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData; import com.google.devtools.build.lib.testutil.TestConstants; import com.google.devtools.build.lib.testutil.TestRuleClassProvider; @@ -1309,7 +1309,7 @@ // never touched by any transition. assertThat(getTransitionDirectoryNameFragment(dep)) .isEqualTo( - FunctionTransitionUtil.transitionDirectoryNameFragment( + BuildConfigurationFunction.transitionDirectoryNameFragment( ImmutableList.of("//test/starlark:the-answer=42"))); } @@ -1373,8 +1373,7 @@ assertThat(getConfiguration(test)).isEqualTo(getConfiguration(dep2)); } - @Test - public void testOutputDirHash_multipleNativeOptionTransitions() throws Exception { + private void writeFilesWithMultipleNativeOptionTransitions() throws Exception { writeAllowlistFile(); scratch.file( "test/transitions.bzl", @@ -1409,7 +1408,13 @@ "load('//test:rules.bzl', 'my_rule', 'simple')", "my_rule(name = 'test', dep = ':dep')", "simple(name = 'dep')"); + } + @Test + public void testOutputDirHash_multipleNativeOptionTransitions_legacyNaming() throws Exception { + writeFilesWithMultipleNativeOptionTransitions(); + + useConfiguration("--experimental_output_directory_naming_scheme=legacy"); ConfiguredTarget test = getConfiguredTarget("//test"); List<String> affectedOptions = getCoreOptions(test).affectedByStarlarkTransition; @@ -1428,12 +1433,36 @@ assertThat(getTransitionDirectoryNameFragment(test)) .isEqualTo( - FunctionTransitionUtil.transitionDirectoryNameFragment( + BuildConfigurationFunction.transitionDirectoryNameFragment( ImmutableList.of("//command_line_option:foo=foosball"))); assertThat(getTransitionDirectoryNameFragment(dep)) .isEqualTo( - FunctionTransitionUtil.transitionDirectoryNameFragment( + BuildConfigurationFunction.transitionDirectoryNameFragment( + ImmutableList.of( + "//command_line_option:bar=barsball", "//command_line_option:foo=foosball"))); + } + + @Test + public void testOutputDirHash_multipleNativeOptionTransitions_diffNaming() throws Exception { + writeFilesWithMultipleNativeOptionTransitions(); + + useConfiguration("--experimental_output_directory_naming_scheme=diff_against_baseline"); + ConfiguredTarget test = getConfiguredTarget("//test"); + + @SuppressWarnings("unchecked") + ConfiguredTarget dep = + Iterables.getOnlyElement( + (List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep")); + + assertThat(getTransitionDirectoryNameFragment(test)) + .isEqualTo( + BuildConfigurationFunction.transitionDirectoryNameFragment( + ImmutableList.of("//command_line_option:foo=foosball"))); + + assertThat(getTransitionDirectoryNameFragment(dep)) + .isEqualTo( + BuildConfigurationFunction.transitionDirectoryNameFragment( ImmutableList.of( "//command_line_option:bar=barsball", "//command_line_option:foo=foosball"))); } @@ -1648,11 +1677,11 @@ assertThat(getTransitionDirectoryNameFragment(test)) .isEqualTo( - FunctionTransitionUtil.transitionDirectoryNameFragment( + BuildConfigurationFunction.transitionDirectoryNameFragment( ImmutableList.of("//test:foo=1"))); assertThat(getTransitionDirectoryNameFragment(dep)) .isEqualTo( - FunctionTransitionUtil.transitionDirectoryNameFragment( + BuildConfigurationFunction.transitionDirectoryNameFragment( ImmutableList.of("//test:foo=true"))); } @@ -1704,8 +1733,7 @@ .isEqualTo(getTransitionDirectoryNameFragment(dep)); } - @Test - public void testOutputDirHash_multipleStarlarkTransitions() throws Exception { + private void writeFilesWithMultipleStarlarkTransitions() throws Exception { writeAllowlistFile(); scratch.file( "test/transitions.bzl", @@ -1746,7 +1774,13 @@ "simple(name = 'dep')", "string_flag(name = 'foo', build_setting_default = '')", "string_flag(name = 'bar', build_setting_default = '')"); + } + @Test + public void testOutputDirHash_multipleStarlarkTransitions_legacyNaming() throws Exception { + writeFilesWithMultipleStarlarkTransitions(); + + useConfiguration("--experimental_output_directory_naming_scheme=legacy"); ConfiguredTarget test = getConfiguredTarget("//test"); @SuppressWarnings("unchecked") @@ -1760,18 +1794,37 @@ assertThat(affectedOptions).containsExactly("//test:bar", "//test:foo"); assertThat(getTransitionDirectoryNameFragment(test)) .isEqualTo( - FunctionTransitionUtil.transitionDirectoryNameFragment( + BuildConfigurationFunction.transitionDirectoryNameFragment( ImmutableList.of("//test:foo=foosball"))); assertThat(getTransitionDirectoryNameFragment(dep)) .isEqualTo( - FunctionTransitionUtil.transitionDirectoryNameFragment( + BuildConfigurationFunction.transitionDirectoryNameFragment( ImmutableList.of("//test:bar=barsball", "//test:foo=foosball"))); } - // This test is massive but mostly exists to ensure that all the parts are working together - // properly amidst multiple complicated transitions. @Test - public void testOutputDirHash_multipleMixedTransitions() throws Exception { + public void testOutputDirHash_multipleStarlarkTransitions_diffNaming() throws Exception { + writeFilesWithMultipleStarlarkTransitions(); + + useConfiguration("--experimental_output_directory_naming_scheme=diff_against_baseline"); + ConfiguredTarget test = getConfiguredTarget("//test"); + + @SuppressWarnings("unchecked") + ConfiguredTarget dep = + Iterables.getOnlyElement( + (List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep")); + + assertThat(getTransitionDirectoryNameFragment(test)) + .isEqualTo( + BuildConfigurationFunction.transitionDirectoryNameFragment( + ImmutableList.of("//test:foo=foosball"))); + assertThat(getTransitionDirectoryNameFragment(dep)) + .isEqualTo( + BuildConfigurationFunction.transitionDirectoryNameFragment( + ImmutableList.of("//test:bar=barsball", "//test:foo=foosball"))); + } + + private void writeFilesWithMultipleMixedTransitions() throws Exception { writeAllowlistFile(); scratch.file( "test/transitions.bzl", @@ -1837,8 +1890,16 @@ "simple(name = 'bottom')", "string_flag(name = 'zee', build_setting_default = '')", "string_flag(name = 'xan', build_setting_default = '')"); + } + + // This test is massive but mostly exists to ensure that all the parts are working together + // properly amidst multiple complicated transitions. + @Test + public void testOutputDirHash_multipleMixedTransitions_legacyNaming() throws Exception { + writeFilesWithMultipleMixedTransitions(); // test:top (foo_transition) + useConfiguration("--experimental_output_directory_naming_scheme=legacy"); ConfiguredTarget top = getConfiguredTarget("//test:top"); List<String> affectedOptionsTop = @@ -1848,7 +1909,7 @@ assertThat(getConfiguration(top).getOptions().getStarlarkOptions()).isEmpty(); assertThat(getTransitionDirectoryNameFragment(top)) .isEqualTo( - FunctionTransitionUtil.transitionDirectoryNameFragment( + BuildConfigurationFunction.transitionDirectoryNameFragment( ImmutableList.of("//command_line_option:foo=foosball"))); // test:middle (foo_transition, zee_transition, bar_transition) @@ -1868,7 +1929,7 @@ assertThat(getTransitionDirectoryNameFragment(middle)) .isEqualTo( - FunctionTransitionUtil.transitionDirectoryNameFragment( + BuildConfigurationFunction.transitionDirectoryNameFragment( ImmutableList.of( "//command_line_option:bar=barsball", "//command_line_option:foo=foosball", @@ -1893,7 +1954,56 @@ Maps.immutableEntry(Label.parseAbsoluteUnchecked("//test:xan"), "xansball")); assertThat(getTransitionDirectoryNameFragment(bottom)) .isEqualTo( - FunctionTransitionUtil.transitionDirectoryNameFragment( + BuildConfigurationFunction.transitionDirectoryNameFragment( + ImmutableList.of( + "//command_line_option:bar=barsball", "//command_line_option:foo=foosball", + "//test:xan=xansball", "//test:zee=zeesball"))); + } + + @Test + public void testOutputDirHash_multipleMixedTransitions_diffNaming() throws Exception { + writeFilesWithMultipleMixedTransitions(); + + // test:top (foo_transition) + useConfiguration("--experimental_output_directory_naming_scheme=diff_against_baseline"); + ConfiguredTarget top = getConfiguredTarget("//test:top"); + + assertThat(getConfiguration(top).getOptions().getStarlarkOptions()).isEmpty(); + assertThat(getTransitionDirectoryNameFragment(top)) + .isEqualTo( + BuildConfigurationFunction.transitionDirectoryNameFragment( + ImmutableList.of("//command_line_option:foo=foosball"))); + + // test:middle (foo_transition, zee_transition, bar_transition) + @SuppressWarnings("unchecked") + ConfiguredTarget middle = + Iterables.getOnlyElement((List<ConfiguredTarget>) getMyInfoFromTarget(top).getValue("dep")); + + assertThat(getConfiguration(middle).getOptions().getStarlarkOptions().entrySet()) + .containsExactly( + Maps.immutableEntry(Label.parseAbsoluteUnchecked("//test:zee"), "zeesball")); + + assertThat(getTransitionDirectoryNameFragment(middle)) + .isEqualTo( + BuildConfigurationFunction.transitionDirectoryNameFragment( + ImmutableList.of( + "//command_line_option:bar=barsball", + "//command_line_option:foo=foosball", + "//test:zee=zeesball"))); + + // test:bottom (foo_transition, zee_transition, bar_transition, xan_transition) + @SuppressWarnings("unchecked") + ConfiguredTarget bottom = + Iterables.getOnlyElement( + (List<ConfiguredTarget>) getMyInfoFromTarget(middle).getValue("dep")); + + assertThat(getConfiguration(bottom).getOptions().getStarlarkOptions().entrySet()) + .containsExactly( + Maps.immutableEntry(Label.parseAbsoluteUnchecked("//test:zee"), "zeesball"), + Maps.immutableEntry(Label.parseAbsoluteUnchecked("//test:xan"), "xansball")); + assertThat(getTransitionDirectoryNameFragment(bottom)) + .isEqualTo( + BuildConfigurationFunction.transitionDirectoryNameFragment( ImmutableList.of( "//command_line_option:bar=barsball", "//command_line_option:foo=foosball", "//test:xan=xansball", "//test:zee=zeesball"))); @@ -2222,7 +2332,12 @@ "my_rule(name = 'dep')", "my_flag(name = 'flag', build_setting_default = '" + buildSettingsDefault + "')"); - useConfiguration(ImmutableMap.of("//test:flag", "frisbee")); + // TODO(blaze-configurability-team): There is a bug in BuildViewTestCase that it does not audit + // these Starlark options at all (i.e. check they are the right type or that values at default + // are unset/set to null rather than explicitly set to the default. + if (!buildSettingsDefault.equals("frisbee")) { + useConfiguration(ImmutableMap.of("//test:flag", "frisbee")); + } ConfiguredTarget test = getConfiguredTarget("//test"); @SuppressWarnings("unchecked")
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationFunctionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationFunctionTest.java new file mode 100644 index 0000000..2d50b5e --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationFunctionTest.java
@@ -0,0 +1,257 @@ +// Copyright 2022 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.analysis.config; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.analysis.ConfiguredTarget; +import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; +import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.packages.Provider; +import com.google.devtools.build.lib.packages.StarlarkProvider; +import com.google.devtools.build.lib.packages.StructImpl; +import com.google.devtools.build.lib.skyframe.BuildConfigurationFunction; +import java.util.List; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for {@link BuildConfigurationFunction}'s special behaviors. */ +@RunWith(JUnit4.class) +public final class BuildConfigurationFunctionTest extends BuildViewTestCase { + + @Before + public void setupMyInfo() throws Exception { + scratch.file("myinfo/myinfo.bzl", "MyInfo = provider()"); + + scratch.file("myinfo/BUILD"); + } + + private static StructImpl getMyInfoFromTarget(ConfiguredTarget configuredTarget) + throws Exception { + Provider.Key key = + new StarlarkProvider.Key( + Label.parseAbsolute("//myinfo:myinfo.bzl", ImmutableMap.of()), "MyInfo"); + return (StructImpl) configuredTarget.get(key); + } + + private void writeAllowlistFile() throws Exception { + scratch.overwriteFile( + "tools/allowlists/function_transition_allowlist/BUILD", + "package_group(", + " name = 'function_transition_allowlist',", + " packages = [", + " '//test/...',", + " ],", + ")"); + } + + private void writeBuildSettingsBzl() throws Exception { + scratch.file( + "test/build_settings.bzl", + "BuildSettingInfo = provider(fields = ['value'])", + "def _impl(ctx):", + " return [BuildSettingInfo(value = ctx.build_setting_value)]", + "string_flag = rule(implementation = _impl, build_setting = config.string(flag=True))"); + } + + private CoreOptions getCoreOptions(ConfiguredTarget target) { + return getConfiguration(target).getOptions().get(CoreOptions.class); + } + + private String getTransitionDirectoryNameFragment(ConfiguredTarget target) { + return getConfiguration(target).getTransitionDirectoryNameFragment(); + } + + @Test + public void testDiffAgainstBaselineOutputScheme_hasHash() throws Exception { + writeAllowlistFile(); + writeBuildSettingsBzl(); + scratch.file( + "test/transitions.bzl", + "def _foo_impl(settings, attr):", + " return {'//test:foo': 'transitioned'}", + "foo_transition = transition(implementation = _foo_impl, inputs = [],", + " outputs = ['//test:foo'])"); + scratch.file( + "test/rules.bzl", + "load('//myinfo:myinfo.bzl', 'MyInfo')", + "load('//test:transitions.bzl', 'foo_transition')", + "def _impl(ctx):", + " return MyInfo(dep = ctx.attr.dep)", + "my_rule = rule(", + " implementation = _impl,", + " attrs = {", + " 'dep': attr.label(cfg = foo_transition), ", + " '_allowlist_function_transition': attr.label(", + " default = '//tools/allowlists/function_transition_allowlist',", + " ),", + " })", + "def _basic_impl(ctx):", + " return []", + "simple = rule(_basic_impl)"); + scratch.file( + "test/BUILD", + "load('//test:rules.bzl', 'my_rule', 'simple')", + "load('//test:build_settings.bzl', 'string_flag')", + "string_flag(name = 'foo', build_setting_default='default')", + "my_rule(name = 'test', dep = ':dep')", + "simple(name = 'dep')"); + + useConfiguration("--experimental_output_directory_naming_scheme=diff_against_baseline"); + ConfiguredTarget test = getConfiguredTarget("//test"); + + assertThat(getTransitionDirectoryNameFragment(test)).isEmpty(); + assertThat(getCoreOptions(test).affectedByStarlarkTransition).isEmpty(); + + @SuppressWarnings("unchecked") + ConfiguredTarget dep = + Iterables.getOnlyElement( + (List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep")); + + assertThat(getTransitionDirectoryNameFragment(dep)) + .isEqualTo( + BuildConfigurationFunction.transitionDirectoryNameFragment( + ImmutableList.of("//test:foo=transitioned"))); + assertThat(getCoreOptions(dep).affectedByStarlarkTransition).isEmpty(); + } + + @Test + public void testDiffAgainstBaselineOutputScheme_avoidHashForInExplicitOutputPath() + throws Exception { + writeAllowlistFile(); + scratch.file( + "test/transitions.bzl", + "def _opt_impl(settings, attr):", + " return {'//command_line_option:compilation_mode': 'opt'}", + "opt_transition = transition(implementation = _opt_impl, inputs = [],", + " outputs = ['//command_line_option:compilation_mode'])"); + scratch.file( + "test/rules.bzl", + "load('//myinfo:myinfo.bzl', 'MyInfo')", + "load('//test:transitions.bzl', 'opt_transition')", + "def _impl(ctx):", + " return MyInfo(dep = ctx.attr.dep)", + "my_rule = rule(", + " implementation = _impl,", + " attrs = {", + " 'dep': attr.label(cfg = opt_transition), ", + " '_allowlist_function_transition': attr.label(", + " default = '//tools/allowlists/function_transition_allowlist',", + " ),", + " })", + "def _basic_impl(ctx):", + " return []", + "simple = rule(_basic_impl)"); + scratch.file( + "test/BUILD", + "load('//test:rules.bzl', 'my_rule', 'simple')", + "my_rule(name = 'test', dep = ':dep')", + "simple(name = 'dep')"); + + useConfiguration( + "--compilation_mode=fastbuild", + "--experimental_output_directory_naming_scheme=diff_against_baseline"); + ConfiguredTarget test = getConfiguredTarget("//test"); + + assertThat(getConfiguration(test).getMnemonic()).contains("fastbuild"); + assertThat(getTransitionDirectoryNameFragment(test)).isEmpty(); + assertThat(getCoreOptions(test).affectedByStarlarkTransition).isEmpty(); + + @SuppressWarnings("unchecked") + ConfiguredTarget dep = + Iterables.getOnlyElement( + (List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep")); + + assertThat(getConfiguration(dep).getMnemonic()).contains("opt"); + assertThat(getTransitionDirectoryNameFragment(dep)).isEmpty(); + assertThat(getCoreOptions(dep).affectedByStarlarkTransition).isEmpty(); + } + + @Test + public void testDiffAgainstBaselineOutputScheme_abaAvoidsHash() throws Exception { + writeAllowlistFile(); + writeBuildSettingsBzl(); + scratch.file( + "test/transitions.bzl", + "def _toggle_impl(settings, attr):", + " if (settings['//test:foo'] != 'default'):", + " return {'//test:foo': 'default'}", + " else:", + " return {'//test:foo': 'transitioned'}", + "toggle_foo_transition = transition(implementation = _toggle_impl,", + " inputs = ['//test:foo'], outputs = ['//test:foo'])"); + scratch.file( + "test/rules.bzl", + "load('//myinfo:myinfo.bzl', 'MyInfo')", + "load('//test:transitions.bzl', 'toggle_foo_transition')", + "def _impl(ctx):", + " return MyInfo(dep = ctx.attr.dep)", + "my_rule = rule(", + " implementation = _impl,", + " attrs = {", + " 'dep': attr.label(cfg = toggle_foo_transition), ", + " '_allowlist_function_transition': attr.label(", + " default = '//tools/allowlists/function_transition_allowlist',", + " ),", + " })", + "def _basic_impl(ctx):", + " return []", + "simple = rule(_basic_impl)"); + scratch.file( + "test/BUILD", + "load('//test:rules.bzl', 'my_rule', 'simple')", + "load('//test:build_settings.bzl', 'string_flag')", + "string_flag(name = 'foo', build_setting_default='default')", + "my_rule(name = 'test', dep = ':middle')", + "my_rule(name = 'middle', dep = ':root')", + "simple(name = 'root')"); + + useConfiguration("--experimental_output_directory_naming_scheme=diff_against_baseline"); + ConfiguredTarget test = getConfiguredTarget("//test"); + + assertThat(getTransitionDirectoryNameFragment(test)).isEmpty(); + assertThat(getCoreOptions(test).affectedByStarlarkTransition).isEmpty(); + + @SuppressWarnings("unchecked") + ConfiguredTarget middle = + Iterables.getOnlyElement( + (List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep")); + + assertThat(getTransitionDirectoryNameFragment(middle)) + .isEqualTo( + BuildConfigurationFunction.transitionDirectoryNameFragment( + ImmutableList.of("//test:foo=transitioned"))); + assertThat(getCoreOptions(middle).affectedByStarlarkTransition).isEmpty(); + + @SuppressWarnings("unchecked") + ConfiguredTarget root = + Iterables.getOnlyElement( + (List<ConfiguredTarget>) getMyInfoFromTarget(middle).getValue("dep")); + + assertThat(getTransitionDirectoryNameFragment(test)).isEmpty(); + assertThat(getCoreOptions(test).affectedByStarlarkTransition).isEmpty(); + + assertThat(getConfiguration(test)).isEqualTo(getConfiguration(root)); + assertThat(getConfiguration(test)).isNotEqualTo(getConfiguration(middle)); + + // This should be implied by everything else but as a final check.... + assertThat(getConfiguration(test).getMnemonic()) + .isEqualTo(getConfiguration(root).getMnemonic()); + } +}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValueTest.java b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValueTest.java index 2f3e339..e50d18f 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValueTest.java
@@ -389,16 +389,20 @@ // these configurations are never trimmed nor even used to build targets so not an issue. new EqualsTester() .addEqualityGroup( - createRaw(parseBuildOptions("--test_arg=1a"), "@testrepo", false), - createRaw(parseBuildOptions("--test_arg=1a"), "@testrepo", false)) + createRaw(parseBuildOptions("--test_arg=1a"), "@testrepo", false, ""), + createRaw(parseBuildOptions("--test_arg=1a"), "@testrepo", false, "")) // Different BuildOptions means non-equal - .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=1b"), "@testrepo", false)) + .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=1b"), "@testrepo", false, "")) // Different --experimental_sibling_repository_layout means non-equal - .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=2"), "@testrepo", true)) - .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=2"), "@testrepo", false)) + .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=2"), "@testrepo", true, "")) + .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=2"), "@testrepo", false, "")) // Different repositoryName means non-equal - .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=3"), "@testrepo1", false)) - .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=3"), "@testrepo2", false)) + .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=3"), "@testrepo1", false, "")) + .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=3"), "@testrepo2", false, "")) + // Different transitionDirectoryNameFragment means non-equal + .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=3"), "@testrepo", false, "")) + .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=3"), "@testrepo", false, "a")) + .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=3"), "@testrepo", false, "b")) .testEquals(); }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java index ed446e5..cc2cdf7 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
@@ -472,6 +472,9 @@ // TODO(blaze-configurability): It would be nice to be able to do some starlark options loading // to ensure that the values given in this map are the right types for their keys. + // TODO(blaze-configurability): It is actually incorrect that build options are potentially + // being explicitly set to their default values. In production, Starlark options set to their + // default are always null (and various parts of the infra rely on this). optionsParser.setStarlarkOptions(starlarkOptions); BuildOptions buildOptions =
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/ConfigurationTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/ConfigurationTestCase.java index 8341714..e07299b 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/ConfigurationTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/ConfigurationTestCase.java
@@ -231,12 +231,16 @@ /** Returns a raw {@link BuildConfigurationValue} with the given parameters. */ protected BuildConfigurationValue createRaw( - BuildOptions buildOptions, String repositoryName, boolean siblingRepositoryLayout) + BuildOptions buildOptions, + String repositoryName, + boolean siblingRepositoryLayout, + String transitionDirectoryNameFragment) throws Exception { return BuildConfigurationValue.create( buildOptions, RepositoryName.create(repositoryName), siblingRepositoryLayout, + transitionDirectoryNameFragment, skyframeExecutor.getBlazeDirectoriesForTesting(), skyframeExecutor.getRuleClassProviderForTesting(), fragmentFactory);
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/BuildEventStreamerTest.java b/src/test/java/com/google/devtools/build/lib/runtime/BuildEventStreamerTest.java index fd26370..9110ce0 100644 --- a/src/test/java/com/google/devtools/build/lib/runtime/BuildEventStreamerTest.java +++ b/src/test/java/com/google/devtools/build/lib/runtime/BuildEventStreamerTest.java
@@ -949,6 +949,7 @@ defaultBuildOptions, RepositoryName.createFromValidStrippedName("workspace"), /*siblingRepositoryLayout=*/ false, + /*transitionDirectoryNameFragment=*/ "", new BlazeDirectories( new ServerDirectories(outputBase, outputBase, outputBase), rootDirectory,