From 82d4db237118a2ae009daba7f6e92c5cb203a4ef Mon Sep 17 00:00:00 2001 From: Ulrik Falklof Date: Tue, 14 Sep 2021 14:56:21 -0700 Subject: [PATCH] Same transition dirs when only cmd line options differs Ignore user defined build settings only set on command line and not affected by any transition, when calculating transitionDirectoryNameFragment output directory. Native options on command line already have this behavior, and this commit provides the same also for user defined build settings. This allows reducing transition scalability issues, by combining transitions with command line options: a) Setting a limited number of common options via transitions, which affects large parts of the code base, but with limited number of variants. b) When needed, *also* setting a few of many specific options, each of them affecting only their parts of the code base, but with many variants. Resolves #12731 Closes #13580. PiperOrigin-RevId: 396689438 --- .../lib/analysis/config/CoreOptions.java | 8 +- .../starlark/FunctionTransitionUtil.java | 79 +++++++++-------- .../StarlarkAttrTransitionProviderTest.java | 84 ++++++++++++++++--- 3 files changed, 122 insertions(+), 49 deletions(-) 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 bb77ae65b0b188..6a13252a55a7ee 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 @@ -261,9 +261,11 @@ public String getTypeDescription() { } /** - * This internal option is a *set* of names (e.g. "cpu") of *native* options that have been - * changed by starlark transitions at any point in the build at the time of accessing. This is - * used to regenerate {@code transitionDirectoryNameFragment} after each starlark transition. + * 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", 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 72f3acce37081f..4e4d2a1182b228 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 @@ -19,6 +19,7 @@ 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; @@ -289,13 +290,13 @@ private static BuildOptions applyTransition( if (!optionName.startsWith(COMMAND_LINE_OPTION_PREFIX)) { // The transition changes a Starlark option. - Object oldValue = - fromOptions.getStarlarkOptions().get(Label.parseAbsoluteUnchecked(optionName)); + Label optionLabel = Label.parseAbsoluteUnchecked(optionName); + Object oldValue = fromOptions.getStarlarkOptions().get(optionLabel); if ((oldValue == null && optionValue != null) || (oldValue != null && optionValue == null) || (oldValue != null && !oldValue.equals(optionValue))) { - changedStarlarkOptions.put(Label.parseAbsoluteUnchecked(optionName), optionValue); - convertedNewValues.add(optionName); + changedStarlarkOptions.put(optionLabel, optionValue); + convertedNewValues.add(optionLabel.toString()); } } else { // The transition changes a native option. @@ -398,9 +399,10 @@ private static BuildOptions applyTransition( } /** - * Compute the output directory name fragment corresponding to the new BuildOptions based on (1) - * the names and values of all native options previously transitioned anywhere in the build by - * starlark transitions, (2) names and values of all entries in the starlark options map. + * 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 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. @@ -421,38 +423,34 @@ private static void updateOutputDirectoryNameFragment( } CoreOptions buildConfigOptions = toOptions.get(CoreOptions.class); - Set updatedAffectedByStarlarkTransition = - new TreeSet<>(buildConfigOptions.affectedByStarlarkTransition); - // Add newly changed native options to overall list of changed native options - for (String option : changedOptions) { - if (option.startsWith(COMMAND_LINE_OPTION_PREFIX)) { - updatedAffectedByStarlarkTransition.add( - option.substring(COMMAND_LINE_OPTION_PREFIX.length())); - } - } - buildConfigOptions.affectedByStarlarkTransition = - ImmutableList.sortedCopyOf(updatedAffectedByStarlarkTransition); - // hash all relevant native option values; + updateAffectedByStarlarkTransition(buildConfigOptions, changedOptions); + TreeMap toHash = new TreeMap<>(); - for (String nativeOption : updatedAffectedByStarlarkTransition) { - Object value; - try { - value = - optionInfoMap - .get(nativeOption) - .getDefinition() - .getField() - .get(toOptions.get(optionInfoMap.get(nativeOption).getOptionClass())); - } catch (IllegalAccessException e) { - throw new RuntimeException( - "IllegalAccess for option " + nativeOption + ": " + e.getMessage()); + 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)); + if (value != null) { + toHash.put(optionName, value); + } } - toHash.put(nativeOption, value); } - // hash all starlark options in map. - toOptions.getStarlarkOptions().forEach((opt, value) -> toHash.put(opt.toString(), value)); ImmutableList.Builder hashStrs = ImmutableList.builderWithExpectedSize(toHash.size()); for (Map.Entry singleOptionAndValue : toHash.entrySet()) { String toAdd = singleOptionAndValue.getKey() + "=" + singleOptionAndValue.getValue(); @@ -462,6 +460,19 @@ private static void updateOutputDirectoryNameFragment( 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) { + Set mutableCopyToUpdate = + new TreeSet<>(buildConfigOptions.affectedByStarlarkTransition); + mutableCopyToUpdate.addAll(changedOptions); + buildConfigOptions.affectedByStarlarkTransition = + ImmutableList.sortedCopyOf(mutableCopyToUpdate); + } + public static String transitionDirectoryNameFragment(Iterable opts) { Fingerprint fp = new Fingerprint(); for (String opt : opts) { 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 268e03bb839251..6e9106eb22e1bd 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 @@ -1137,10 +1137,59 @@ public void testTransitionOnBuildSetting_fromCommandLine() throws Exception { .isEqualTo(StarlarkInt.of(42)); } + @Test + public void testTransitionOnBuildSetting_onlyTransitionsAffectsDirectory() throws Exception { + writeAllowlistFile(); + writeBuildSettingsBzl(); + writeRulesWithAttrTransitionBzl(); + 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 = ':dep')", + "my_rule(name = 'dep')", + "int_flag(name = 'the-answer', build_setting_default = 0)", + "int_flag(name = 'cmd-line-option', build_setting_default = 0)"); + + useConfiguration(ImmutableMap.of("//test/starlark:cmd-line-option", 100), "--cpu=FOO"); + + ConfiguredTarget test = getConfiguredTarget("//test/starlark:test"); + + @SuppressWarnings("unchecked") + ConfiguredTarget dep = + Iterables.getOnlyElement( + (List) getMyInfoFromTarget(test).getValue("dep")); + + // Assert starlark option set via transition. + assertThat(getStarlarkOption(dep, "//test/starlark:the-answer")).isEqualTo(StarlarkInt.of(42)); + + // Assert starlark option set via command line. + assertThat(getStarlarkOption(dep, "//test/starlark:cmd-line-option")).isEqualTo(100); + + // Assert native option set via command line. + assertThat(getCoreOptions(dep).cpu).isEqualTo("FOO"); + + // 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) + .isEqualTo( + FunctionTransitionUtil.transitionDirectoryNameFragment( + ImmutableList.of("//test/starlark:the-answer=42"))); + } + private CoreOptions getCoreOptions(ConfiguredTarget target) { return getConfiguration(target).getOptions().get(CoreOptions.class); } + private ImmutableMap getStarlarkOptions(ConfiguredTarget target) { + return getConfiguration(target).getOptions().getStarlarkOptions(); + } + + private Object getStarlarkOption(ConfiguredTarget target, String absName) { + return getStarlarkOptions(target).get(Label.parseAbsoluteUnchecked(absName)); + } + @Test public void testOutputDirHash_multipleNativeOptionTransitions() throws Exception { writeAllowlistFile(); @@ -1182,7 +1231,7 @@ public void testOutputDirHash_multipleNativeOptionTransitions() throws Exception List affectedOptions = getCoreOptions(test).affectedByStarlarkTransition; - assertThat(affectedOptions).containsExactly("foo"); + assertThat(affectedOptions).containsExactly("//command_line_option:foo"); @SuppressWarnings("unchecked") ConfiguredTarget dep = @@ -1191,17 +1240,19 @@ public void testOutputDirHash_multipleNativeOptionTransitions() throws Exception affectedOptions = getCoreOptions(dep).affectedByStarlarkTransition; - assertThat(affectedOptions).containsExactly("foo", "bar"); + assertThat(affectedOptions) + .containsExactly("//command_line_option:foo", "//command_line_option:bar"); assertThat(getCoreOptions(test).transitionDirectoryNameFragment) .isEqualTo( FunctionTransitionUtil.transitionDirectoryNameFragment( - ImmutableList.of("foo=foosball"))); + ImmutableList.of("//command_line_option:foo=foosball"))); assertThat(getCoreOptions(dep).transitionDirectoryNameFragment) .isEqualTo( FunctionTransitionUtil.transitionDirectoryNameFragment( - ImmutableList.of("bar=barsball", "foo=foosball"))); + ImmutableList.of( + "//command_line_option:bar=barsball", "//command_line_option:foo=foosball"))); } // Test that a no-op starlark transition to an already starlark transitioned configuration @@ -1523,8 +1574,7 @@ public void testOutputDirHash_multipleStarlarkTransitions() throws Exception { List affectedOptions = getConfiguration(dep).getOptions().get(CoreOptions.class).affectedByStarlarkTransition; - // Assert that affectedOptions is empty but final fragment is still different. - assertThat(affectedOptions).isEmpty(); + assertThat(affectedOptions).containsExactly("//test:bar", "//test:foo"); assertThat(getCoreOptions(test).transitionDirectoryNameFragment) .isEqualTo( FunctionTransitionUtil.transitionDirectoryNameFragment( @@ -1611,12 +1661,12 @@ public void testOutputDirHash_multipleMixedTransitions() throws Exception { List affectedOptionsTop = getConfiguration(top).getOptions().get(CoreOptions.class).affectedByStarlarkTransition; - assertThat(affectedOptionsTop).containsExactly("foo"); + assertThat(affectedOptionsTop).containsExactly("//command_line_option:foo"); assertThat(getConfiguration(top).getOptions().getStarlarkOptions()).isEmpty(); assertThat(getCoreOptions(top).transitionDirectoryNameFragment) .isEqualTo( FunctionTransitionUtil.transitionDirectoryNameFragment( - ImmutableList.of("foo=foosball"))); + ImmutableList.of("//command_line_option:foo=foosball"))); // test:middle (foo_transition, zee_transition, bar_transition) @SuppressWarnings("unchecked") @@ -1626,14 +1676,20 @@ public void testOutputDirHash_multipleMixedTransitions() throws Exception { List affectedOptionsMiddle = getConfiguration(middle).getOptions().get(CoreOptions.class).affectedByStarlarkTransition; - assertThat(affectedOptionsMiddle).containsExactly("foo", "bar"); + 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")); + assertThat(getCoreOptions(middle).transitionDirectoryNameFragment) .isEqualTo( FunctionTransitionUtil.transitionDirectoryNameFragment( - ImmutableList.of("//test:zee=zeesball", "bar=barsball", "foo=foosball"))); + 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") @@ -1644,7 +1700,10 @@ public void testOutputDirHash_multipleMixedTransitions() throws Exception { List affectedOptionsBottom = getConfiguration(bottom).getOptions().get(CoreOptions.class).affectedByStarlarkTransition; - assertThat(affectedOptionsBottom).containsExactly("foo", "bar"); + 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"), @@ -1653,7 +1712,8 @@ public void testOutputDirHash_multipleMixedTransitions() throws Exception { .isEqualTo( FunctionTransitionUtil.transitionDirectoryNameFragment( ImmutableList.of( - "//test:xan=xansball", "//test:zee=zeesball", "bar=barsball", "foo=foosball"))); + "//command_line_option:bar=barsball", "//command_line_option:foo=foosball", + "//test:xan=xansball", "//test:zee=zeesball"))); } @Test