Skip to content

Commit 711c44e

Browse files
twiggcopybara-github
twigg
authored and
copybara-github
committed
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
1 parent 2255ce4 commit 711c44e

8 files changed

Lines changed: 95 additions & 69 deletions

File tree

‎src/main/java/com/google/devtools/build/lib/analysis/BUILD‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1526,6 +1526,7 @@ java_library(
15261526
":config/run_under",
15271527
":config/transitive_option_details",
15281528
":platform_options",
1529+
":starlark/function_transition_util",
15291530
"//src/main/java/com/google/devtools/build/lib/actions",
15301531
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
15311532
"//src/main/java/com/google/devtools/build/lib/buildeventstream",

‎src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import com.google.devtools.build.lib.analysis.BlazeDirectories;
2929
import com.google.devtools.build.lib.analysis.PlatformOptions;
3030
import com.google.devtools.build.lib.analysis.config.OutputDirectories.InvalidMnemonicException;
31+
import com.google.devtools.build.lib.analysis.starlark.FunctionTransitionUtil;
3132
import com.google.devtools.build.lib.buildeventstream.BuildEventIdUtil;
3233
import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
3334
import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildEventId;
@@ -68,7 +69,7 @@
6869
*
6970
* <p>Instances of {@code BuildConfigurationValue} are canonical:
7071
*
71-
* <pre>c1.equals(c2) <=> c1==c2.</pre>
72+
* <pre>{@code c1.equals(c2) <=> c1==c2.}</pre>
7273
*/
7374
@AutoCodec
7475
public class BuildConfigurationValue implements BuildConfigurationApi, SkyValue {
@@ -106,6 +107,14 @@ public interface ActionEnvironmentProvider {
106107
private final BuildOptions buildOptions;
107108
private final CoreOptions options;
108109

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

111120
/** Data for introspecting the options used by this configuration. */
@@ -160,14 +169,17 @@ public BuildConfigurationValue(
160169
if (buildOptions.contains(PlatformOptions.class)) {
161170
platformOptions = buildOptions.get(PlatformOptions.class);
162171
}
172+
this.transitionDirectoryNameFragment =
173+
FunctionTransitionUtil.computeOutputDirectoryNameFragment(buildOptions);
163174
this.outputDirectories =
164175
new OutputDirectories(
165176
directories,
166177
options,
167178
platformOptions,
168179
this.fragments,
169180
mainRepositoryName,
170-
siblingRepositoryLayout);
181+
siblingRepositoryLayout,
182+
transitionDirectoryNameFragment);
171183
this.mainRepositoryName = mainRepositoryName;
172184
this.siblingRepositoryLayout = siblingRepositoryLayout;
173185

@@ -376,6 +388,11 @@ public String getMnemonic() {
376388
return outputDirectories.getMnemonic();
377389
}
378390

391+
@VisibleForTesting
392+
public String getTransitionDirectoryNameFragment() {
393+
return transitionDirectoryNameFragment;
394+
}
395+
379396
@Override
380397
public String toString() {
381398
return checksum();

‎src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java‎

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -240,22 +240,6 @@ public class CoreOptions extends FragmentOptions implements Cloneable {
240240
+ "'fastbuild', 'dbg', 'opt'.")
241241
public CompilationMode hostCompilationMode;
242242

243-
/**
244-
* This option is used by starlark transitions to add a distinguishing element to the output
245-
* directory name, in order to avoid name clashing.
246-
*/
247-
@Option(
248-
name = "transition directory name fragment",
249-
defaultValue = "null",
250-
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
251-
effectTags = {
252-
OptionEffectTag.LOSES_INCREMENTAL_STATE,
253-
OptionEffectTag.AFFECTS_OUTPUTS,
254-
OptionEffectTag.LOADING_AND_ANALYSIS
255-
},
256-
metadataTags = {OptionMetadataTag.INTERNAL})
257-
public String transitionDirectoryNameFragment;
258-
259243
@Option(
260244
name = "experimental_enable_aspect_hints",
261245
defaultValue = "false",
@@ -839,7 +823,6 @@ public IncludeConfigFragmentsEnumConverter() {
839823
public FragmentOptions getHost() {
840824
CoreOptions host = (CoreOptions) getDefault();
841825

842-
host.transitionDirectoryNameFragment = transitionDirectoryNameFragment;
843826
host.affectedByStarlarkTransition = affectedByStarlarkTransition;
844827
host.compilationMode = hostCompilationMode;
845828
host.isHost = true;

‎src/main/java/com/google/devtools/build/lib/analysis/config/OutputDirectories.java‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -136,10 +136,12 @@ public ArtifactRoot getRoot(
136136
@Nullable PlatformOptions platformOptions,
137137
ImmutableSortedMap<Class<? extends Fragment>, Fragment> fragments,
138138
RepositoryName mainRepositoryName,
139-
boolean siblingRepositoryLayout)
139+
boolean siblingRepositoryLayout,
140+
String transitionDirectoryNameFragment)
140141
throws InvalidMnemonicException {
141142
this.directories = directories;
142-
this.mnemonic = buildMnemonic(options, platformOptions, fragments);
143+
this.mnemonic =
144+
buildMnemonic(options, platformOptions, fragments, transitionDirectoryNameFragment);
143145
this.outputDirName = options.isHost ? "host" : mnemonic;
144146

145147
this.outputDirectory =
@@ -196,7 +198,8 @@ private static void validateMnemonicPart(String part, String errorTemplate, Obje
196198
private static String buildMnemonic(
197199
CoreOptions options,
198200
@Nullable PlatformOptions platformOptions,
199-
ImmutableSortedMap<Class<? extends Fragment>, Fragment> fragments)
201+
ImmutableSortedMap<Class<? extends Fragment>, Fragment> fragments,
202+
String transitionDirectoryNameFragment)
200203
throws InvalidMnemonicException {
201204
// See explanation at declaration for outputRoots.
202205
List<String> nameParts = new ArrayList<>();
@@ -219,9 +222,7 @@ private static String buildMnemonic(
219222

220223
// Add the transition suffix.
221224
addMnemonicPart(
222-
nameParts,
223-
options.transitionDirectoryNameFragment,
224-
"Transition directory name fragment '%s'");
225+
nameParts, transitionDirectoryNameFragment, "Transition directory name fragment '%s'");
225226

226227
// Join all the parts.
227228
String mnemonic = nameParts.stream().filter(not(Strings::isNullOrEmpty)).collect(joining("-"));

‎src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java‎

Lines changed: 19 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414

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

17+
import static com.google.common.collect.ImmutableSet.toImmutableSet;
1718
import static com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition.COMMAND_LINE_OPTION_PREFIX;
1819
import static com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition.PATCH_TRANSITION_KEY;
1920
import static java.util.stream.Collectors.joining;
@@ -57,7 +58,7 @@
5758
* Utility class for common work done across {@link StarlarkAttributeTransitionProvider} and {@link
5859
* StarlarkRuleTransitionProvider}.
5960
*/
60-
public class FunctionTransitionUtil {
61+
public final class FunctionTransitionUtil {
6162

6263
// The length of the hash of the config tacked onto the end of the output path.
6364
// Limited for ergonomics and MAX_PATH reasons.
@@ -165,7 +166,7 @@ private static ImmutableMap<String, OptionInfo> buildOptionInfo(BuildOptions bui
165166
ImmutableSet<Class<? extends FragmentOptions>> optionClasses =
166167
buildOptions.getNativeOptions().stream()
167168
.map(FragmentOptions::getClass)
168-
.collect(ImmutableSet.toImmutableSet());
169+
.collect(toImmutableSet());
169170

170171
for (Class<? extends FragmentOptions> optionClass : optionClasses) {
171172
ImmutableList<OptionDefinition> optionDefinitions =
@@ -394,7 +395,8 @@ private static BuildOptions applyTransition(
394395
convertedNewValues.add("//command_line_option:evaluating for analysis test");
395396
toOptions.get(CoreOptions.class).evaluatingForAnalysisTest = true;
396397
}
397-
updateOutputDirectoryNameFragment(convertedNewValues, optionInfoMap, toOptions);
398+
399+
updateAffectedByStarlarkTransition(toOptions.get(CoreOptions.class), convertedNewValues);
398400
return toOptions;
399401
}
400402

@@ -404,27 +406,20 @@ private static BuildOptions applyTransition(
404406
* the build by Starlark transitions. Options only set on command line are not affecting the
405407
* computation.
406408
*
407-
* @param changedOptions the names of all options changed by this transition in label form e.g.
408-
* "//command_line_option:cpu" for native options and "//myapp:foo" for starlark options.
409-
* @param optionInfoMap a map of all native options (name -> OptionInfo) present in {@code
410-
* toOptions}.
411-
* @param toOptions the newly transitioned {@link BuildOptions} for which we need to updated
412-
* {@code transitionDirectoryNameFragment} and {@code affectedByStarlarkTransition}.
409+
* @param toOptions the {@link BuildOptions} to use to calculate which we need to compute {@code
410+
* transitionDirectoryNameFragment}.
413411
*/
414412
// TODO(bazel-team): This hashes different forms of equivalent values differently though they
415413
// should be the same configuration. Starlark transitions are flexible about the values they
416414
// take (e.g. bool-typed options can take 0/1, True/False, "0"/"1", or "True"/"False") which
417415
// makes it so that two configurations that are the same in value may hash differently.
418-
private static void updateOutputDirectoryNameFragment(
419-
Set<String> changedOptions, Map<String, OptionInfo> optionInfoMap, BuildOptions toOptions) {
420-
// Return without doing anything if this transition hasn't changed any option values.
421-
if (changedOptions.isEmpty()) {
422-
return;
423-
}
424-
416+
public static String computeOutputDirectoryNameFragment(BuildOptions toOptions) {
425417
CoreOptions buildConfigOptions = toOptions.get(CoreOptions.class);
426-
427-
updateAffectedByStarlarkTransition(buildConfigOptions, changedOptions);
418+
if (buildConfigOptions.affectedByStarlarkTransition.isEmpty()) {
419+
return "";
420+
}
421+
// TODO(blaze-configurability-team): A mild performance optimization would have this be global.
422+
Map<String, OptionInfo> optionInfoMap = buildOptionInfo(toOptions);
428423

429424
TreeMap<String, Object> toHash = new TreeMap<>();
430425
for (String optionName : buildConfigOptions.affectedByStarlarkTransition) {
@@ -456,8 +451,7 @@ private static void updateOutputDirectoryNameFragment(
456451
String toAdd = singleOptionAndValue.getKey() + "=" + singleOptionAndValue.getValue();
457452
hashStrs.add(toAdd);
458453
}
459-
buildConfigOptions.transitionDirectoryNameFragment =
460-
transitionDirectoryNameFragment(hashStrs.build());
454+
return transitionDirectoryNameFragment(hashStrs.build());
461455
}
462456

463457
/**
@@ -466,6 +460,9 @@ private static void updateOutputDirectoryNameFragment(
466460
*/
467461
private static void updateAffectedByStarlarkTransition(
468462
CoreOptions buildConfigOptions, Set<String> changedOptions) {
463+
if (changedOptions.isEmpty()) {
464+
return;
465+
}
469466
Set<String> mutableCopyToUpdate =
470467
new TreeSet<>(buildConfigOptions.affectedByStarlarkTransition);
471468
mutableCopyToUpdate.addAll(changedOptions);
@@ -503,4 +500,6 @@ OptionDefinition getDefinition() {
503500
return definition;
504501
}
505502
}
503+
504+
private FunctionTransitionUtil() {}
506505
}

‎src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java‎

Lines changed: 25 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1216,7 +1216,7 @@ public void testTransitionOnBuildSetting_onlyTransitionsAffectsDirectory() throw
12161216
// Assert that transitionDirectoryNameFragment is only affected by options
12171217
// set via transitions. Not by native or starlark options set via command line,
12181218
// never touched by any transition.
1219-
assertThat(getCoreOptions(dep).transitionDirectoryNameFragment)
1219+
assertThat(getTransitionDirectoryNameFragment(dep))
12201220
.isEqualTo(
12211221
FunctionTransitionUtil.transitionDirectoryNameFragment(
12221222
ImmutableList.of("//test/starlark:the-answer=42")));
@@ -1234,6 +1234,10 @@ private Object getStarlarkOption(ConfiguredTarget target, String absName) {
12341234
return getStarlarkOptions(target).get(Label.parseAbsoluteUnchecked(absName));
12351235
}
12361236

1237+
private String getTransitionDirectoryNameFragment(ConfiguredTarget target) {
1238+
return getConfiguration(target).getTransitionDirectoryNameFragment();
1239+
}
1240+
12371241
@Test
12381242
public void testOutputDirHash_multipleNativeOptionTransitions() throws Exception {
12391243
writeAllowlistFile();
@@ -1287,12 +1291,12 @@ public void testOutputDirHash_multipleNativeOptionTransitions() throws Exception
12871291
assertThat(affectedOptions)
12881292
.containsExactly("//command_line_option:foo", "//command_line_option:bar");
12891293

1290-
assertThat(getCoreOptions(test).transitionDirectoryNameFragment)
1294+
assertThat(getTransitionDirectoryNameFragment(test))
12911295
.isEqualTo(
12921296
FunctionTransitionUtil.transitionDirectoryNameFragment(
12931297
ImmutableList.of("//command_line_option:foo=foosball")));
12941298

1295-
assertThat(getCoreOptions(dep).transitionDirectoryNameFragment)
1299+
assertThat(getTransitionDirectoryNameFragment(dep))
12961300
.isEqualTo(
12971301
FunctionTransitionUtil.transitionDirectoryNameFragment(
12981302
ImmutableList.of(
@@ -1346,8 +1350,8 @@ public void testOutputDirHash_noop_changeToSameState() throws Exception {
13461350
Iterables.getOnlyElement(
13471351
(List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep"));
13481352

1349-
assertThat(getCoreOptions(test).transitionDirectoryNameFragment)
1350-
.isEqualTo(getCoreOptions(dep).transitionDirectoryNameFragment);
1353+
assertThat(getTransitionDirectoryNameFragment(test))
1354+
.isEqualTo(getTransitionDirectoryNameFragment(dep));
13511355
}
13521356

13531357
@Test
@@ -1390,8 +1394,8 @@ public void testOutputDirHash_noop_emptyReturns() throws Exception {
13901394
Iterables.getOnlyElement(
13911395
(List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep"));
13921396

1393-
assertThat(getCoreOptions(test).transitionDirectoryNameFragment)
1394-
.isEqualTo(getCoreOptions(dep).transitionDirectoryNameFragment);
1397+
assertThat(getTransitionDirectoryNameFragment(test))
1398+
.isEqualTo(getTransitionDirectoryNameFragment(dep));
13951399
}
13961400

13971401
// Test that setting all starlark options back to default != null hash of top level.
@@ -1452,7 +1456,7 @@ public void testOutputDirHash_multipleStarlarkOptionTransitions_backToDefaultCom
14521456
(List<ConfiguredTarget>)
14531457
getMyInfoFromTarget(getConfiguredTarget("//test")).getValue("dep"));
14541458

1455-
assertThat(getCoreOptions(dep).transitionDirectoryNameFragment).isNotNull();
1459+
assertThat(getTransitionDirectoryNameFragment(dep)).isNotEmpty();
14561460
}
14571461

14581462
/** See comment above {@link FunctionTransitionUtil#updateOutputDirectoryNameFragment} */
@@ -1507,11 +1511,11 @@ public void testOutputDirHash_starlarkOption_differentBoolRepresentationsNotEqua
15071511
Iterables.getOnlyElement(
15081512
(List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep"));
15091513

1510-
assertThat(getCoreOptions(test).transitionDirectoryNameFragment)
1514+
assertThat(getTransitionDirectoryNameFragment(test))
15111515
.isEqualTo(
15121516
FunctionTransitionUtil.transitionDirectoryNameFragment(
15131517
ImmutableList.of("//test:foo=1")));
1514-
assertThat(getCoreOptions(dep).transitionDirectoryNameFragment)
1518+
assertThat(getTransitionDirectoryNameFragment(dep))
15151519
.isEqualTo(
15161520
FunctionTransitionUtil.transitionDirectoryNameFragment(
15171521
ImmutableList.of("//test:foo=true")));
@@ -1561,8 +1565,8 @@ public void testOutputDirHash_nativeOption_differentBoolRepresentationsEquals()
15611565
Iterables.getOnlyElement(
15621566
(List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep"));
15631567

1564-
assertThat(getCoreOptions(test).transitionDirectoryNameFragment)
1565-
.isEqualTo(getCoreOptions(dep).transitionDirectoryNameFragment);
1568+
assertThat(getTransitionDirectoryNameFragment(test))
1569+
.isEqualTo(getTransitionDirectoryNameFragment(dep));
15661570
}
15671571

15681572
@Test
@@ -1619,11 +1623,11 @@ public void testOutputDirHash_multipleStarlarkTransitions() throws Exception {
16191623
getConfiguration(dep).getOptions().get(CoreOptions.class).affectedByStarlarkTransition;
16201624

16211625
assertThat(affectedOptions).containsExactly("//test:bar", "//test:foo");
1622-
assertThat(getCoreOptions(test).transitionDirectoryNameFragment)
1626+
assertThat(getTransitionDirectoryNameFragment(test))
16231627
.isEqualTo(
16241628
FunctionTransitionUtil.transitionDirectoryNameFragment(
16251629
ImmutableList.of("//test:foo=foosball")));
1626-
assertThat(getCoreOptions(dep).transitionDirectoryNameFragment)
1630+
assertThat(getTransitionDirectoryNameFragment(dep))
16271631
.isEqualTo(
16281632
FunctionTransitionUtil.transitionDirectoryNameFragment(
16291633
ImmutableList.of("//test:bar=barsball", "//test:foo=foosball")));
@@ -1707,7 +1711,7 @@ public void testOutputDirHash_multipleMixedTransitions() throws Exception {
17071711

17081712
assertThat(affectedOptionsTop).containsExactly("//command_line_option:foo");
17091713
assertThat(getConfiguration(top).getOptions().getStarlarkOptions()).isEmpty();
1710-
assertThat(getCoreOptions(top).transitionDirectoryNameFragment)
1714+
assertThat(getTransitionDirectoryNameFragment(top))
17111715
.isEqualTo(
17121716
FunctionTransitionUtil.transitionDirectoryNameFragment(
17131717
ImmutableList.of("//command_line_option:foo=foosball")));
@@ -1727,7 +1731,7 @@ public void testOutputDirHash_multipleMixedTransitions() throws Exception {
17271731
.containsExactly(
17281732
Maps.immutableEntry(Label.parseAbsoluteUnchecked("//test:zee"), "zeesball"));
17291733

1730-
assertThat(getCoreOptions(middle).transitionDirectoryNameFragment)
1734+
assertThat(getTransitionDirectoryNameFragment(middle))
17311735
.isEqualTo(
17321736
FunctionTransitionUtil.transitionDirectoryNameFragment(
17331737
ImmutableList.of(
@@ -1752,7 +1756,7 @@ public void testOutputDirHash_multipleMixedTransitions() throws Exception {
17521756
.containsExactly(
17531757
Maps.immutableEntry(Label.parseAbsoluteUnchecked("//test:zee"), "zeesball"),
17541758
Maps.immutableEntry(Label.parseAbsoluteUnchecked("//test:xan"), "xansball"));
1755-
assertThat(getCoreOptions(bottom).transitionDirectoryNameFragment)
1759+
assertThat(getTransitionDirectoryNameFragment(bottom))
17561760
.isEqualTo(
17571761
FunctionTransitionUtil.transitionDirectoryNameFragment(
17581762
ImmutableList.of(
@@ -2023,8 +2027,8 @@ private void testNoOpTransitionLeavesSameConfig_native(boolean directRead) throw
20232027
ConfiguredTarget dep =
20242028
Iterables.getOnlyElement(
20252029
(List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep"));
2026-
assertThat(getCoreOptions(test).transitionDirectoryNameFragment)
2027-
.isEqualTo(getCoreOptions(dep).transitionDirectoryNameFragment);
2030+
assertThat(getTransitionDirectoryNameFragment(test))
2031+
.isEqualTo(getTransitionDirectoryNameFragment(dep));
20282032
}
20292033

20302034
@Test
@@ -2091,8 +2095,8 @@ private void testNoOpTransitionLeavesSameConfig_starlark(boolean directRead, boo
20912095
ConfiguredTarget dep =
20922096
Iterables.getOnlyElement(
20932097
(List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep"));
2094-
assertThat(getCoreOptions(test).transitionDirectoryNameFragment)
2095-
.isEqualTo(getCoreOptions(dep).transitionDirectoryNameFragment);
2098+
assertThat(getTransitionDirectoryNameFragment(test))
2099+
.isEqualTo(getTransitionDirectoryNameFragment(dep));
20962100
}
20972101

20982102
@Test

0 commit comments

Comments
 (0)