Fix label resolution for external repos (#896) `%package(...)%` resolved relative to rules_cc itself, not the location where the `cc_toolchain` was defined. For `cc_shared_library`, the existing initializer did resolve labels correctly, but failed to do so for `select` expressions. Closes #896 COPYBARA_INTEGRATE_REVIEW=https://github.com/bazelbuild/rules_cc/pull/896 from fmeum:claude/package-paths-bzlmod-toolchain-w8lcdu 0468ba063b2cc44e5555b6787e1dcd33020600be PiperOrigin-RevId: 1002808922 Change-Id: Iea958478b558c0a5f80e6edd84bc13f20d6a9b98
diff --git a/cc/private/rules_impl/cc_shared_library.bzl b/cc/private/rules_impl/cc_shared_library.bzl index 7df94c7..8566333 100644 --- a/cc/private/rules_impl/cc_shared_library.bzl +++ b/cc/private/rules_impl/cc_shared_library.bzl
@@ -79,31 +79,8 @@ implementation = _graph_structure_aspect_impl, ) -def _cc_shared_library_initializer(**kwargs): - """Converts labels in exports_filter into canonical form relative to the current repository. - - This conversion can only be done in a macro as it requires access to the repository mapping of - the repository containing the cc_shared_library target. This mapping is automatically - applied to label attributes, but exports_filter is a list of strings attribute. - """ - if "exports_filter" not in kwargs: - return kwargs - - raw_exports_filter = kwargs["exports_filter"] - if type(raw_exports_filter) != type([]): - # TODO: Also canonicalize labels in selects once macros can operate on them. - # https://github.com/bazelbuild/bazel/issues/14157 - return kwargs - - canonical_exports_filter = [ - str(native.package_relative_label(s)) - for s in raw_exports_filter - ] - return kwargs | {"exports_filter": canonical_exports_filter} - cc_shared_library = rule( implementation = proxy, - initializer = _cc_shared_library_initializer, doc = """ <p>It produces a shared library.</p>
diff --git a/cc/private/rules_impl/cc_shared_library_impl.bzl b/cc/private/rules_impl/cc_shared_library_impl.bzl index 44abc8a..5c8bce7 100644 --- a/cc/private/rules_impl/cc_shared_library_impl.bzl +++ b/cc/private/rules_impl/cc_shared_library_impl.bzl
@@ -293,9 +293,9 @@ return pattern.package == value.package and pattern.name == value.name -def _check_if_target_should_be_exported_with_filter(target, current_label, exports_filter): +def _check_if_target_should_be_exported_with_filter(ctx, target, exports_filter): for export_filter in exports_filter: - export_filter_label = current_label.relative(export_filter) + export_filter_label = ctx.package_relative_label(export_filter) if _check_if_target_under_path(target, export_filter_label): return True @@ -478,8 +478,8 @@ linker_input, ) if _check_if_target_should_be_exported_with_filter( + ctx, linker_input.owner, - ctx.label, ctx.attr.exports_filter, ): exports[owner] = True
diff --git a/cc/private/rules_impl/cc_toolchain_provider_helper.bzl b/cc/private/rules_impl/cc_toolchain_provider_helper.bzl index 6495fe2..b0dea3f 100644 --- a/cc/private/rules_impl/cc_toolchain_provider_helper.bzl +++ b/cc/private/rules_impl/cc_toolchain_provider_helper.bzl
@@ -73,7 +73,7 @@ return tool_paths_collector -def _resolve_include_dir(target_label, s, sysroot, crosstool_path): +def _resolve_include_dir(ctx, s, sysroot, crosstool_path): """ Resolve the given include directory. If it starts with %sysroot%/, that part is replaced with the actual sysroot. @@ -102,7 +102,7 @@ # This is necessary to avoid the hard work of parsing, # and use an already existing API. - dummy_label = target_label.relative(package + ":dummy_target") + dummy_label = ctx.package_relative_label(package + ":dummy_target") repo_prefix = dummy_label.workspace_root path_prefix = get_relative_path(repo_prefix, dummy_label.package) path_start_index = package_end_index + len(_PACKAGE_END) @@ -234,7 +234,7 @@ builtin_include_directories = [] for s in toolchain_config_info.cxx_builtin_include_directories: - builtin_include_directories.append(_resolve_include_dir(ctx.label, s, sysroot, tools_directory)) + builtin_include_directories.append(_resolve_include_dir(ctx, s, sysroot, tools_directory)) build_variables_dict = _get_cc_toolchain_vars(ctx.fragments.cpp, sysroot) build_variables = cc_common.cc_toolchain_variables(vars = build_variables_dict)
diff --git a/tests/builtins_bzl/cc/cc_shared_library/test/BUILD b/tests/builtins_bzl/cc/cc_shared_library/test/BUILD index d6949cd..7d45fbf 100644 --- a/tests/builtins_bzl/cc/cc_shared_library/test/BUILD +++ b/tests/builtins_bzl/cc/cc_shared_library/test/BUILD
@@ -552,6 +552,19 @@ ], ) +exports_test( + name = "external_select_exports_filter_exports_test", + bazel_only = True, + # target = "external_export_so", # copybara-comment-this-out-please + target = "@cross_repo_test//:bar_so", # copybara-uncomment-this-please + targets_that_should_be_claimed_to_be_exported = [ + # buildifier: disable=canonical-repository + "@@test_repo+//:bar", # copybara-uncomment-this-please + # buildifier: disable=canonical-repository + "@@test_repo+//:bar_wrapper", # copybara-uncomment-this-please + ], +) + pdb_test( name = "pdb_test", target = ":foo_so",
diff --git a/tests/builtins_bzl/cc/cc_shared_library/test2/BUILD.bazel b/tests/builtins_bzl/cc/cc_shared_library/test2/BUILD.bazel index 88a92df..133dff6 100644 --- a/tests/builtins_bzl/cc/cc_shared_library/test2/BUILD.bazel +++ b/tests/builtins_bzl/cc/cc_shared_library/test2/BUILD.bazel
@@ -1,4 +1,5 @@ load("@rules_cc//cc:cc_library.bzl", "cc_library") +load("@rules_cc//cc:cc_shared_library.bzl", "cc_shared_library") licenses(["notice"]) @@ -8,3 +9,17 @@ hdrs = ["bar.h"], visibility = ["//visibility:public"], ) + +cc_library( + name = "bar_wrapper", + visibility = ["//visibility:public"], + deps = [":bar"], +) + +cc_shared_library( + name = "bar_so", + exports_filter = select({"//conditions:default": ["@test_repo//:__pkg__"]}), + tags = ["manual"], + visibility = ["//visibility:public"], + deps = [":bar_wrapper"], +)
diff --git a/tests/builtins_bzl/cc/cc_shared_library/test2/toolchain_include_dirs/BUILD.bazel b/tests/builtins_bzl/cc/cc_shared_library/test2/toolchain_include_dirs/BUILD.bazel new file mode 100644 index 0000000..b5605a2 --- /dev/null +++ b/tests/builtins_bzl/cc/cc_shared_library/test2/toolchain_include_dirs/BUILD.bazel
@@ -0,0 +1,31 @@ +load("@rules_cc//cc/toolchains:cc_toolchain.bzl", "cc_toolchain") +load(":cc_toolchain_config.bzl", "cc_toolchain_config") + +package(default_visibility = ["//visibility:public"]) + +filegroup(name = "empty") + +cc_toolchain_config( + name = "toolchain_config", + cxx_builtin_include_directories = [ + "%package(@test_repo//toolchain_include_dirs)%/include", + "%package(//toolchain_include_dirs)%/other", + "%package(@rules_cc//cc)%", + ], + tags = ["manual"], +) + +cc_toolchain( + name = "toolchain", + all_files = ":empty", + ar_files = ":empty", + as_files = ":empty", + compiler_files = ":empty", + dwp_files = ":empty", + linker_files = ":empty", + objcopy_files = ":empty", + strip_files = ":empty", + tags = ["manual"], + toolchain_config = ":toolchain_config", + toolchain_identifier = "test-toolchain", +)
diff --git a/tests/builtins_bzl/cc/cc_shared_library/test2/toolchain_include_dirs/cc_toolchain_config.bzl b/tests/builtins_bzl/cc/cc_shared_library/test2/toolchain_include_dirs/cc_toolchain_config.bzl new file mode 100644 index 0000000..2b2c922 --- /dev/null +++ b/tests/builtins_bzl/cc/cc_shared_library/test2/toolchain_include_dirs/cc_toolchain_config.bzl
@@ -0,0 +1,23 @@ +"""A minimal cc_toolchain_config rule for testing cxx_builtin_include_directories.""" + +load("@rules_cc//cc/common:cc_common.bzl", "cc_common") +load("@rules_cc//cc/toolchains:cc_toolchain_config_info.bzl", "CcToolchainConfigInfo") + +def _impl(ctx): + return cc_common.create_cc_toolchain_config_info( + ctx = ctx, + toolchain_identifier = "test-toolchain", + compiler = "test-compiler", + target_system_name = "local", + target_cpu = "k8", + target_libc = "unknown", + cxx_builtin_include_directories = ctx.attr.cxx_builtin_include_directories, + ) + +cc_toolchain_config = rule( + implementation = _impl, + attrs = { + "cxx_builtin_include_directories": attr.string_list(), + }, + provides = [CcToolchainConfigInfo], +)
diff --git a/tests/cc/common/BUILD b/tests/cc/common/BUILD index 94535da..cc4dda4 100644 --- a/tests/cc/common/BUILD +++ b/tests/cc/common/BUILD
@@ -12,6 +12,7 @@ load(":cc_linkstamp_compile_test.bzl", "cc_linkstamp_compile_tests") load(":cc_objc_library_configured_target_tests.bzl", "cc_objc_library_configured_target_tests") load(":cc_output_groups_test.bzl", "cc_output_groups_tests") +load(":cc_toolchain_include_directories_test.bzl", "cc_toolchain_include_directories_tests") load(":compile_build_variables_tests.bzl", "compile_build_variables_tests") load(":cpp_modules_test.bzl", "cpp_modules_tests") load(":debug_info_packaging_test.bzl", "debug_info_packaging_tests") @@ -55,3 +56,5 @@ cc_linkstamp_compile_tests(name = "cc_linkstamp_compile_tests") cc_binary_fdo_tests(name = "cc_binary_fdo_tests") + +cc_toolchain_include_directories_tests(name = "cc_toolchain_include_directories_tests")
diff --git a/tests/cc/common/cc_toolchain_include_directories_test.bzl b/tests/cc/common/cc_toolchain_include_directories_test.bzl new file mode 100644 index 0000000..fdfa887 --- /dev/null +++ b/tests/cc/common/cc_toolchain_include_directories_test.bzl
@@ -0,0 +1,37 @@ +"""Tests for the resolution of cxx_builtin_include_directories by cc_toolchain.""" + +load("@bazel_features//:features.bzl", "bazel_features") +load("@bazel_skylib//lib:paths.bzl", "paths") +load("@rules_testing//lib:analysis_test.bzl", "test_suite") +load("//cc/common:cc_common.bzl", "cc_common") +load("//tests/cc/testutil:cc_analysis_test.bzl", "cc_analysis_test") + +# buildifier: disable=unused-variable +def _test_package_include_directories_use_toolchain_repo_mapping(name): + cc_analysis_test( + name = name, + impl = _test_package_include_directories_use_toolchain_repo_mapping_impl, + target = "@cross_repo_test//toolchain_include_dirs:toolchain", + ) + +def _test_package_include_directories_use_toolchain_repo_mapping_impl(env, target): + test_repo_root = Label("@cross_repo_test//toolchain_include_dirs:toolchain").workspace_root + rules_cc_root = Label("//cc:BUILD").workspace_root + env.expect.that_collection( + target[cc_common.CcToolchainInfo].built_in_include_directories, + ).contains_exactly([ + paths.join(test_repo_root, "toolchain_include_dirs/include"), + paths.join(test_repo_root, "toolchain_include_dirs/other"), + paths.join(rules_cc_root, "cc"), + ]).in_order() + +def cc_toolchain_include_directories_tests(name): + tests = [] + if bazel_features.cc.cc_common_is_in_rules_cc: + tests.append(_test_package_include_directories_use_toolchain_repo_mapping) # copybara-uncomment-this-please + # pass # copybara-comment-this-out-please + + test_suite( + name = name, + tests = tests, + )