From 557a7e71eeb5396f2c87c909ddc025fde2678780 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 11 Nov 2021 11:42:03 +0100 Subject: [PATCH] Fixes for the Starlark transition hash computation (#14251) * 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 https://github.com/bazelbuild/bazel/issues/13464 and https://github.com/bazelbuild/bazel/pull/13915, respectively. This is related to https://github.com/bazelbuild/bazel/issues/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 https://github.com/bazelbuild/bazel/issues/14239 PiperOrigin-RevId: 408701552 Co-authored-by: twigg --- .../google/devtools/build/lib/analysis/BUILD | 1 + .../analysis/config/BuildConfiguration.java | 21 ++- .../lib/analysis/config/CoreOptions.java | 17 --- .../analysis/config/OutputDirectories.java | 13 +- .../starlark/FunctionTransitionUtil.java | 52 ++++---- .../StarlarkAttrTransitionProviderTest.java | 121 +++++++++++++++--- .../lib/analysis/util/DummyTestFragment.java | 10 ++ .../skyframe/PlatformMappingFunctionTest.java | 17 ++- 8 files changed, 178 insertions(+), 74 deletions(-) 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 b096dcb50d0893..eb84111920299d 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -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", diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java index 578af01bb76de3..af8bc0183e9091 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java @@ -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; @@ -67,7 +68,7 @@ * *

Instances of BuildConfiguration are canonical: * - * 

c1.equals(c2) <=> c1==c2.
+ * 
{@code c1.equals(c2) <=> c1==c2.}
*/ // TODO(janakr): If overhead of fragments class names is too high, add constructor that just takes // fragments and gets names from them. @@ -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. + * + *

See b/203470434 or #14023 for more information and planned behavior changes. + */ + private final String transitionDirectoryNameFragment; + private final ImmutableMap commandLineBuildVariables; private final int hashCode; // We can precompute the hash code as all its inputs are immutable. @@ -189,6 +198,8 @@ public BuildConfiguration( if (buildOptions.contains(PlatformOptions.class)) { platformOptions = buildOptions.get(PlatformOptions.class); } + this.transitionDirectoryNameFragment = + FunctionTransitionUtil.computeOutputDirectoryNameFragment(buildOptions); this.outputDirectories = new OutputDirectories( directories, @@ -196,7 +207,8 @@ public BuildConfiguration( platformOptions, this.fragments, mainRepositoryName, - siblingRepositoryLayout); + siblingRepositoryLayout, + transitionDirectoryNameFragment); this.mainRepositoryName = mainRepositoryName; this.siblingRepositoryLayout = siblingRepositoryLayout; @@ -403,6 +415,11 @@ public String getMnemonic() { return outputDirectories.getMnemonic(); } + @VisibleForTesting + public String getTransitionDirectoryNameFragment() { + return transitionDirectoryNameFragment; + } + @Override public String toString() { return checksum(); 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 744acfa7cb7890..5203a883b811f6 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 @@ -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", @@ -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; diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/OutputDirectories.java b/src/main/java/com/google/devtools/build/lib/analysis/config/OutputDirectories.java index ddd7fec1f56228..413251d5f63b99 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/OutputDirectories.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/OutputDirectories.java @@ -147,10 +147,12 @@ public ArtifactRoot getRoot( @Nullable PlatformOptions platformOptions, ImmutableSortedMap, 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 = @@ -207,7 +209,8 @@ private static void validateMnemonicPart(String part, String errorTemplate, Obje private static String buildMnemonic( CoreOptions options, @Nullable PlatformOptions platformOptions, - ImmutableSortedMap, Fragment> fragments) + ImmutableSortedMap, Fragment> fragments, + String transitionDirectoryNameFragment) throws InvalidMnemonicException { // See explanation at declaration for outputRoots. List nameParts = new ArrayList<>(); @@ -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("-")); 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 4e4d2a1182b228..2c95da7fe32ac0 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 @@ -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; @@ -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. @@ -165,7 +166,7 @@ static ImmutableMap buildOptionInfo(BuildOptions buildOption ImmutableSet> optionClasses = buildOptions.getNativeOptions().stream() .map(FragmentOptions::getClass) - .collect(ImmutableSet.toImmutableSet()); + .collect(toImmutableSet()); for (Class optionClass : optionClasses) { ImmutableList optionDefinitions = @@ -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; } @@ -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 changedOptions, Map 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 optionInfoMap = buildOptionInfo(toOptions); TreeMap toHash = new TreeMap<>(); for (String optionName : buildConfigOptions.affectedByStarlarkTransition) { @@ -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 hashStrs = ImmutableList.builderWithExpectedSize(toHash.size()); for (Map.Entry 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()); } /** @@ -466,6 +463,9 @@ private static void updateOutputDirectoryNameFragment( */ private static void updateAffectedByStarlarkTransition( CoreOptions buildConfigOptions, Set changedOptions) { + if (changedOptions.isEmpty()) { + return; + } Set mutableCopyToUpdate = new TreeSet<>(buildConfigOptions.affectedByStarlarkTransition); mutableCopyToUpdate.addAll(changedOptions); @@ -503,4 +503,6 @@ OptionDefinition getDefinition() { return definition; } } + + private FunctionTransitionUtil() {} } 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 39a8afe67c3f97..bb7211909ccc71 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 @@ -1181,6 +1181,81 @@ public void testTransitionOnBuildSetting_fromCommandLine() throws Exception { .isEqualTo(StarlarkInt.of(42)); } + @Test + public void testTransitionBackToStarlarkDefaultOK() throws Exception { + writeAllowlistFile(); + writeBuildSettingsBzl(); + scratch.file( + "test/starlark/rules.bzl", + "load('//myinfo:myinfo.bzl', 'MyInfo')", + "def _transition_impl(settings, attr):", + " return {", + " '//test/starlark:the-answer': attr.answer_for_dep,", + " '//test/starlark:did-transition': 1,", + "}", + "my_transition = transition(", + " implementation = _transition_impl,", + " inputs = [],", + " outputs = ['//test/starlark:the-answer', '//test/starlark:did-transition']", + ")", + "def _rule_impl(ctx):", + " return MyInfo(dep = ctx.attr.dep)", + "my_rule = rule(", + " implementation = _rule_impl,", + " attrs = {", + " 'dep': attr.label(cfg = my_transition),", + " 'answer_for_dep': attr.int(),", + " '_allowlist_function_transition': attr.label(", + " default = '//tools/allowlists/function_transition_allowlist'),", + " }", + ")"); + scratch.file( + "test/starlark/BUILD", + "load('//test/starlark:rules.bzl', 'my_rule')", + "load('//test/starlark:build_settings.bzl', 'int_flag')", + "my_rule(name = 'test', dep = ':dep1', answer_for_dep=0)", + "my_rule(name = 'dep1', dep = ':dep2', answer_for_dep=42)", + "my_rule(name = 'dep2', dep = ':dep3', answer_for_dep=0)", + "my_rule(name = 'dep3')", + "int_flag(name = 'the-answer', build_setting_default=0)", + "int_flag(name = 'did-transition', build_setting_default=0)"); + useConfiguration(ImmutableMap.of(), "--cpu=FOO"); + + ConfiguredTarget test = getConfiguredTarget("//test/starlark:test"); + + // '//test/starlark:did-transition ensures ST-hash is 'turned on' since :test has no ST-hash + // and thus will trivially have a unique getTransitionDirectoryNameFragment + + @SuppressWarnings("unchecked") + ConfiguredTarget dep1 = + Iterables.getOnlyElement( + (List) getMyInfoFromTarget(test).getValue("dep")); + + @SuppressWarnings("unchecked") + ConfiguredTarget dep2 = + Iterables.getOnlyElement( + (List) getMyInfoFromTarget(dep1).getValue("dep")); + + @SuppressWarnings("unchecked") + ConfiguredTarget dep3 = + Iterables.getOnlyElement( + (List) getMyInfoFromTarget(dep2).getValue("dep")); + + // These must be true + assertThat(getTransitionDirectoryNameFragment(dep1)) + .isNotEqualTo(getTransitionDirectoryNameFragment(dep2)); + + assertThat(getTransitionDirectoryNameFragment(dep2)) + .isNotEqualTo(getTransitionDirectoryNameFragment(dep3)); + + // TODO(blaze-configurability-team): When "affected by starlark transition" is gone, + // will be equal and thus getTransitionDirectoryNameFragment can be equal. + if (!getConfiguration(dep1).equals(getConfiguration(dep3))) { + assertThat(getTransitionDirectoryNameFragment(dep1)) + .isNotEqualTo(getTransitionDirectoryNameFragment(dep3)); + } + } + @Test public void testTransitionOnBuildSetting_onlyTransitionsAffectsDirectory() throws Exception { writeAllowlistFile(); @@ -1216,7 +1291,7 @@ public void testTransitionOnBuildSetting_onlyTransitionsAffectsDirectory() throw // Assert that transitionDirectoryNameFragment is only affected by options // set via transitions. Not by native or starlark options set via command line, // never touched by any transition. - assertThat(getCoreOptions(dep).transitionDirectoryNameFragment) + assertThat(getTransitionDirectoryNameFragment(dep)) .isEqualTo( FunctionTransitionUtil.transitionDirectoryNameFragment( ImmutableList.of("//test/starlark:the-answer=42"))); @@ -1234,6 +1309,10 @@ private Object getStarlarkOption(ConfiguredTarget target, String absName) { return getStarlarkOptions(target).get(Label.parseAbsoluteUnchecked(absName)); } + private String getTransitionDirectoryNameFragment(ConfiguredTarget target) { + return getConfiguration(target).getTransitionDirectoryNameFragment(); + } + @Test public void testOutputDirHash_multipleNativeOptionTransitions() throws Exception { writeAllowlistFile(); @@ -1287,12 +1366,12 @@ public void testOutputDirHash_multipleNativeOptionTransitions() throws Exception assertThat(affectedOptions) .containsExactly("//command_line_option:foo", "//command_line_option:bar"); - assertThat(getCoreOptions(test).transitionDirectoryNameFragment) + assertThat(getTransitionDirectoryNameFragment(test)) .isEqualTo( FunctionTransitionUtil.transitionDirectoryNameFragment( ImmutableList.of("//command_line_option:foo=foosball"))); - assertThat(getCoreOptions(dep).transitionDirectoryNameFragment) + assertThat(getTransitionDirectoryNameFragment(dep)) .isEqualTo( FunctionTransitionUtil.transitionDirectoryNameFragment( ImmutableList.of( @@ -1346,8 +1425,8 @@ public void testOutputDirHash_noop_changeToSameState() throws Exception { Iterables.getOnlyElement( (List) getMyInfoFromTarget(test).getValue("dep")); - assertThat(getCoreOptions(test).transitionDirectoryNameFragment) - .isEqualTo(getCoreOptions(dep).transitionDirectoryNameFragment); + assertThat(getTransitionDirectoryNameFragment(test)) + .isEqualTo(getTransitionDirectoryNameFragment(dep)); } @Test @@ -1390,8 +1469,8 @@ public void testOutputDirHash_noop_emptyReturns() throws Exception { Iterables.getOnlyElement( (List) getMyInfoFromTarget(test).getValue("dep")); - assertThat(getCoreOptions(test).transitionDirectoryNameFragment) - .isEqualTo(getCoreOptions(dep).transitionDirectoryNameFragment); + assertThat(getTransitionDirectoryNameFragment(test)) + .isEqualTo(getTransitionDirectoryNameFragment(dep)); } // Test that setting all starlark options back to default != null hash of top level. @@ -1452,7 +1531,7 @@ public void testOutputDirHash_multipleStarlarkOptionTransitions_backToDefaultCom (List) getMyInfoFromTarget(getConfiguredTarget("//test")).getValue("dep")); - assertThat(getCoreOptions(dep).transitionDirectoryNameFragment).isNotNull(); + assertThat(getTransitionDirectoryNameFragment(dep)).isNotEmpty(); } /** See comment above {@link FunctionTransitionUtil#updateOutputDirectoryNameFragment} */ @@ -1507,11 +1586,11 @@ public void testOutputDirHash_starlarkOption_differentBoolRepresentationsNotEqua Iterables.getOnlyElement( (List) getMyInfoFromTarget(test).getValue("dep")); - assertThat(getCoreOptions(test).transitionDirectoryNameFragment) + assertThat(getTransitionDirectoryNameFragment(test)) .isEqualTo( FunctionTransitionUtil.transitionDirectoryNameFragment( ImmutableList.of("//test:foo=1"))); - assertThat(getCoreOptions(dep).transitionDirectoryNameFragment) + assertThat(getTransitionDirectoryNameFragment(dep)) .isEqualTo( FunctionTransitionUtil.transitionDirectoryNameFragment( ImmutableList.of("//test:foo=true"))); @@ -1561,8 +1640,8 @@ public void testOutputDirHash_nativeOption_differentBoolRepresentationsEquals() Iterables.getOnlyElement( (List) getMyInfoFromTarget(test).getValue("dep")); - assertThat(getCoreOptions(test).transitionDirectoryNameFragment) - .isEqualTo(getCoreOptions(dep).transitionDirectoryNameFragment); + assertThat(getTransitionDirectoryNameFragment(test)) + .isEqualTo(getTransitionDirectoryNameFragment(dep)); } @Test @@ -1619,11 +1698,11 @@ public void testOutputDirHash_multipleStarlarkTransitions() throws Exception { getConfiguration(dep).getOptions().get(CoreOptions.class).affectedByStarlarkTransition; assertThat(affectedOptions).containsExactly("//test:bar", "//test:foo"); - assertThat(getCoreOptions(test).transitionDirectoryNameFragment) + assertThat(getTransitionDirectoryNameFragment(test)) .isEqualTo( FunctionTransitionUtil.transitionDirectoryNameFragment( ImmutableList.of("//test:foo=foosball"))); - assertThat(getCoreOptions(dep).transitionDirectoryNameFragment) + assertThat(getTransitionDirectoryNameFragment(dep)) .isEqualTo( FunctionTransitionUtil.transitionDirectoryNameFragment( ImmutableList.of("//test:bar=barsball", "//test:foo=foosball"))); @@ -1707,7 +1786,7 @@ public void testOutputDirHash_multipleMixedTransitions() throws Exception { assertThat(affectedOptionsTop).containsExactly("//command_line_option:foo"); assertThat(getConfiguration(top).getOptions().getStarlarkOptions()).isEmpty(); - assertThat(getCoreOptions(top).transitionDirectoryNameFragment) + assertThat(getTransitionDirectoryNameFragment(top)) .isEqualTo( FunctionTransitionUtil.transitionDirectoryNameFragment( ImmutableList.of("//command_line_option:foo=foosball"))); @@ -1727,7 +1806,7 @@ public void testOutputDirHash_multipleMixedTransitions() throws Exception { .containsExactly( Maps.immutableEntry(Label.parseAbsoluteUnchecked("//test:zee"), "zeesball")); - assertThat(getCoreOptions(middle).transitionDirectoryNameFragment) + assertThat(getTransitionDirectoryNameFragment(middle)) .isEqualTo( FunctionTransitionUtil.transitionDirectoryNameFragment( ImmutableList.of( @@ -1752,7 +1831,7 @@ public void testOutputDirHash_multipleMixedTransitions() throws Exception { .containsExactly( Maps.immutableEntry(Label.parseAbsoluteUnchecked("//test:zee"), "zeesball"), Maps.immutableEntry(Label.parseAbsoluteUnchecked("//test:xan"), "xansball")); - assertThat(getCoreOptions(bottom).transitionDirectoryNameFragment) + assertThat(getTransitionDirectoryNameFragment(bottom)) .isEqualTo( FunctionTransitionUtil.transitionDirectoryNameFragment( ImmutableList.of( @@ -2022,8 +2101,8 @@ private void testNoOpTransitionLeavesSameConfig_native(boolean directRead) throw ConfiguredTarget dep = Iterables.getOnlyElement( (List) getMyInfoFromTarget(test).getValue("dep")); - assertThat(getCoreOptions(test).transitionDirectoryNameFragment) - .isEqualTo(getCoreOptions(dep).transitionDirectoryNameFragment); + assertThat(getTransitionDirectoryNameFragment(test)) + .isEqualTo(getTransitionDirectoryNameFragment(dep)); } @Test @@ -2090,8 +2169,8 @@ private void testNoOpTransitionLeavesSameConfig_starlark(boolean directRead, boo ConfiguredTarget dep = Iterables.getOnlyElement( (List) getMyInfoFromTarget(test).getValue("dep")); - assertThat(getCoreOptions(test).transitionDirectoryNameFragment) - .isEqualTo(getCoreOptions(dep).transitionDirectoryNameFragment); + assertThat(getTransitionDirectoryNameFragment(test)) + .isEqualTo(getTransitionDirectoryNameFragment(dep)); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/DummyTestFragment.java b/src/test/java/com/google/devtools/build/lib/analysis/util/DummyTestFragment.java index c07afa21817329..f9d5dda1bde143 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/DummyTestFragment.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/DummyTestFragment.java @@ -22,6 +22,7 @@ import com.google.devtools.common.options.Option; import com.google.devtools.common.options.OptionDocumentationCategory; import com.google.devtools.common.options.OptionEffectTag; +import com.google.devtools.common.options.OptionMetadataTag; import java.util.List; /** @@ -58,6 +59,15 @@ public static class DummyTestOptions extends FragmentOptions { help = "A regular string-typed option") public String foo; + @Option( + name = "internal foo", + defaultValue = "", + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + effectTags = {OptionEffectTag.NO_OP}, + metadataTags = {OptionMetadataTag.INTERNAL}, + help = "A string-typed option that cannot be set on the commandline") + public String internalFoo; + @Option( name = "bar", defaultValue = "", diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionTest.java index 3c19e2048da518..1e0785f4f04655 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionTest.java @@ -20,15 +20,18 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.actions.MissingInputFileException; +import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; import com.google.devtools.build.lib.analysis.PlatformConfiguration; import com.google.devtools.build.lib.analysis.PlatformOptions; 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.FragmentClassSet; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; +import com.google.devtools.build.lib.analysis.util.DummyTestFragment; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction; import com.google.devtools.build.lib.skyframe.util.SkyframeExecutorTestUtils; +import com.google.devtools.build.lib.testutil.TestRuleClassProvider; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.skyframe.EvaluationResult; import java.util.Optional; @@ -58,6 +61,14 @@ public final class PlatformMappingFunctionTest extends BuildViewTestCase { private BuildOptions defaultBuildOptions; + @Override + protected ConfiguredRuleClassProvider createRuleClassProvider() { + ConfiguredRuleClassProvider.Builder builder = new ConfiguredRuleClassProvider.Builder(); + TestRuleClassProvider.addStandardRules(builder); + builder.addConfigurationFragment(DummyTestFragment.class); + return builder.build(); + } + @Before public void setDefaultBuildOptions() { defaultBuildOptions = @@ -218,7 +229,7 @@ public void ableToChangeInternalOption() throws Exception { "my_mapping_file", "platforms:", // Force line break " //platforms:one", // Force line break - " --transition directory name fragment=updated_output_dir"); + " --internal foo=something_new"); PlatformMappingValue platformMappingValue = executeFunction(PlatformMappingValue.Key.create(PathFragment.create("my_mapping_file"))); @@ -228,8 +239,8 @@ public void ableToChangeInternalOption() throws Exception { BuildConfigurationValue.Key mapped = platformMappingValue.map(keyForOptions(modifiedOptions)); - assertThat(mapped.getOptions().get(CoreOptions.class).transitionDirectoryNameFragment) - .isEqualTo("updated_output_dir"); + assertThat(mapped.getOptions().get(DummyTestFragment.DummyTestOptions.class).internalFoo) + .isEqualTo("something_new"); } private PlatformMappingValue executeFunction(PlatformMappingValue.Key key) throws Exception {