From 5ac62a2a5bfc04387d904f6ff1e74c6290bda9fd Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 5 Oct 2023 01:37:02 -0700 Subject: [PATCH] BEGIN_PUBLIC Switch Cpp `BuildInfo` system to new API. END_PUBLIC PiperOrigin-RevId: 570939842 Change-Id: Iee651c29b5334c5fb28f032e3c32fb0f2360e5f5 --- .../config/BuildConfigurationValue.java | 9 ++- .../starlark/StarlarkRuleContext.java | 15 ++-- .../build/lib/rules/cpp/CcModule.java | 22 +++--- .../lib/rules/cpp/CppLinkActionBuilder.java | 16 +++-- .../rules/nativedeps/NativeDepsHelper.java | 16 +++-- .../builtins_bzl/common/cc/cc_common.bzl | 5 -- .../common/python/py_executable.bzl | 30 +++++--- .../BuildConfigurationStarlarkTest.java | 4 +- .../lib/analysis/mock/BazelAnalysisMock.java | 10 ++- .../lib/rules/cpp/StarlarkCcCommonTest.java | 70 ++++--------------- 10 files changed, 88 insertions(+), 109 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java index 5817189ae9aef7..3b7043cab8bd4b 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java @@ -83,6 +83,13 @@ public class BuildConfigurationValue private static final Interner, Fragment>> fragmentsInterner = BlazeInterners.newWeakInterner(); + private static final ImmutableSet ANDROID_ALLOWLIST = + ImmutableSet.of( + BuiltinRestriction.allowlistEntry("", "third_party/bazel_rules/rules_android"), + BuiltinRestriction.allowlistEntry("build_bazel_rules_android", ""), + BuiltinRestriction.allowlistEntry("rules_android", ""), + BuiltinRestriction.allowlistEntry("", "tools/build_defs/android")); + /** Global state necessary to build a BuildConfiguration. */ public interface GlobalStateProvider { /** Computes the default shell environment for actions from the command line options. */ @@ -733,7 +740,7 @@ public boolean isToolConfiguration() { @Override public boolean isToolConfigurationForStarlark(StarlarkThread thread) throws EvalException { - BuiltinRestriction.failIfCalledOutsideBuiltins(thread); + BuiltinRestriction.failIfCalledOutsideAllowlist(thread, ANDROID_ALLOWLIST); return isToolConfiguration(); } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java index 05a9fa86c788f3..d89a0e0581765b 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java @@ -114,13 +114,14 @@ public final class StarlarkRuleContext implements StarlarkRuleContextApi, StarlarkActionContext { - static final ImmutableSet PRIVATE_STARLARKIFICATION_ALLOWLIST = - ImmutableSet.of( - BuiltinRestriction.allowlistEntry("", "test"), // for tests - BuiltinRestriction.allowlistEntry("", "third_party/bazel_rules/rules_android"), - BuiltinRestriction.allowlistEntry("build_bazel_rules_android", ""), - BuiltinRestriction.allowlistEntry("rules_android", ""), - BuiltinRestriction.allowlistEntry("", "tools/build_defs/android")); + public static final ImmutableSet + PRIVATE_STARLARKIFICATION_ALLOWLIST = + ImmutableSet.of( + BuiltinRestriction.allowlistEntry("", "test"), // for tests + BuiltinRestriction.allowlistEntry("", "third_party/bazel_rules/rules_android"), + BuiltinRestriction.allowlistEntry("build_bazel_rules_android", ""), + BuiltinRestriction.allowlistEntry("rules_android", ""), + BuiltinRestriction.allowlistEntry("", "tools/build_defs/android")); private static final String EXECUTABLE_OUTPUT_NAME = "executable"; diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java index 9329bad03b6c67..e335b242382228 100755 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java @@ -3001,7 +3001,15 @@ public void registerLinkstampCompileAction( compilationInputs.getSet(Artifact.class), /* nonCodeInputs= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER), inputsForValidation.getSet(Artifact.class), - ruleContext.getBuildInfo(CppBuildInfo.KEY), + AnalysisUtils.isStampingEnabled(ruleContext, ruleContext.getConfiguration()) + ? ccToolchain + .getCcBuildInfoTranslator() + .getOutputGroup("non_redacted_build_info_files") + .toList() + : ccToolchain + .getCcBuildInfoTranslator() + .getOutputGroup("redacted_build_info_files") + .toList(), /* additionalLinkstampDefines= */ ImmutableList.of(), ccToolchain, ruleContext.getConfiguration().isCodeCoverageEnabled(), @@ -3017,18 +3025,6 @@ public void registerLinkstampCompileAction( getSemantics())); } - @StarlarkMethod( - name = "get_build_info", - documented = false, - parameters = {@Param(name = "ctx")}, - useStarlarkThread = true) - public Sequence getBuildInfo(StarlarkRuleContext ruleContext, StarlarkThread thread) - throws EvalException, InterruptedException { - isCalledFromStarlarkCcCommon(thread); - return StarlarkList.immutableCopyOf( - ruleContext.getRuleContext().getBuildInfo(CppBuildInfo.KEY)); - } - @StarlarkMethod( name = "create_extra_link_time_library", documented = false, diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java index b5c3816840042e..a85850bc26f573 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java @@ -556,11 +556,17 @@ public CppLinkAction build() throws InterruptedException, RuleErrorException { } final ImmutableList buildInfoHeaderArtifacts = - !linkstamps.isEmpty() - ? actionConstructionContext - .getAnalysisEnvironment() - .getBuildInfo(isStampingEnabled, CppBuildInfo.KEY, configuration) - : ImmutableList.of(); + linkstamps.isEmpty() + ? ImmutableList.of() + : isStampingEnabled + ? toolchain + .getCcBuildInfoTranslator() + .getOutputGroup("non_redacted_build_info_files") + .toList() + : toolchain + .getCcBuildInfoTranslator() + .getOutputGroup("redacted_build_info_files") + .toList(); boolean needWholeArchive = wholeArchive diff --git a/src/main/java/com/google/devtools/build/lib/rules/nativedeps/NativeDepsHelper.java b/src/main/java/com/google/devtools/build/lib/rules/nativedeps/NativeDepsHelper.java index f267c2ddc21923..9f318977fa3741 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/nativedeps/NativeDepsHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/nativedeps/NativeDepsHelper.java @@ -41,7 +41,6 @@ import com.google.devtools.build.lib.rules.cpp.CcLinkingHelper; import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration; import com.google.devtools.build.lib.rules.cpp.CcToolchainProvider; -import com.google.devtools.build.lib.rules.cpp.CppBuildInfo; import com.google.devtools.build.lib.rules.cpp.CppConfiguration; import com.google.devtools.build.lib.rules.cpp.CppHelper; import com.google.devtools.build.lib.rules.cpp.CppLinkAction; @@ -189,12 +188,15 @@ public static NativeDepsRunfiles createNativeDepsAction( List buildInfoArtifacts = linkstamps.isEmpty() ? ImmutableList.of() - : ruleContext - .getAnalysisEnvironment() - .getBuildInfo( - AnalysisUtils.isStampingEnabled(ruleContext, configuration), - CppBuildInfo.KEY, - configuration); + : AnalysisUtils.isStampingEnabled(ruleContext, configuration) + ? toolchain + .getCcBuildInfoTranslator() + .getOutputGroup("non_redacted_build_info_files") + .toList() + : toolchain + .getCcBuildInfoTranslator() + .getOutputGroup("redacted_build_info_files") + .toList(); ImmutableSortedSet.Builder requestedFeaturesBuilder = ImmutableSortedSet.naturalOrder() diff --git a/src/main/starlark/builtins_bzl/common/cc/cc_common.bzl b/src/main/starlark/builtins_bzl/common/cc/cc_common.bzl index 366a611981595f..004b7e25fd1f5c 100644 --- a/src/main/starlark/builtins_bzl/common/cc/cc_common.bzl +++ b/src/main/starlark/builtins_bzl/common/cc/cc_common.bzl @@ -651,10 +651,6 @@ def _get_tool_requirement_for_action(*, feature_configuration, action_name): cc_common_internal.check_private_api(allowlist = _PRIVATE_STARLARKIFICATION_ALLOWLIST) return cc_common_internal.get_tool_requirement_for_action(feature_configuration = feature_configuration, action_name = action_name) -def _get_build_info(ctx): - cc_common_internal.check_private_api(allowlist = _PRIVATE_STARLARKIFICATION_ALLOWLIST) - return cc_common_internal.get_build_info(ctx) - def _create_extra_link_time_library(*, build_library_func, **kwargs): cc_common_internal.check_private_api(allowlist = _BUILTINS) return cc_common_internal.create_extra_link_time_library(build_library_func = build_library_func, **kwargs) @@ -919,7 +915,6 @@ cc_common = struct( create_debug_context = _create_debug_context, merge_debug_context = _merge_debug_context, get_tool_requirement_for_action = _get_tool_requirement_for_action, - get_build_info = _get_build_info, create_extra_link_time_library = _create_extra_link_time_library, register_linkstamp_compile_action = _register_linkstamp_compile_action, compile = _compile, diff --git a/src/main/starlark/builtins_bzl/common/python/py_executable.bzl b/src/main/starlark/builtins_bzl/common/python/py_executable.bzl index 7d0128cf4f150f..5ab99720e4e930 100644 --- a/src/main/starlark/builtins_bzl/common/python/py_executable.bzl +++ b/src/main/starlark/builtins_bzl/common/python/py_executable.bzl @@ -13,7 +13,17 @@ # limitations under the License. """Common functionality between test/binary executables.""" +load(":common/cc/cc_common.bzl", _cc_common = "cc_common") load(":common/cc/cc_helper.bzl", "cc_helper") +load( + ":common/python/attributes.bzl", + "AGNOSTIC_EXECUTABLE_ATTRS", + "COMMON_ATTRS", + "PY_SRCS_ATTRS", + "SRCS_VERSION_ALL_VALUES", + "create_srcs_attr", + "create_srcs_version_attr", +) load( ":common/python/common.bzl", "TOOLCHAIN_TYPE", @@ -27,15 +37,6 @@ load( "filter_to_py_srcs", "union_attrs", ) -load( - ":common/python/attributes.bzl", - "AGNOSTIC_EXECUTABLE_ATTRS", - "COMMON_ATTRS", - "PY_SRCS_ATTRS", - "SRCS_VERSION_ALL_VALUES", - "create_srcs_attr", - "create_srcs_version_attr", -) load( ":common/python/providers.bzl", "PyCcLinkParamsProvider", @@ -48,7 +49,6 @@ load( "IS_BAZEL", "PY_RUNTIME_ATTR_NAME", ) -load(":common/cc/cc_common.bzl", _cc_common = "cc_common") _py_builtins = _builtins.internal.py_builtins @@ -490,6 +490,7 @@ def _get_native_deps_details(ctx, *, semantics, cc_details, is_test): if share_native_deps: linked_lib = _create_shared_native_deps_dso( ctx, + cc_toolchain = cc_details.cc_toolchain, cc_info = cc_info, is_test = is_test, requested_features = cc_feature_config.requested_features, @@ -527,6 +528,7 @@ def _get_native_deps_details(ctx, *, semantics, cc_details, is_test): def _create_shared_native_deps_dso( ctx, *, + cc_toolchain, cc_info, is_test, feature_configuration, @@ -542,6 +544,12 @@ def _create_shared_native_deps_dso( feature_configuration = feature_configuration, ) ) + if not linkstamps: + build_info_artifacts = [] + elif cc_helper.is_stamping_enabled(ctx): + build_info_artifacts = cc_toolchain.build_info_files().non_redacted_build_info_files.to_list() + else: + build_info_artifacts = cc_toolchain.build_info_files().redacted_build_info_files.to_list() dso_hash = _get_shared_native_deps_hash( linker_inputs = cc_helper.get_static_mode_params_for_dynamic_library_libraries( depset([ @@ -556,7 +564,7 @@ def _create_shared_native_deps_dso( for flag in input.user_link_flags ], linkstamps = [linkstamp.file() for linkstamp in linkstamps.to_list()], - build_info_artifacts = _cc_common.get_build_info(ctx) if linkstamps else [], + build_info_artifacts = build_info_artifacts, features = requested_features, is_test_target_partially_disabled_thin_lto = is_test and partially_disabled_thin_lto, ) diff --git a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationStarlarkTest.java b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationStarlarkTest.java index 27fc4740af68a0..824ee5e350ae54 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationStarlarkTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationStarlarkTest.java @@ -73,9 +73,7 @@ public void testIsToolConfigurationIsBlocked() throws Exception { AssertionError e = assertThrows(AssertionError.class, () -> getConfiguredTarget("//example:custom")); - assertThat(e) - .hasMessageThat() - .contains("file '//example:rule.bzl' cannot use private @_builtins API"); + assertThat(e).hasMessageThat().contains("file '//example:rule.bzl' cannot use private API"); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java index 740ac5f8786fd7..9a90b7db5a460c 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java @@ -629,9 +629,15 @@ public void setupMockToolsRepository(MockToolsConfig config) throws IOException config.create( "embedded_tools/tools/build_defs/build_info/bazel_cc_build_info.bzl", "def _impl(ctx):", + " volatile_file = ctx.actions.declare_file('volatile_file.h')", + " non_volatile_file = ctx.actions.declare_file('non_volatile_file.h')", + " redacted_file = ctx.actions.declare_file('redacted_file.h')", + " ctx.actions.write(output = volatile_file, content = '')", + " ctx.actions.write(output = non_volatile_file, content = '')", + " ctx.actions.write(output = redacted_file, content = '')", " output_groups = {", - " 'non_redacted_build_info_files': depset([ctx.info_file, ctx.version_file]),", - " 'redacted_build_info_files': depset([ctx.version_file]),", + " 'non_redacted_build_info_files': depset([volatile_file, non_volatile_file]),", + " 'redacted_build_info_files': depset([redacted_file]),", " }", " return OutputGroupInfo(**output_groups)", "bazel_cc_build_info = rule(implementation = _impl)"); diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/StarlarkCcCommonTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/StarlarkCcCommonTest.java index b1469e32c449bd..f642a172a6b0df 100755 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/StarlarkCcCommonTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/StarlarkCcCommonTest.java @@ -16,7 +16,6 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth8.assertThat; import static com.google.devtools.build.lib.actions.util.ActionsTestUtil.baseArtifactNames; -import static com.google.devtools.build.lib.actions.util.ActionsTestUtil.prettyArtifactNames; import static com.google.devtools.build.lib.rules.cpp.SolibSymlinkAction.MAX_FILENAME_LENGTH; import static org.junit.Assert.assertThrows; @@ -28,7 +27,6 @@ import com.google.devtools.build.lib.actions.CommandLineExpansionException; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.analysis.ConfiguredTarget; -import com.google.devtools.build.lib.analysis.FileProvider; import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.analysis.util.AnalysisMock; import com.google.devtools.build.lib.analysis.util.AnalysisTestUtil; @@ -86,6 +84,11 @@ @RunWith(JUnit4.class) public class StarlarkCcCommonTest extends BuildViewTestCase { + private static final String REDACTED_ARTIFACT_PATH = + "tools/build_defs/build_info/redacted_file.h"; + private static final String NON_REDACTED_ARTIFACT_PATH = + "tools/build_defs/build_info/volatile_file.h"; + @Before public void setUp() throws Exception { scratch.file("myinfo/myinfo.bzl", "MyInfo = provider()"); @@ -6061,16 +6064,23 @@ private CppCompileAction getLinkstampCompileAction(String label) return linkstampCompileAction; } + public String getBuildInfoFile(String commonPath) { + if (AnalysisMock.get().isThisBazel()) { + return getRelativeOutputPath() + "/k8-fastbuild/bin/external/bazel_tools/" + commonPath; + } else { + return getRelativeOutputPath() + "/k8-fastbuild/bin/" + commonPath; + } + } + private void assertStampEnabled(CppCompileAction linkstampAction) throws CommandLineExpansionException { assertThat(linkstampAction.getArguments()) - .contains(getRelativeOutputPath() + "/k8-fastbuild/include/build-info-volatile.h"); + .contains(getBuildInfoFile(NON_REDACTED_ARTIFACT_PATH)); } private void assertStampDisabled(CppCompileAction linkstampAction) throws CommandLineExpansionException { - assertThat(linkstampAction.getArguments()) - .contains(getRelativeOutputPath() + "/k8-fastbuild/include/build-info-redacted.h"); + assertThat(linkstampAction.getArguments()).contains(getBuildInfoFile(REDACTED_ARTIFACT_PATH)); } @Test @@ -8063,56 +8073,6 @@ public void testCcToolchainCompileFilesNotAccessibleFromOutsideBuiltins() throws assertThat(e).hasMessageThat().contains("cannot use private API"); } - @Test - public void testGetBuildInfoArtifactsIsPrivateApi() throws Exception { - scratch.file( - "foo/BUILD", "load(':custom_rule.bzl', 'custom_rule')", "custom_rule(name = 'custom')"); - scratch.file( - "foo/custom_rule.bzl", - "def _impl(ctx):", - " cc_common.get_build_info(ctx)", - " return []", - "custom_rule = rule(", - " implementation = _impl,", - ")"); - invalidatePackages(); - - AssertionError e = - assertThrows(AssertionError.class, () -> getConfiguredTarget("//foo:custom")); - - assertThat(e).hasMessageThat().contains("cannot use private API"); - } - - @Test - public void testBuildInfoArtifacts() throws Exception { - scratch.file( - "bazel_internal/test_rules/cc/rule.bzl", - "def _impl(ctx):", - " artifacts = cc_common.get_build_info(ctx)", - " return [DefaultInfo(files = depset(artifacts))]", - "build_info_rule = rule(", - " implementation = _impl,", - " attrs = {'stamp': attr.int()},", - ")"); - scratch.file( - "bazel_internal/test_rules/cc/BUILD", - "load(':rule.bzl', 'build_info_rule')", - "build_info_rule(name = 'stamped', stamp = 1,)", - "build_info_rule(name = 'unstamped', stamp = 0,)"); - assertThat( - prettyArtifactNames( - getConfiguredTarget("//bazel_internal/test_rules/cc:stamped") - .getProvider(FileProvider.class) - .getFilesToBuild())) - .containsExactly("build-info-nonvolatile.h", "build-info-volatile.h"); - assertThat( - prettyArtifactNames( - getConfiguredTarget("//bazel_internal/test_rules/cc:unstamped") - .getProvider(FileProvider.class) - .getFilesToBuild())) - .containsExactly("build-info-redacted.h"); - } - @Test public void testCheckPrivateApiCanOnlyBeCalledFromCcCommonBzl() throws Exception { scratch.file(