Shorten the hash suffix in the output path caused by a config transition.
We make it 48-bit, which should be sufficient to avoid hash collisions for the tiny handful of output paths we generate. In return, we save 52 characters in the output path. This is important for Windows with its tiny MAX_PATH, but should lead to more ergonomic output paths in general.
RELNOTES:
PiperOrigin-RevId: 318072937
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 b99c527..6c6d2fb 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
@@ -55,6 +55,11 @@
public class FunctionTransitionUtil {
public static final String COMMAND_LINE_OPTION_PREFIX = "//command_line_option:";
+
+ // The length of the hash of the config tacked onto the end of the output path.
+ // Limited for ergonomics and MAX_PATH reasons.
+ private static final int HASH_LENGTH = 12;
+
/**
* Figure out what build settings the given transition changes and apply those changes to the
* incoming {@link BuildOptions}. For native options, this involves a preprocess step of
@@ -384,14 +389,25 @@
// hash all starlark options in map.
toOptions.getStarlarkOptions().forEach((opt, value) -> toHash.put(opt.toString(), value));
-
- Fingerprint fp = new Fingerprint();
+ ImmutableList.Builder<String> hashStrs = ImmutableList.builderWithExpectedSize(toHash.size());
for (Map.Entry<String, Object> singleOptionAndValue : toHash.entrySet()) {
String toAdd = singleOptionAndValue.getKey() + "=" + singleOptionAndValue.getValue();
- fp.addString(toAdd);
+ hashStrs.add(toAdd);
}
- // Make this hash somewhat recognizable
- buildConfigOptions.transitionDirectoryNameFragment = "ST-" + fp.hexDigestAndReset();
+ buildConfigOptions.transitionDirectoryNameFragment =
+ transitionDirectoryNameFragment(hashStrs.build());
+ }
+
+ public static String transitionDirectoryNameFragment(Iterable<String> opts) {
+ Fingerprint fp = new Fingerprint();
+ for (String opt : opts) {
+ fp.addString(opt);
+ }
+ // Shorten the hash to 48 bits. This should provide sufficient collision avoidance
+ // (that is, we don't expect anyone to experience a collision ever).
+ // Shortening the hash is important for Windows paths that tend to be short.
+ String suffix = fp.hexDigestAndReset().substring(0, HASH_LENGTH);
+ return "ST-" + suffix;
}
/** Stores option info useful to a FunctionSplitTransition. */
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/BUILD
index b2c10a4..7ea210c 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/analysis/BUILD
@@ -93,6 +93,7 @@
"//src/main/java/com/google/devtools/build/lib/analysis:resolved_toolchain_context",
"//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment",
"//src/main/java/com/google/devtools/build/lib/analysis:server_directories",
+ "//src/main/java/com/google/devtools/build/lib/analysis:starlark/function_transition_util",
"//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration",
"//src/main/java/com/google/devtools/build/lib/analysis:test/test_trimming_transition_factory",
"//src/main/java/com/google/devtools/build/lib/analysis:toolchain_collection",
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 012886b..15e0379 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
@@ -26,6 +26,7 @@
import com.google.devtools.build.lib.analysis.StarlarkRuleTransitionProviderTest.DummyTestLoader;
import com.google.devtools.build.lib.analysis.config.CoreOptions;
import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition;
+import com.google.devtools.build.lib.analysis.starlark.FunctionTransitionUtil;
import com.google.devtools.build.lib.analysis.test.TestConfiguration.TestOptions;
import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
import com.google.devtools.build.lib.cmdline.Label;
@@ -40,7 +41,6 @@
import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
import com.google.devtools.build.lib.syntax.Starlark;
import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
-import com.google.devtools.build.lib.util.Fingerprint;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
@@ -1057,15 +1057,14 @@
assertThat(affectedOptions).containsExactly("foo", "bar");
assertThat(getCoreOptions(test).transitionDirectoryNameFragment)
- .isEqualTo("ST-" + new Fingerprint().addString("foo=foosball").hexDigestAndReset());
+ .isEqualTo(
+ FunctionTransitionUtil.transitionDirectoryNameFragment(
+ ImmutableList.of("foo=foosball")));
assertThat(getCoreOptions(dep).transitionDirectoryNameFragment)
.isEqualTo(
- "ST-"
- + new Fingerprint()
- .addString("bar=barsball")
- .addString("foo=foosball")
- .hexDigestAndReset());
+ FunctionTransitionUtil.transitionDirectoryNameFragment(
+ ImmutableList.of("bar=barsball", "foo=foosball")));
}
// Test that a no-op starlark transition to an already starlark transitioned configuration
@@ -1383,9 +1382,13 @@
(List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep"));
assertThat(getCoreOptions(test).transitionDirectoryNameFragment)
- .isEqualTo("ST-" + new Fingerprint().addString("//test:foo=1").hexDigestAndReset());
+ .isEqualTo(
+ FunctionTransitionUtil.transitionDirectoryNameFragment(
+ ImmutableList.of("//test:foo=1")));
assertThat(getCoreOptions(dep).transitionDirectoryNameFragment)
- .isEqualTo("ST-" + new Fingerprint().addString("//test:foo=true").hexDigestAndReset());
+ .isEqualTo(
+ FunctionTransitionUtil.transitionDirectoryNameFragment(
+ ImmutableList.of("//test:foo=true")));
}
@Test
@@ -1492,14 +1495,13 @@
// Assert that affectedOptions is empty but final fragment is still different.
assertThat(affectedOptions).isEmpty();
assertThat(getCoreOptions(test).transitionDirectoryNameFragment)
- .isEqualTo("ST-" + new Fingerprint().addString("//test:foo=foosball").hexDigestAndReset());
+ .isEqualTo(
+ FunctionTransitionUtil.transitionDirectoryNameFragment(
+ ImmutableList.of("//test:foo=foosball")));
assertThat(getCoreOptions(dep).transitionDirectoryNameFragment)
.isEqualTo(
- "ST-"
- + new Fingerprint()
- .addString("//test:bar=barsball")
- .addString("//test:foo=foosball")
- .hexDigestAndReset());
+ FunctionTransitionUtil.transitionDirectoryNameFragment(
+ ImmutableList.of("//test:bar=barsball", "//test:foo=foosball")));
}
// This test is massive but mostly exists to ensure that all the parts are working together
@@ -1581,7 +1583,9 @@
assertThat(affectedOptionsTop).containsExactly("foo");
assertThat(getConfiguration(top).getOptions().getStarlarkOptions()).isEmpty();
assertThat(getCoreOptions(top).transitionDirectoryNameFragment)
- .isEqualTo("ST-" + new Fingerprint().addString("foo=foosball").hexDigestAndReset());
+ .isEqualTo(
+ FunctionTransitionUtil.transitionDirectoryNameFragment(
+ ImmutableList.of("foo=foosball")));
// test:middle (foo_transition, zee_transition, bar_transition)
@SuppressWarnings("unchecked")
@@ -1597,12 +1601,8 @@
Maps.immutableEntry(Label.parseAbsoluteUnchecked("//test:zee"), "zeesball"));
assertThat(getCoreOptions(middle).transitionDirectoryNameFragment)
.isEqualTo(
- "ST-"
- + new Fingerprint()
- .addString("//test:zee=zeesball")
- .addString("bar=barsball")
- .addString("foo=foosball")
- .hexDigestAndReset());
+ FunctionTransitionUtil.transitionDirectoryNameFragment(
+ ImmutableList.of("//test:zee=zeesball", "bar=barsball", "foo=foosball")));
// test:bottom (foo_transition, zee_transition, bar_transition, xan_transition)
@SuppressWarnings("unchecked")
@@ -1620,13 +1620,9 @@
Maps.immutableEntry(Label.parseAbsoluteUnchecked("//test:xan"), "xansball"));
assertThat(getCoreOptions(bottom).transitionDirectoryNameFragment)
.isEqualTo(
- "ST-"
- + new Fingerprint()
- .addString("//test:xan=xansball")
- .addString("//test:zee=zeesball")
- .addString("bar=barsball")
- .addString("foo=foosball")
- .hexDigestAndReset());
+ FunctionTransitionUtil.transitionDirectoryNameFragment(
+ ImmutableList.of(
+ "//test:xan=xansball", "//test:zee=zeesball", "bar=barsball", "foo=foosball")));
}
@Test
