Skip to content

Commit

Permalink
Enable revised output directory hash suffix computation
Browse files Browse the repository at this point in the history
Flips `--experimental_output_directory_naming_scheme` and
`--experimental_exec_configuration_distinguisher` to
`diff_against_baseline` and `off`, respectively, which makes it so that
Starlark transitions can return to previous configurations, thus
improving cache hit rates.

Also changes the fixed platform suffix in the exec configuration from an
empty string to `"exec"` for compatibility with logic that looks for the
 substring `-exec-` in paths to determine whether the current
configuration is the exec configuration.

As a result of this commit, output directory paths can change in the
following ways:
* `-ST-<hash>` suffixes can change or disappear and, in rare cases,
  appear where they previously didn't
* for exec configuration directories, `k8-opt-exec-2B5CBBC6` becomes
  `k8-opt-exec-ST-011b9bdc32ed`

This may result in tool paths that are seven characters longer, which
can cause builds on Windows to fail that were already very close to the
`MAX_PATH` limit. A follow-up commit will reduce the length of the hash
by three characters, bringing that increase down to four.

Work towards bazelbuild#14023
  • Loading branch information
fmeum committed Mar 8, 2023
1 parent 486a964 commit 84f7911
Show file tree
Hide file tree
Showing 8 changed files with 48 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ public ExecConfigurationDistinguisherSchemeConverter() {

@Option(
name = "experimental_exec_configuration_distinguisher",
defaultValue = "legacy",
defaultValue = "off",
converter = ExecConfigurationDistinguisherSchemeConverter.class,
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {OptionEffectTag.AFFECTS_OUTPUTS},
Expand Down Expand Up @@ -593,7 +593,7 @@ public OutputDirectoryNamingSchemeConverter() {

@Option(
name = "experimental_output_directory_naming_scheme",
defaultValue = "legacy",
defaultValue = "diff_against_baseline",
converter = OutputDirectoryNamingSchemeConverter.class,
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {OptionEffectTag.AFFECTS_OUTPUTS},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,8 @@ private static BuildOptions transitionImpl(BuildOptionsView options, Label execu
result = result.clone();
break;
default:
// else if OFF do nothing
// else if OFF just mark that we are now in an exec transition
coreOptions.platformSuffix = "exec";
}

return result;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,8 @@ private static final class BuildConfigurationFunctionException extends SkyFuncti
* Compute the hash for the new BuildOptions based on the names and values of all options (both
* native and Starlark) that are different from some supplied baseline configuration.
*/
private static String computeNameFragmentWithDiff(
@VisibleForTesting
public static String computeNameFragmentWithDiff(
BuildOptions toOptions, BuildOptions baselineOptions) {
// Quick short-circuit for trivial case.
if (toOptions.equals(baselineOptions)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,6 @@ public void executionTransition_confDist_off()
new StoredEventHandler());

assertThat(result.get(CoreOptions.class).affectedByStarlarkTransition).isEmpty();
assertThat(result.get(CoreOptions.class).platformSuffix)
.isEqualTo(options.get(CoreOptions.class).platformSuffix);
assertThat(result.get(CoreOptions.class).platformSuffix).isEqualTo("exec");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/analysis:config/config_conditions",
"//src/main/java/com/google/devtools/build/lib/analysis:config/config_matching_provider",
"//src/main/java/com/google/devtools/build/lib/analysis:config/core_option_converters",
"//src/main/java/com/google/devtools/build/lib/analysis:config/core_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/execution_transition_factory",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_factory",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.google.devtools.build.lib.analysis.ServerDirectories;
import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
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.FragmentFactory;
import com.google.devtools.build.lib.analysis.config.FragmentOptions;
import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
Expand Down Expand Up @@ -130,6 +131,10 @@ public final void initializeSkyframeExecutor() throws Exception {
SkyframeExecutorTestHelper.process(skyframeExecutor);
skyframeExecutor.injectExtraPrecomputedValues(
ImmutableList.of(
PrecomputedValue.injected(
PrecomputedValue.BASELINE_CONFIGURATION,
BuildOptions.getDefaultBuildOptionsForFragments(
ImmutableList.of(CoreOptions.class))),
PrecomputedValue.injected(
RepositoryDelegatorFunction.RESOLVED_FILE_INSTEAD_OF_WORKSPACE, Optional.empty()),
PrecomputedValue.injected(
Expand Down
2 changes: 2 additions & 0 deletions src/test/java/com/google/devtools/build/lib/rules/objc/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration",
"//src/main/java/com/google/devtools/build/lib/analysis:config/build_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/compilation_mode",
"//src/main/java/com/google/devtools/build/lib/analysis:config/core_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/invalid_configuration_exception",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/split_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
Expand All @@ -94,6 +95,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/rules/apple/cpp",
"//src/main/java/com/google/devtools/build/lib/rules/cpp",
"//src/main/java/com/google/devtools/build/lib/rules/objc",
"//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_cluster",
"//src/main/java/com/google/devtools/build/lib/util",
"//src/main/java/com/google/devtools/build/lib/vfs",
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@
import com.google.devtools.build.lib.analysis.config.BuildOptions;
import com.google.devtools.build.lib.analysis.config.BuildOptionsView;
import com.google.devtools.build.lib.analysis.config.CompilationMode;
import com.google.devtools.build.lib.analysis.config.CoreOptions;
import com.google.devtools.build.lib.analysis.config.CoreOptions.OutputDirectoryNamingScheme;
import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
import com.google.devtools.build.lib.analysis.config.transitions.SplitTransition;
import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
Expand All @@ -48,12 +50,14 @@
import com.google.devtools.build.lib.packages.util.MockObjcSupport;
import com.google.devtools.build.lib.rules.apple.AppleCommandLineOptions;
import com.google.devtools.build.lib.rules.apple.AppleConfiguration.ConfigurationDistinguisher;
import com.google.devtools.build.lib.rules.apple.ApplePlatform.PlatformType;
import com.google.devtools.build.lib.rules.apple.AppleToolchain;
import com.google.devtools.build.lib.rules.apple.DottedVersion;
import com.google.devtools.build.lib.rules.cpp.CcCompilationContext;
import com.google.devtools.build.lib.rules.cpp.CcInfo;
import com.google.devtools.build.lib.rules.cpp.CppLinkAction;
import com.google.devtools.build.lib.rules.objc.CompilationSupport.ExtraLinkArgs;
import com.google.devtools.build.lib.skyframe.BuildConfigurationFunction;
import com.google.devtools.build.lib.testutil.Scratch;
import com.google.devtools.build.lib.testutil.TestConstants;
import com.google.devtools.build.lib.vfs.PathFragment;
Expand Down Expand Up @@ -176,6 +180,29 @@ private String configurationDir(
CompilationMode compilationMode) {
String minOsSegment = minOsVersion == null ? "" : "-min" + minOsVersion;
String modeSegment = compilationModeFlag(compilationMode);

String hash = "";
if (targetConfig.getOptions().get(CoreOptions.class).outputDirectoryNamingScheme
== OutputDirectoryNamingScheme.DIFF_AGAINST_BASELINE) {
PlatformType platformType = null;
switch (configurationDistinguisher) {
case APPLEBIN_IOS:
platformType = PlatformType.IOS;
break;
case APPLEBIN_WATCHOS:
platformType = PlatformType.WATCHOS;
break;
}
BuildOptions transitionedConfig = targetConfig.cloneOptions();
transitionedConfig.get(CoreOptions.class).cpu = platformType + "_" + arch;
transitionedConfig.get(
AppleCommandLineOptions.class).configurationDistinguisher = configurationDistinguisher;
transitionedConfig.get(AppleCommandLineOptions.class).applePlatformType = platformType;
transitionedConfig.get(AppleCommandLineOptions.class).appleSplitCpu = arch;
hash = "-" + BuildConfigurationFunction.computeNameFragmentWithDiff(
transitionedConfig, targetConfig.getOptions());
}

switch (configurationDistinguisher) {
case UNKNOWN:
return String.format("%s-out/ios_%s-%s/", TestConstants.PRODUCT_NAME, arch, modeSegment);
Expand All @@ -185,20 +212,22 @@ private String configurationDir(
TestConstants.PRODUCT_NAME, arch, minOsSegment, modeSegment);
case APPLEBIN_IOS:
return String.format(
"%1$s-out/ios-%2$s%4$s-%3$s-ios_%2$s-%5$s/",
"%1$s-out/ios-%2$s%4$s-%3$s-ios_%2$s-%5$s%6$s/",
TestConstants.PRODUCT_NAME,
arch,
configurationDistinguisher.toString().toLowerCase(Locale.US),
minOsSegment,
modeSegment);
modeSegment,
hash);
case APPLEBIN_WATCHOS:
return String.format(
"%1$s-out/watchos-%2$s%4$s-%3$s-watchos_%2$s-%5$s/",
"%1$s-out/watchos-%2$s%4$s-%3$s-watchos_%2$s-%5$s%6$s/",
TestConstants.PRODUCT_NAME,
arch,
configurationDistinguisher.toString().toLowerCase(Locale.US),
minOsSegment,
modeSegment);
modeSegment,
hash);
default:
throw new AssertionError();
}
Expand Down

0 comments on commit 84f7911

Please sign in to comment.