Fix versioned shared libraries for macOS toolchain

src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainVariables.java

@@ -671,6 +671,7 @@ public StructureValue build() {
   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 @@ public static LibraryToLinkValue forDynamicLibrary(String name) {
       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 @@ protected String getTypeName() {
     }
 
     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

src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java

@@ -822,7 +822,7 @@ public static ImmutableList<CToolchain.Feature> getLegacyFeatures(
                             "      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: {",

src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java

@@ -613,7 +613,7 @@ private void addDynamicInputLinkOptions(
       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

src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/BUILD.builtin_test

@@ -115,6 +115,7 @@ cc_shared_library(
         "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 @@ cc_library(
     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 @@ cc_shared_library(
     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 = [

src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/starlark_tests.bzl

@@ -166,6 +166,7 @@ def _runfiles_test_impl(ctx):
         "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",

src/test/java/com/google/devtools/build/lib/packages/util/mock/osx_cc_toolchain_config.bzl

@@ -7540,11 +7540,11 @@ def _impl(ctx):
                             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",
                                     ),
                                 ],

src/test/java/com/google/devtools/build/lib/rules/cpp/LibraryToLinkValueTest.java

@@ -47,9 +47,9 @@ public void equalsAndHashCode() {
 
     // #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,14 +135,22 @@ public void getFieldValue_forDynamicLibrary() {
 
   @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",
                 /*field=*/ "type",
                 /*expander=*/ null,
                 /*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",

tools/cpp/osx_cc_wrapper.sh.tpl

@@ -28,6 +28,7 @@
 set -eu
 
 LIBS=
+LIB_PATHS=
 LIB_DIRS=
 RPATHS=
 OUTPUT=
@@ -38,6 +39,10 @@ function parse_option() {
         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 @@ function get_otool_path() {
     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 @@ for rpath in ${RPATHS}; do
         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
-

tools/cpp/unix_cc_toolchain_config.bzl

@@ -811,6 +811,24 @@ def _impl(ctx):
         ],
     )
 
+    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 @@ def _impl(ctx):
                                     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 @@ def _impl(ctx):
         ],
     )
 
-    is_linux = ctx.attr.target_libc != "macosx"
     libtool_feature = feature(
         name = "libtool",
         enabled = not is_linux,

tools/osx/crosstool/cc_toolchain_config.bzl

@@ -1478,11 +1478,11 @@ def _impl(ctx):
                             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",
                                     ),
                                 ],