From 42c16922c0c1631567975d66e4c026078e79be67 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 4e175abf8c6efb..32e527b543bf55 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 @@ -333,7 +333,7 @@ public ExecConfigurationDistinguisherSchemeConverter() { @Option( name = "experimental_exec_configuration_distinguisher", - defaultValue = "legacy", + defaultValue = "off", converter = ExecConfigurationDistinguisherSchemeConverter.class, documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, effectTags = {OptionEffectTag.AFFECTS_OUTPUTS}, @@ -578,7 +578,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 a4559c333d53c0..b5a2437bf786b2 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 @@ -186,7 +186,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 731401d0f5e238..01a208a37c7a63 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 @@ -138,7 +138,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 3582b14b63c6b6..94cadf5eba7346 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 @@ -212,7 +212,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 d8ff4b60f2ee40..7025c54335b4a1 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 @@ -48,6 +48,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 a8dd539e53faeb..79fdc3f6bbce55 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 @@ -25,6 +25,7 @@ import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection; 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 07b5b2631d10ac..9458b00b947cb7 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 @@ -77,6 +77,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", @@ -91,6 +92,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 0c7d748dc3ef4e..ac6b685cdcd5ca 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 @@ -40,6 +40,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; @@ -49,12 +51,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; @@ -170,6 +174,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, @@ -177,6 +182,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); @@ -186,20 +216,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(); } @@ -654,7 +686,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());