Skip to content

Commit

Permalink
Fixes for the Starlark transition hash computation (#14251)
Browse files Browse the repository at this point in the history
* Move transitionDirectoryNameFragment calculation to BuildConfigurationValue

As per discussion in b/203470434, transitionDirectoryNameFragment should completely depend on the current values of the (rest of) the BuildOptions class. Thus, it is far better to have this always computed from BuildOptions when building a BuildConfigurationValue  than rely on users keeping it consistent. (Other results of BuildConfigurationValue itself are themselves wholly computed from BuildOptions so placement there is a natural fit.)

This naturally fixes the exec transition forgetting to update transitionDirectoryNameFragment.

This fixes and subsumes #13464 and #13915, respectively. This is related to #14023

PiperOrigin-RevId: 407913175

* Properly account for StarlarkOptions at their default (=null) when calculating ST-hash

Previously, the hash calculation was skipping including StarlarkOptions that happened to be at their default values.

This is wrong since those values may still be in "affected by Starlark transition" (because either the commandline set them and the Starlark transition reset them to their Starlark defaults thus still requiring a hash change OR the commandline did not set them but a series of Starlark transitions did an default->B->default anyways causing the Starlark option to still be 'stuck' in "affected by Starlark transition").

Resolves #14239

PiperOrigin-RevId: 408701552

Co-authored-by: twigg <[email protected]>
  • Loading branch information
fmeum and twigg authored Nov 11, 2021
1 parent c500e7a commit 557a7e7
Show file tree
Hide file tree
Showing 8 changed files with 178 additions and 74 deletions.
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -1526,6 +1526,7 @@ java_library(
":config/run_under",
":config/transitive_option_details",
":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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
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;
Expand Down Expand Up @@ -67,7 +68,7 @@
*
* <p>Instances of BuildConfiguration are canonical:
*
* <pre>c1.equals(c2) <=> c1==c2.</pre>
* <pre>{@code c1.equals(c2) <=> c1==c2.}</pre>
*/
// TODO(janakr): If overhead of fragments class names is too high, add constructor that just takes
// fragments and gets names from them.
Expand Down Expand Up @@ -107,6 +108,14 @@ public interface ActionEnvironmentProvider {
private final BuildOptions buildOptions;
private final CoreOptions options;

/**
* If non-empty, this is appended to output directories as ST-[transitionDirectoryNameFragment].
* The value is a hash of BuildOptions that have been affected by a Starlark transition.
*
* <p>See b/203470434 or #14023 for more information and planned behavior changes.
*/
private final String transitionDirectoryNameFragment;

private final ImmutableMap<String, String> commandLineBuildVariables;

private final int hashCode; // We can precompute the hash code as all its inputs are immutable.
Expand Down Expand Up @@ -189,14 +198,17 @@ public BuildConfiguration(
if (buildOptions.contains(PlatformOptions.class)) {
platformOptions = buildOptions.get(PlatformOptions.class);
}
this.transitionDirectoryNameFragment =
FunctionTransitionUtil.computeOutputDirectoryNameFragment(buildOptions);
this.outputDirectories =
new OutputDirectories(
directories,
options,
platformOptions,
this.fragments,
mainRepositoryName,
siblingRepositoryLayout);
siblingRepositoryLayout,
transitionDirectoryNameFragment);
this.mainRepositoryName = mainRepositoryName;
this.siblingRepositoryLayout = siblingRepositoryLayout;

Expand Down Expand Up @@ -403,6 +415,11 @@ public String getMnemonic() {
return outputDirectories.getMnemonic();
}

@VisibleForTesting
public String getTransitionDirectoryNameFragment() {
return transitionDirectoryNameFragment;
}

@Override
public String toString() {
return checksum();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -240,22 +240,6 @@ public class CoreOptions extends FragmentOptions implements Cloneable {
+ "'fastbuild', 'dbg', 'opt'.")
public CompilationMode hostCompilationMode;

/**
* This option is used by starlark transitions to add a distinguishing element to the output
* directory name, in order to avoid name clashing.
*/
@Option(
name = "transition directory name fragment",
defaultValue = "null",
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {
OptionEffectTag.LOSES_INCREMENTAL_STATE,
OptionEffectTag.AFFECTS_OUTPUTS,
OptionEffectTag.LOADING_AND_ANALYSIS
},
metadataTags = {OptionMetadataTag.INTERNAL})
public String transitionDirectoryNameFragment;

@Option(
name = "experimental_enable_aspect_hints",
defaultValue = "false",
Expand Down Expand Up @@ -839,7 +823,6 @@ public IncludeConfigFragmentsEnumConverter() {
public FragmentOptions getHost() {
CoreOptions host = (CoreOptions) getDefault();

host.transitionDirectoryNameFragment = transitionDirectoryNameFragment;
host.affectedByStarlarkTransition = affectedByStarlarkTransition;
host.compilationMode = hostCompilationMode;
host.isHost = true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,10 +147,12 @@ public ArtifactRoot getRoot(
@Nullable PlatformOptions platformOptions,
ImmutableSortedMap<Class<? extends Fragment>, Fragment> fragments,
RepositoryName mainRepositoryName,
boolean siblingRepositoryLayout)
boolean siblingRepositoryLayout,
String transitionDirectoryNameFragment)
throws InvalidMnemonicException {
this.directories = directories;
this.mnemonic = buildMnemonic(options, platformOptions, fragments);
this.mnemonic =
buildMnemonic(options, platformOptions, fragments, transitionDirectoryNameFragment);
this.outputDirName = options.isHost ? "host" : mnemonic;

this.outputDirectory =
Expand Down Expand Up @@ -207,7 +209,8 @@ private static void validateMnemonicPart(String part, String errorTemplate, Obje
private static String buildMnemonic(
CoreOptions options,
@Nullable PlatformOptions platformOptions,
ImmutableSortedMap<Class<? extends Fragment>, Fragment> fragments)
ImmutableSortedMap<Class<? extends Fragment>, Fragment> fragments,
String transitionDirectoryNameFragment)
throws InvalidMnemonicException {
// See explanation at declaration for outputRoots.
List<String> nameParts = new ArrayList<>();
Expand All @@ -230,9 +233,7 @@ private static String buildMnemonic(

// Add the transition suffix.
addMnemonicPart(
nameParts,
options.transitionDirectoryNameFragment,
"Transition directory name fragment '%s'");
nameParts, transitionDirectoryNameFragment, "Transition directory name fragment '%s'");

// Join all the parts.
String mnemonic = nameParts.stream().filter(not(Strings::isNullOrEmpty)).collect(joining("-"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

package com.google.devtools.build.lib.analysis.starlark;

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.stream.Collectors.joining;
Expand Down Expand Up @@ -57,7 +58,7 @@
* Utility class for common work done across {@link StarlarkAttributeTransitionProvider} and {@link
* StarlarkRuleTransitionProvider}.
*/
public class FunctionTransitionUtil {
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.
Expand Down Expand Up @@ -165,7 +166,7 @@ static ImmutableMap<String, OptionInfo> buildOptionInfo(BuildOptions buildOption
ImmutableSet<Class<? extends FragmentOptions>> optionClasses =
buildOptions.getNativeOptions().stream()
.map(FragmentOptions::getClass)
.collect(ImmutableSet.toImmutableSet());
.collect(toImmutableSet());

for (Class<? extends FragmentOptions> optionClass : optionClasses) {
ImmutableList<OptionDefinition> optionDefinitions =
Expand Down Expand Up @@ -394,7 +395,8 @@ private static BuildOptions applyTransition(
convertedNewValues.add("//command_line_option:evaluating for analysis test");
toOptions.get(CoreOptions.class).evaluatingForAnalysisTest = true;
}
updateOutputDirectoryNameFragment(convertedNewValues, optionInfoMap, toOptions);

updateAffectedByStarlarkTransition(toOptions.get(CoreOptions.class), convertedNewValues);
return toOptions;
}

Expand All @@ -404,27 +406,20 @@ private static BuildOptions applyTransition(
* the build by Starlark transitions. Options only set on command line are not affecting the
* computation.
*
* @param changedOptions the names of all options changed by this transition in label form e.g.
* "//command_line_option:cpu" for native options and "//myapp:foo" for starlark options.
* @param optionInfoMap a map of all native options (name -> OptionInfo) present in {@code
* toOptions}.
* @param toOptions the newly transitioned {@link BuildOptions} for which we need to updated
* {@code transitionDirectoryNameFragment} and {@code affectedByStarlarkTransition}.
* @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.
private static void updateOutputDirectoryNameFragment(
Set<String> changedOptions, Map<String, OptionInfo> optionInfoMap, BuildOptions toOptions) {
// Return without doing anything if this transition hasn't changed any option values.
if (changedOptions.isEmpty()) {
return;
}

public static String computeOutputDirectoryNameFragment(BuildOptions toOptions) {
CoreOptions buildConfigOptions = toOptions.get(CoreOptions.class);

updateAffectedByStarlarkTransition(buildConfigOptions, changedOptions);
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) {
Expand All @@ -445,19 +440,21 @@ private static void updateOutputDirectoryNameFragment(
toHash.put(optionName, value);
} else {
Object value = toOptions.getStarlarkOptions().get(Label.parseAbsoluteUnchecked(optionName));
if (value != null) {
toHash.put(optionName, value);
}
toHash.put(optionName, value);
}
}

ImmutableList.Builder<String> hashStrs = ImmutableList.builderWithExpectedSize(toHash.size());
for (Map.Entry<String, Object> singleOptionAndValue : toHash.entrySet()) {
String toAdd = singleOptionAndValue.getKey() + "=" + singleOptionAndValue.getValue();
hashStrs.add(toAdd);
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");
}
}
buildConfigOptions.transitionDirectoryNameFragment =
transitionDirectoryNameFragment(hashStrs.build());
return transitionDirectoryNameFragment(hashStrs.build());
}

/**
Expand All @@ -466,6 +463,9 @@ private static void updateOutputDirectoryNameFragment(
*/
private static void updateAffectedByStarlarkTransition(
CoreOptions buildConfigOptions, Set<String> changedOptions) {
if (changedOptions.isEmpty()) {
return;
}
Set<String> mutableCopyToUpdate =
new TreeSet<>(buildConfigOptions.affectedByStarlarkTransition);
mutableCopyToUpdate.addAll(changedOptions);
Expand Down Expand Up @@ -503,4 +503,6 @@ OptionDefinition getDefinition() {
return definition;
}
}

private FunctionTransitionUtil() {}
}
Loading

0 comments on commit 557a7e7

Please sign in to comment.