From 94d8bd3c152c42b6c75d980b4302b22a3543437c Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 13 Apr 2023 07:47:07 -0700 Subject: [PATCH] Enable revised output directory hash suffix computation 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-` 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 https://github.com/bazelbuild/bazel/issues/14023 Closes #16910. PiperOrigin-RevId: 523999034 Change-Id: Id5548a00b62ebfe7889cd40177995210ecc224c1 --- .../lib/analysis/config/CoreOptions.java | 4 +- .../config/ExecutionTransitionFactory.java | 3 +- .../skyframe/BuildConfigurationFunction.java | 3 +- .../ExecutionTransitionFactoryTest.java | 3 +- .../devtools/build/lib/analysis/util/BUILD | 1 + .../analysis/util/ConfigurationTestCase.java | 5 +++ .../devtools/build/lib/rules/objc/BUILD | 2 + .../lib/rules/objc/ObjcRuleTestCase.java | 42 ++++++++++++++++--- 8 files changed, 52 insertions(+), 11 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 dcf0966c2b9106..8cf4e392eb115b 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 @@ -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}, @@ -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}, diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionTransitionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionTransitionFactory.java index a613ff3070cbb5..8351fed4cee587 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionTransitionFactory.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionTransitionFactory.java @@ -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; diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java index 2240ee58b5d11a..46dc2b55684550 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java @@ -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)) { diff --git a/src/test/java/com/google/devtools/build/lib/analysis/config/ExecutionTransitionFactoryTest.java b/src/test/java/com/google/devtools/build/lib/analysis/config/ExecutionTransitionFactoryTest.java index d0a0c0e938f05b..fa2d3b6b182f89 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/config/ExecutionTransitionFactoryTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/config/ExecutionTransitionFactoryTest.java @@ -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"); } } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD index dcf8de62cbefab..30fe172c1c3fbe 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD @@ -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", diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/ConfigurationTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/ConfigurationTestCase.java index 68af7fa14d6eb8..2c2f99024ef550 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/ConfigurationTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/ConfigurationTestCase.java @@ -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; @@ -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( diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/BUILD b/src/test/java/com/google/devtools/build/lib/rules/objc/BUILD index fe77b2ee883298..551e72edfb5a9a 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/objc/BUILD +++ b/src/test/java/com/google/devtools/build/lib/rules/objc/BUILD @@ -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", @@ -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", diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java index 9391790b08fca1..e3209fb6107620 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java @@ -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; @@ -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; @@ -169,6 +173,7 @@ private static String toolExecutable(String toolSrcPath) { TestConstants.PRODUCT_NAME, TestConstants.TOOLS_REPOSITORY_PATH_PREFIX + toolSrcPath); } + @SuppressWarnings("MissingCasesInEnumSwitch") private String configurationDir( String arch, ConfigurationDistinguisher configurationDistinguisher, @@ -176,6 +181,31 @@ 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); @@ -185,20 +215,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(); } @@ -669,7 +701,7 @@ protected void checkProvidesHdrsAndIncludes(RuleType ruleType, Optional getConfiguredTarget("//x:x", getAppleCrosstoolConfiguration()) .get(CcInfo.PROVIDER) .getCcCompilationContext(); - List declaredIncludeSrcs = + ImmutableList declaredIncludeSrcs = ccCompilationContext.getDeclaredIncludeSrcs().toList().stream() .map(x -> removeConfigFragment(x.getExecPathString())) .collect(toImmutableList());