Fix versioned shared libraries for macOS toolchain (#20847)
The toolchain was passing -l:name. This mechanism doesn' t exist on
macOS, instead the full path to the shared library should be passed.
Mainline commit:
https://github.com/bazelbuild/bazel/commit/f0ade80ce920be0719b1a43a40258397f68a944d
As it says on that mainline commit, it cannot be checked in with a test
unlike this PR, because the macOS toolchain was moved outside of Bazel.
Therefore, the change must go in, the macOS toolchain needs to be
updated, then the test can be added. This change does go in with a test
because the toolchain is still embedded in Bazel for 6.5.
Fixes https://github.com/bazelbuild/bazel/issues/20487
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainVariables.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainVariables.java
index a8b7d0f..b9611d3 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainVariables.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainVariables.java
@@ -671,6 +671,7 @@
public abstract static class LibraryToLinkValue extends VariableValueAdapter {
public static final String OBJECT_FILES_FIELD_NAME = "object_files";
public static final String NAME_FIELD_NAME = "name";
+ public static final String PATH_FIELD_NAME = "path";
public static final String TYPE_FIELD_NAME = "type";
public static final String IS_WHOLE_ARCHIVE_FIELD_NAME = "is_whole_archive";
@@ -680,8 +681,8 @@
return new ForDynamicLibrary(name);
}
- public static LibraryToLinkValue forVersionedDynamicLibrary(String name) {
- return new ForVersionedDynamicLibrary(name);
+ public static LibraryToLinkValue forVersionedDynamicLibrary(String name, String path) {
+ return new ForVersionedDynamicLibrary(name, path);
}
public static LibraryToLinkValue forInterfaceLibrary(String name) {
@@ -802,8 +803,40 @@
}
private static final class ForVersionedDynamicLibrary extends LibraryToLinkValueWithName {
- private ForVersionedDynamicLibrary(String name) {
+ private String path;
+
+ private ForVersionedDynamicLibrary(String name, String path) {
super(name);
+ this.path = path;
+ }
+
+ @Override
+ public VariableValue getFieldValue(
+ String variableName,
+ String field,
+ @Nullable ArtifactExpander expander,
+ boolean throwOnMissingVariable) {
+ if (PATH_FIELD_NAME.equals(field)) {
+ return new StringValue(path);
+ }
+ return super.getFieldValue(variableName, field, expander, throwOnMissingVariable);
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (!(obj instanceof ForVersionedDynamicLibrary)) {
+ return false;
+ }
+ if (this == obj) {
+ return true;
+ }
+ ForVersionedDynamicLibrary other = (ForVersionedDynamicLibrary) obj;
+ return this.path.equals(other.path) && super.equals(other);
+ }
+
+ @Override
+ public int hashCode() {
+ return 31 * super.hashCode() + path.hashCode();
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java
index e7561c2..c77af14 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java
@@ -822,7 +822,7 @@
" variable: 'libraries_to_link.type'",
" value: 'versioned_dynamic_library'",
" }",
- " flag: '-l:%{libraries_to_link.name}'",
+ " flag: '%{libraries_to_link.path}'",
" }"),
" flag_group {",
" expand_if_equal: {",
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java
index 0704dde..ec1b6bc 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java
@@ -613,7 +613,7 @@
librariesToLink.addValue(LibraryToLinkValue.forDynamicLibrary(libName));
} else if (CppFileTypes.SHARED_LIBRARY.matches(name)
|| CppFileTypes.VERSIONED_SHARED_LIBRARY.matches(name)) {
- librariesToLink.addValue(LibraryToLinkValue.forVersionedDynamicLibrary(name));
+ librariesToLink.addValue(LibraryToLinkValue.forVersionedDynamicLibrary(name, inputArtifact.getExecPathString()));
} else {
// Interface shared objects have a non-standard extension
// that the linker won't be able to find. So use the
diff --git a/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/BUILD.builtin_test b/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/BUILD.builtin_test
index 1962316..b429c32 100644
--- a/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/BUILD.builtin_test
+++ b/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/BUILD.builtin_test
@@ -115,6 +115,7 @@
"bar_so",
"//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library3:diff_pkg_so",
"private_lib_so",
+ "renamed_so_file_2",
],
features = ["windows_export_all_symbols"],
exports_filter = [
@@ -385,6 +386,11 @@
srcs = [":private_cc_library.cc"]
)
+cc_library(
+ name = "private_lib_2",
+ srcs = [":private_cc_library.cc"]
+)
+
build_failure_test(
name = "link_once_repeated_test_binary",
messages = [
@@ -447,6 +453,15 @@
shared_lib_name = "renamed_so_file.so",
)
+cc_shared_library(
+ name = "renamed_so_file_2",
+ features = ["windows_export_all_symbols"],
+ deps = [
+ ":private_lib_2",
+ ],
+ shared_lib_name = "renamed_so_file_2.so",
+)
+
cc_library(
name = "direct_so_file_cc_lib",
srcs = [
diff --git a/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/starlark_tests.bzl b/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/starlark_tests.bzl
index a1d8d18..5c66ad0 100644
--- a/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/starlark_tests.bzl
+++ b/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/starlark_tests.bzl
@@ -166,6 +166,7 @@
"libbar_so.so",
"libdiff_pkg_so.so",
"libprivate_lib_so.so",
+ "renamed_so_file_2.so",
"Smain_Sstarlark_Stests_Sbuiltins_Ubzl_Scc_Scc_Ushared_Ulibrary_Stest_Ucc_Ushared_Ulibrary_Slibfoo_Uso.so",
"Smain_Sstarlark_Stests_Sbuiltins_Ubzl_Scc_Scc_Ushared_Ulibrary_Stest_Ucc_Ushared_Ulibrary_Slibbar_Uso.so",
"Smain_Sstarlark_Stests_Sbuiltins_Ubzl_Scc_Scc_Ushared_Ulibrary_Stest_Ucc_Ushared_Ulibrary3_Slibdiff_Upkg_Uso.so",
diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/mock/osx_cc_toolchain_config.bzl b/src/test/java/com/google/devtools/build/lib/packages/util/mock/osx_cc_toolchain_config.bzl
index b3015cb..7bc9aca 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/util/mock/osx_cc_toolchain_config.bzl
+++ b/src/test/java/com/google/devtools/build/lib/packages/util/mock/osx_cc_toolchain_config.bzl
@@ -7540,11 +7540,11 @@
flag_group(
flag_groups = [
flag_group(
- flags = ["-l:%{libraries_to_link.name}"],
+ flags = ["%{libraries_to_link.path}"],
expand_if_false = "libraries_to_link.is_whole_archive",
),
flag_group(
- flags = ["-Wl,-force_load,-l:%{libraries_to_link.name}"],
+ flags = ["-Wl,-force_load,%{libraries_to_link.path}"],
expand_if_true = "libraries_to_link.is_whole_archive",
),
],
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/LibraryToLinkValueTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/LibraryToLinkValueTest.java
index 243e6ec..ecf4c34 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/LibraryToLinkValueTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/LibraryToLinkValueTest.java
@@ -47,9 +47,9 @@
// #forVersionedDynamicLibrary
equalsTester.addEqualityGroup(
- LibraryToLinkValue.forVersionedDynamicLibrary("foo"),
- LibraryToLinkValue.forVersionedDynamicLibrary("foo"));
- equalsTester.addEqualityGroup(LibraryToLinkValue.forVersionedDynamicLibrary("bar"));
+ LibraryToLinkValue.forVersionedDynamicLibrary("foo", /* path= */ ""),
+ LibraryToLinkValue.forVersionedDynamicLibrary("foo", /* path= */ ""));
+ equalsTester.addEqualityGroup(LibraryToLinkValue.forVersionedDynamicLibrary("bar", /* path= */ ""));
// #forInterfaceLibrary
equalsTester.addEqualityGroup(
@@ -135,7 +135,7 @@
@Test
public void getFieldValue_forVersionedDynamicLibrary() {
- LibraryToLinkValue libraryToLinkValue = LibraryToLinkValue.forVersionedDynamicLibrary("foo");
+ LibraryToLinkValue libraryToLinkValue = LibraryToLinkValue.forVersionedDynamicLibrary("foo", "foo/bar.so");
assertThat(
libraryToLinkValue.getFieldValue(
/*variableName=*/ "variable name doesn't matter",
@@ -144,6 +144,14 @@
/*throwOnMissingVariable=*/ false))
.isEqualTo(new StringValue("versioned_dynamic_library"));
assertThat(
+ libraryToLinkValue
+ .getFieldValue(
+ /* variableName= */ "variable name doesn't matter",
+ /* field= */ "path",
+ /* expander= */ null,
+ /* throwOnMissingVariable= */ false))
+ .isEqualTo(new StringValue("foo/bar.so"));
+ assertThat(
libraryToLinkValue.getFieldValue(
/*variableName=*/ "variable name doesn't matter",
/*field=*/ "is_whole_archive",
diff --git a/tools/cpp/osx_cc_wrapper.sh.tpl b/tools/cpp/osx_cc_wrapper.sh.tpl
index 8264090..fa60f33 100644
--- a/tools/cpp/osx_cc_wrapper.sh.tpl
+++ b/tools/cpp/osx_cc_wrapper.sh.tpl
@@ -28,6 +28,7 @@
set -eu
LIBS=
+LIB_PATHS=
LIB_DIRS=
RPATHS=
OUTPUT=
@@ -38,6 +39,10 @@
OUTPUT=$opt
elif [[ "$opt" =~ ^-l(.*)$ ]]; then
LIBS="${BASH_REMATCH[1]} $LIBS"
+ elif [[ "$opt" =~ ^(.*)\.so$ ]]; then
+ LIB_PATHS="${opt} $LIB_PATHS"
+ elif [[ "$opt" =~ ^(.*)\.dylib$ ]]; then
+ LIB_PATHS="${opt} $LIB_PATHS"
elif [[ "$opt" =~ ^-L(.*)$ ]]; then
LIB_DIRS="${BASH_REMATCH[1]} $LIB_DIRS"
elif [[ "$opt" =~ ^\@loader_path/(.*)$ ]]; then
@@ -94,6 +99,11 @@
get_realpath $1 | sed 's|^.*/bazel-out/|bazel-out/|'
}
+function call_install_name() {
+ /usr/bin/xcrun install_name_tool -change $(get_otool_path "$1") \
+ "@loader_path/$2/$3" "${OUTPUT}"
+}
+
# Do replacements in the output
for rpath in ${RPATHS}; do
for lib in ${LIBS}; do
@@ -108,10 +118,16 @@
if [[ -n "${libname-}" ]]; then
libpath=$(get_library_path ${lib})
if [ -n "${libpath}" ]; then
- /usr/bin/xcrun install_name_tool -change $(get_otool_path "${libpath}") \
- "@loader_path/${rpath}/${libname}" "${OUTPUT}"
+ call_install_name "${libpath}" "${rpath}" "${libname}"
+ fi
+ fi
+ done
+ for libpath in ${LIB_PATHS}; do
+ if [ -f "$libpath" ]; then
+ libname=$(basename "$libpath")
+ if [ -f "$(dirname ${OUTPUT})/${rpath}/${libname}" ]; then
+ call_install_name "${libpath}" "${rpath}" "${libname}"
fi
fi
done
done
-
diff --git a/tools/cpp/unix_cc_toolchain_config.bzl b/tools/cpp/unix_cc_toolchain_config.bzl
index 1a825f1..a7967b1 100644
--- a/tools/cpp/unix_cc_toolchain_config.bzl
+++ b/tools/cpp/unix_cc_toolchain_config.bzl
@@ -811,6 +811,24 @@
],
)
+ is_linux = ctx.attr.target_libc != "macosx"
+ if is_linux:
+ versioned_library_flag_group = flag_group(
+ flags = ["-l:%{libraries_to_link.name}"],
+ expand_if_equal = variable_with_value(
+ name = "libraries_to_link.type",
+ value = "versioned_dynamic_library",
+ ),
+ )
+ else:
+ versioned_library_flag_group = flag_group(
+ flags = ["%{libraries_to_link.path}"],
+ expand_if_equal = variable_with_value(
+ name = "libraries_to_link.type",
+ value = "versioned_dynamic_library",
+ ),
+ )
+
libraries_to_link_feature = feature(
name = "libraries_to_link",
flag_sets = [
@@ -868,13 +886,7 @@
value = "dynamic_library",
),
),
- flag_group(
- flags = ["-l:%{libraries_to_link.name}"],
- expand_if_equal = variable_with_value(
- name = "libraries_to_link.type",
- value = "versioned_dynamic_library",
- ),
- ),
+ versioned_library_flag_group,
flag_group(
flags = ["-Wl,-no-whole-archive"],
expand_if_true = "libraries_to_link.is_whole_archive",
@@ -1283,7 +1295,6 @@
],
)
- is_linux = ctx.attr.target_libc != "macosx"
libtool_feature = feature(
name = "libtool",
enabled = not is_linux,
diff --git a/tools/osx/crosstool/cc_toolchain_config.bzl b/tools/osx/crosstool/cc_toolchain_config.bzl
index a27068f..bded91d 100644
--- a/tools/osx/crosstool/cc_toolchain_config.bzl
+++ b/tools/osx/crosstool/cc_toolchain_config.bzl
@@ -1478,11 +1478,11 @@
flag_group(
flag_groups = [
flag_group(
- flags = ["-l:%{libraries_to_link.name}"],
+ flags = ["%{libraries_to_link.path}"],
expand_if_false = "libraries_to_link.is_whole_archive",
),
flag_group(
- flags = ["-Wl,-force_load,-l:%{libraries_to_link.name}"],
+ flags = ["-Wl,-force_load,%{libraries_to_link.path}"],
expand_if_true = "libraries_to_link.is_whole_archive",
),
],
