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..a0b31aad474ff3 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..b8b7c060a8093e 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 @@ -289,13 +289,14 @@ private static BuildOptions applyTransition( if (!optionName.startsWith(COMMAND_LINE_OPTION_PREFIX)) { // The transition changes a Starlark option. + Label optionLabel = Label.parseAbsoluteUnchecked(optionName); Object oldValue = - fromOptions.getStarlarkOptions().get(Label.parseAbsoluteUnchecked(optionName)); + 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 RuntimeException( + "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,18 @@ 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); + for (String option : changedOptions) { + mutableCopyToUpdate.add(option); + } + 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..201aecbf398edf 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,66 @@ 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 +1238,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 +1247,17 @@ 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 +1579,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 +1666,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 +1681,19 @@ 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 +1704,9 @@ 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 +1715,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