From 5eefd23b0a0710e717ac09651df295c393bc5c03 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Tue, 9 Nov 2021 12:18:42 +0100 Subject: [PATCH] Draft: Compute affectedByStarlarkTransition in BuildConfigurationValue This includes debug prints and has at least the following deficencies: * no regard for Skyframe intricacies (e.g. no equals implementation for BuildConfigurationValue). * no normalization of hashed BuildOptions options (akin to what happens in FunctionTransitionUtil#applyTransition). --- .../config/BuildConfigurationValue.java | 41 ++++++++- .../lib/analysis/config/CoreOptions.java | 35 -------- .../starlark/FunctionTransitionUtil.java | 88 ++++++++----------- .../skyframe/BuildConfigurationFunction.java | 23 ++++- .../build/lib/skyframe/SkyframeExecutor.java | 2 +- .../StarlarkAttrTransitionProviderTest.java | 32 +------ .../lib/runtime/BuildEventStreamerTest.java | 3 +- 7 files changed, 100 insertions(+), 124 deletions(-) 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 91f328245129a0..2aedd4406fce11 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 @@ -153,6 +153,10 @@ private ActionEnvironment setupTestEnvironment() { return ActionEnvironment.split(testEnv); } + /** + * Only used for AutoCodec, construct an instance via + * {@link BuildConfigurationValue#createBuildConfigurationValue} instead. + */ public BuildConfigurationValue( BlazeDirectories directories, ImmutableMap, Fragment> fragments, @@ -161,7 +165,8 @@ public BuildConfigurationValue( ImmutableSet reservedActionMnemonics, ActionEnvironment actionEnvironment, RepositoryName mainRepositoryName, - boolean siblingRepositoryLayout) + boolean siblingRepositoryLayout, + String transitionDirectoryNameFragment) throws InvalidMnemonicException { this.fragments = fragmentsInterner.intern( @@ -174,8 +179,7 @@ public BuildConfigurationValue( if (buildOptions.contains(PlatformOptions.class)) { platformOptions = buildOptions.get(PlatformOptions.class); } - this.transitionDirectoryNameFragment = - FunctionTransitionUtil.computeOutputDirectoryNameFragment(buildOptions); + this.transitionDirectoryNameFragment = transitionDirectoryNameFragment; this.outputDirectories = new OutputDirectories( directories, @@ -221,6 +225,37 @@ public BuildConfigurationValue( this.commandLineLimits = new CommandLineLimits(options.minParamFileSize); } + /** + * Constructs an instance of {@link BuildConfigurationValue} with the + * transitionDirectoryNameFragment computed from the buildOptions and topLevelBuildOptions. + *

+ * This factory method exists so that AutoCodec does not store the topLevelBuildOptions for each + * BuildConfigurationValue. + */ + public static BuildConfigurationValue createBuildConfigurationValue( + BlazeDirectories directories, + ImmutableMap, Fragment> fragments, + FragmentClassSet fragmentClassSet, + BuildOptions buildOptions, + BuildOptions topLevelBuildOptions, + ImmutableSet reservedActionMnemonics, + ActionEnvironment actionEnvironment, + RepositoryName mainRepositoryName, + boolean siblingRepositoryLayout) throws InvalidMnemonicException { + return new BuildConfigurationValue( + directories, + fragments, + fragmentClassSet, + buildOptions, + reservedActionMnemonics, + actionEnvironment, + mainRepositoryName, + siblingRepositoryLayout, + FunctionTransitionUtil.computeOutputDirectoryNameFragment(buildOptions, + topLevelBuildOptions) + ); + } + private ImmutableMap> buildIndexOfStarlarkVisibleFragments() { ImmutableMap.Builder> builder = ImmutableMap.builder(); 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 b9fb5bcf285eb5..bd293323d857a3 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 @@ -248,40 +248,6 @@ public class CoreOptions extends FragmentOptions implements Cloneable { metadataTags = {OptionMetadataTag.EXPERIMENTAL}) public boolean enableAspectHints; - /** Regardless of input, converts to an empty list. For use with affectedByStarlarkTransition */ - public static class EmptyListConverter implements Converter> { - @Override - public List convert(String input) throws OptionsParsingException { - return ImmutableList.of(); - } - - @Override - public String getTypeDescription() { - return "Regardless of input, converts to an empty list. For use with" - + " affectedByStarlarkTransition"; - } - } - - /** - * This internal option is a *set* of names of options that have been changed by starlark - * transitions at any point in the build at the time of accessing. It contains both native and - * starlark options in label form. e.g. "//command_line_option:cpu" for native options and - * "//myapp:foo" for starlark options. This is used to regenerate {@code - * transitionDirectoryNameFragment} after each starlark transition. - */ - @Option( - name = "affected by starlark transition", - defaultValue = "", - converter = EmptyListConverter.class, - documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, - effectTags = { - OptionEffectTag.LOSES_INCREMENTAL_STATE, - OptionEffectTag.AFFECTS_OUTPUTS, - OptionEffectTag.LOADING_AND_ANALYSIS - }, - metadataTags = {OptionMetadataTag.INTERNAL}) - public List affectedByStarlarkTransition; - @Option( name = "platform_suffix", defaultValue = "null", @@ -823,7 +789,6 @@ public IncludeConfigFragmentsEnumConverter() { public FragmentOptions getHost() { CoreOptions host = (CoreOptions) getDefault(); - host.affectedByStarlarkTransition = affectedByStarlarkTransition; host.compilationMode = hostCompilationMode; host.isHost = true; host.isExec = false; 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 48979fd45a818b..5b273011c8b29f 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 @@ -20,12 +20,12 @@ 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; import com.google.common.collect.Sets; import com.google.devtools.build.lib.analysis.config.BuildOptions; +import com.google.devtools.build.lib.analysis.config.BuildOptions.OptionsDiff; 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.StarlarkDefinedConfigTransition; @@ -44,9 +44,9 @@ import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.Map.Entry; import java.util.Set; import java.util.TreeMap; -import java.util.TreeSet; import javax.annotation.Nullable; import net.starlark.java.eval.Dict; import net.starlark.java.eval.EvalException; @@ -396,52 +396,51 @@ private static BuildOptions applyTransition( toOptions.get(CoreOptions.class).evaluatingForAnalysisTest = true; } - updateAffectedByStarlarkTransition(toOptions.get(CoreOptions.class), convertedNewValues); 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. + * names and values of all options (both native and Starlark) that differ from the options set on + * the command line. * - * @param toOptions the {@link BuildOptions} to use to calculate which we need to compute {@code - * transitionDirectoryNameFragment}. + * @param toOptions the {@link BuildOptions} to compute {@code transitionDirectoryNameFragment} + * for. + * @param topLevelOptions the {@link BuildOptions} representing the top-level configuration as + * determined by the command-line. */ // 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()) { + public static String computeOutputDirectoryNameFragment(BuildOptions toOptions, + BuildOptions topLevelOptions) { + OptionsDiff difference = BuildOptions.diff(toOptions, topLevelOptions); + + if (difference.getFirst().isEmpty()) { + // The current configuration is identical to the top-level configuration determined by the + // command line. Either we never transitioned away from it or a chain of transition has + // returned to an identical configuration. In both cases, there is no need to append a hash to + // the output directory. + System.err.println("Identical options\ntransitionDirectoryNameFragment = \"\"\n"); return ""; } - // TODO(blaze-configurability-team): A mild performance optimization would have this be global. - Map optionInfoMap = buildOptionInfo(toOptions); - TreeMap 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 { - value = - optionInfoMap - .get(nativeOptionName) - .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); - } + StringBuilder sb = new StringBuilder("Differing options:\n"); + for (Entry nativeOption : difference.getFirst().entrySet()) { + sb.append(String.format("%s: %s -> %s%n", + COMMAND_LINE_OPTION_PREFIX + nativeOption.getKey().getOptionName(), + difference.getSecond().get( + nativeOption.getKey()).toArray()[0], nativeOption.getValue())); + // FIXME: This should probably convert the option value similar to applyTransition. + toHash.put(COMMAND_LINE_OPTION_PREFIX + nativeOption.getKey().getOptionName(), + nativeOption.getValue()); + } + for (Label starlarkOption : difference.getChangedStarlarkOptions()) { + sb.append(String.format("%s: %s -> %s%n", starlarkOption.toString(), + topLevelOptions.getStarlarkOptions().get(starlarkOption), + toOptions.getStarlarkOptions().get(starlarkOption))); + toHash.put(starlarkOption.toString(), toOptions.getStarlarkOptions().get(starlarkOption)); } ImmutableList.Builder hashStrs = ImmutableList.builderWithExpectedSize(toHash.size()); @@ -449,23 +448,10 @@ public static String computeOutputDirectoryNameFragment(BuildOptions toOptions) String toAdd = singleOptionAndValue.getKey() + "=" + singleOptionAndValue.getValue(); hashStrs.add(toAdd); } - return transitionDirectoryNameFragment(hashStrs.build()); - } - - /** - * Extend the global build config affectedByStarlarkTransition, by adding any new option names - * from changedOptions - */ - private static void updateAffectedByStarlarkTransition( - CoreOptions buildConfigOptions, Set changedOptions) { - if (changedOptions.isEmpty()) { - return; - } - Set mutableCopyToUpdate = - new TreeSet<>(buildConfigOptions.affectedByStarlarkTransition); - mutableCopyToUpdate.addAll(changedOptions); - buildConfigOptions.affectedByStarlarkTransition = - ImmutableList.sortedCopyOf(mutableCopyToUpdate); + String stHash = transitionDirectoryNameFragment(hashStrs.build()); + sb.append(String.format("transitionDirectoryFragment = \"%s\"%n%n", stHash)); + System.err.println(sb); + return stHash; } public static String transitionDirectoryNameFragment(Iterable opts) { 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 821ca326f9f19d..ecd7f95602ae75 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 @@ -32,6 +32,7 @@ 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.skyframe.SkyframeExecutor.BuildViewProvider; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunctionException; import com.google.devtools.build.skyframe.SkyKey; @@ -47,13 +48,17 @@ public final class BuildConfigurationFunction implements SkyFunction { /** Cache with weak values can't have null values. */ private static final Fragment NULL_MARKER = new Fragment() {}; + private final BuildViewProvider buildViewProvider; private final BlazeDirectories directories; private final ConfiguredRuleClassProvider ruleClassProvider; private final LoadingCache fragmentCache = Caffeine.newBuilder().weakValues().build(BuildConfigurationFunction::makeFragment); public BuildConfigurationFunction( - BlazeDirectories directories, RuleClassProvider ruleClassProvider) { + BuildViewProvider buildViewProvider, + BlazeDirectories directories, + RuleClassProvider ruleClassProvider) { + this.buildViewProvider = buildViewProvider; this.directories = directories; this.ruleClassProvider = (ConfiguredRuleClassProvider) ruleClassProvider; } @@ -89,12 +94,26 @@ public SkyValue compute(SkyKey skyKey, Environment env) ActionEnvironment actionEnvironment = ruleClassProvider.getActionEnvironmentProvider().getActionEnvironment(key.getOptions()); + BuildConfigurationValue hostConfiguration = buildViewProvider.getSkyframeBuildView() + .getHostConfiguration(); + BuildOptions topLevelBuildOptions; + if (hostConfiguration != null) { + topLevelBuildOptions = hostConfiguration.getOptions(); + } else { + // hostConfiguration can be null early in the build (see the comment on + // SkyframeBuildView#getHostConfiguration. At that point it is probably safe to assume that no + // transition has taken place, so the current BuildOptions are still those set on the command + // line. + System.err.println("hostConfiguration was null"); + topLevelBuildOptions = key.getOptions(); + } try { - return new BuildConfigurationValue( + return BuildConfigurationValue.createBuildConfigurationValue( directories, fragments, fragmentClasses, key.getOptions(), + topLevelBuildOptions, ruleClassProvider.getReservedActionMnemonics(), actionEnvironment, RepositoryName.createFromValidStrippedName(workspaceNameValue.getName()), diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java index f7644e8ef83321..2bc41a9c64e0b5 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java @@ -583,7 +583,7 @@ private ImmutableMap skyFunctions() { new TopLevelActionLookupConflictFindingFunction()); map.put( SkyFunctions.BUILD_CONFIGURATION, - new BuildConfigurationFunction(directories, ruleClassProvider)); + new BuildConfigurationFunction(new BuildViewProvider(), directories, ruleClassProvider)); map.put(SkyFunctions.WORKSPACE_NAME, new WorkspaceNameFunction()); map.put( WorkspaceFileValue.WORKSPACE_FILE, 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 4ba6086c0ef8b8..610f3aaf735675 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 @@ -1277,20 +1277,11 @@ public void testOutputDirHash_multipleNativeOptionTransitions() throws Exception ConfiguredTarget test = getConfiguredTarget("//test"); - List affectedOptions = getCoreOptions(test).affectedByStarlarkTransition; - - assertThat(affectedOptions).containsExactly("//command_line_option:foo"); - @SuppressWarnings("unchecked") ConfiguredTarget dep = Iterables.getOnlyElement( (List) getMyInfoFromTarget(test).getValue("dep")); - affectedOptions = getCoreOptions(dep).affectedByStarlarkTransition; - - assertThat(affectedOptions) - .containsExactly("//command_line_option:foo", "//command_line_option:bar"); - assertThat(getTransitionDirectoryNameFragment(test)) .isEqualTo( FunctionTransitionUtil.transitionDirectoryNameFragment( @@ -1459,7 +1450,7 @@ public void testOutputDirHash_multipleStarlarkOptionTransitions_backToDefaultCom assertThat(getTransitionDirectoryNameFragment(dep)).isNotEmpty(); } - /** See comment above {@link FunctionTransitionUtil#updateOutputDirectoryNameFragment} */ + /** See comment above {@link FunctionTransitionUtil#computeOutputDirectoryNameFragment} */ // TODO(bazel-team): This can be optimized. Make these the same configuration. @Test public void testOutputDirHash_starlarkOption_differentBoolRepresentationsNotEquals() @@ -1619,10 +1610,6 @@ public void testOutputDirHash_multipleStarlarkTransitions() throws Exception { Iterables.getOnlyElement( (List) getMyInfoFromTarget(test).getValue("dep")); - List affectedOptions = - getConfiguration(dep).getOptions().get(CoreOptions.class).affectedByStarlarkTransition; - - assertThat(affectedOptions).containsExactly("//test:bar", "//test:foo"); assertThat(getTransitionDirectoryNameFragment(test)) .isEqualTo( FunctionTransitionUtil.transitionDirectoryNameFragment( @@ -1706,10 +1693,6 @@ public void testOutputDirHash_multipleMixedTransitions() throws Exception { // test:top (foo_transition) ConfiguredTarget top = getConfiguredTarget("//test:top"); - List affectedOptionsTop = - getConfiguration(top).getOptions().get(CoreOptions.class).affectedByStarlarkTransition; - - assertThat(affectedOptionsTop).containsExactly("//command_line_option:foo"); assertThat(getConfiguration(top).getOptions().getStarlarkOptions()).isEmpty(); assertThat(getTransitionDirectoryNameFragment(top)) .isEqualTo( @@ -1721,12 +1704,6 @@ public void testOutputDirHash_multipleMixedTransitions() throws Exception { ConfiguredTarget middle = Iterables.getOnlyElement((List) getMyInfoFromTarget(top).getValue("dep")); - List affectedOptionsMiddle = - getConfiguration(middle).getOptions().get(CoreOptions.class).affectedByStarlarkTransition; - - assertThat(affectedOptionsMiddle) - .containsExactly("//command_line_option:foo", "//command_line_option:bar", "//test:zee"); - assertThat(getConfiguration(middle).getOptions().getStarlarkOptions().entrySet()) .containsExactly( Maps.immutableEntry(Label.parseAbsoluteUnchecked("//test:zee"), "zeesball")); @@ -1745,13 +1722,6 @@ public void testOutputDirHash_multipleMixedTransitions() throws Exception { Iterables.getOnlyElement( (List) getMyInfoFromTarget(middle).getValue("dep")); - List affectedOptionsBottom = - getConfiguration(bottom).getOptions().get(CoreOptions.class).affectedByStarlarkTransition; - - assertThat(affectedOptionsBottom) - .containsExactly( - "//command_line_option:foo", "//command_line_option:bar", "//test:xan", "//test:zee"); - assertThat(getConfiguration(bottom).getOptions().getStarlarkOptions().entrySet()) .containsExactly( Maps.immutableEntry(Label.parseAbsoluteUnchecked("//test:zee"), "zeesball"), 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 aa193017128405..a92d1a7d27eabf 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 @@ -954,7 +954,8 @@ public void testReportedConfigurations() throws Exception { /*reservedActionMnemonics=*/ ImmutableSet.of(), ActionEnvironment.EMPTY, RepositoryName.createFromValidStrippedName("workspace"), - /*siblingRepositoryLayout=*/ false); + /*siblingRepositoryLayout=*/ false, + ""); BuildEvent firstWithConfiguration = new GenericConfigurationEvent(testId("first"), configuration.toBuildEvent()); BuildEvent secondWithConfiguration =