Skipping linker flag for versioned dylib on Darwin

.bazelci/presubmit.yml

@@ -3,12 +3,16 @@ tasks:
   ubuntu1804_bazel400:
     platform: ubuntu1804
     bazel: 4.2.0 # test minimum supported version of bazel
+    shell_commands:
+      - tests/core/cgo/generate_imported_dylib.sh
     build_targets:
     - "//..."
     test_targets:
     - "//..."
   ubuntu2004:
     # enable some unflipped incompatible flags on this platform to ensure we don't regress.
+    shell_commands:
+      - tests/core/cgo/generate_imported_dylib.sh
     build_flags:
     - "--incompatible_load_proto_rules_from_bzl"
     - "--incompatible_enable_cc_toolchain_resolution"
@@ -20,11 +24,15 @@ tasks:
     test_targets:
     - "//..."
   macos:
+    shell_commands:
+      - tests/core/cgo/generate_imported_dylib.sh
     build_targets:
     - "//..."
     test_targets:
     - "//..."
   rbe_ubuntu1604:
+    shell_commands:
+      - tests/core/cgo/generate_imported_dylib.sh
     build_targets:
     - "//..."
     test_flags:

.gitignore

@@ -1,2 +1,3 @@
 /bazel-*
 /tests/core/cgo/libimported.*
+/tests/core/cgo/libversioned.*

go/private/common.bzl

@@ -173,31 +173,31 @@ def has_shared_lib_extension(path):
     Matches filenames of shared libraries, with or without a version number extension.
     """
     return (has_simple_shared_lib_extension(path) or
-            has_versioned_shared_lib_extension(path))
+            get_versioned_shared_lib_extension(path))
 
 def has_simple_shared_lib_extension(path):
     """
     Matches filenames of shared libraries, without a version number extension.
     """
-    if any([path.endswith(ext) for ext in SHARED_LIB_EXTENSIONS]):
-        return True
-    return False
+    return any([path.endswith(ext) for ext in SHARED_LIB_EXTENSIONS])
 
-def has_versioned_shared_lib_extension(path):
-    """Returns whether the path appears to be an versioned .so or .dylib file."""
+def get_versioned_shared_lib_extension(path):
+    """If appears to be an versioned .so or .dylib file, return the extension; otherwise empty"""
     parts = path.split("/")[-1].split(".")
     if not parts[-1].isdigit():
-        return False
+        return ""
+
+    # only iterating to 1 because parts[0] has to be the lib name
     for i in range(len(parts) - 1, 0, -1):
         if not parts[i].isdigit():
             if parts[i] == "dylib" or parts[i] == "so":
-                return True
+                return ".".join(parts[i:])
 
-            # somehting like foo.bar.1.2
-            return False
+            # something like foo.bar.1.2 or dylib.1.2
+            return ""
 
     # something like 1.2.3, or so.1.2, or dylib.1.2, or foo.1.2
-    return False
+    return ""
 
 MINIMUM_BAZEL_VERSION = "4.2.0"
 

go/private/rules/cgo.bzl

@@ -14,8 +14,8 @@
 
 load(
     "//go/private:common.bzl",
+    "get_versioned_shared_lib_extension",
     "has_simple_shared_lib_extension",
-    "has_versioned_shared_lib_extension",
     "hdr_exts",
 )
 load(
@@ -119,25 +119,32 @@ def cgo_configure(go, srcs, cdeps, cppopts, copts, cxxopts, clinkopts):
                 # If both static and dynamic variants are available, Bazel will only give
                 # us the static variant. We'll get one file for each transitive dependency,
                 # so the same file may appear more than once.
-                if (lib.basename.startswith("lib") and
-                    has_simple_shared_lib_extension(lib.basename)):
-                    # If the loader would be able to find the library using rpaths,
-                    # use -L and -l instead of hard coding the path to the library in
-                    # the binary. This gives users more flexibility. The linker will add
-                    # rpaths later. We can't add them here because they are relative to
-                    # the binary location, and we don't know where that is.
-                    libname = lib.basename[len("lib"):lib.basename.rindex(".")]
-                    clinkopts.extend(["-L", lib.dirname, "-l", libname])
-                    inputs_direct.append(lib)
-                elif (lib.basename.startswith("lib") and
-                      has_versioned_shared_lib_extension(lib.basename)):
-                    # With a versioned shared library, we must use the full filename,
-                    # otherwise the library will not be found by the linker.
-                    libname = ":%s" % lib.basename
-                    clinkopts.extend(["-L", lib.dirname, "-l", libname])
-                    inputs_direct.append(lib)
-                else:
-                    lib_opts.append(lib.path)
+                if lib.basename.startswith("lib"):
+                    if has_simple_shared_lib_extension(lib.basename):
+                        # If the loader would be able to find the library using rpaths,
+                        # use -L and -l instead of hard coding the path to the library in
+                        # the binary. This gives users more flexibility. The linker will add
+                        # rpaths later. We can't add them here because they are relative to
+                        # the binary location, and we don't know where that is.
+                        libname = lib.basename[len("lib"):lib.basename.rindex(".")]
+                        clinkopts.extend(["-L", lib.dirname, "-l", libname])
+                        inputs_direct.append(lib)
+                        continue
+                    extension = get_versioned_shared_lib_extension(lib.basename)
+                    if extension.startswith("so"):
+                        # With a versioned .so file, we must use the full filename,
+                        # otherwise the library will not be found by the linker.
+                        libname = ":%s" % lib.basename
+                        clinkopts.extend(["-L", lib.dirname, "-l", libname])
+                        inputs_direct.append(lib)
+                        continue
+                    elif extension.startswith("dylib"):
+                        # A standard versioned dylib is named as libMagick.2.dylib, which is
+                        # treated as a simple shared library. Non-standard versioned dylibs such as
+                        # libclntsh.dylib.12.1, users have to create a unversioned symbolic link,
+                        # so it can be treated as a simple shared library too.
+                        continue
+                lib_opts.append(lib.path)
             clinkopts.extend(cc_link_flags)
 
         elif hasattr(d, "objc"):

tests/core/cgo/BUILD.bazel

@@ -151,22 +151,24 @@ cc_binary(
     tags = ["manual"],
 )
 
+# //tests/core/cgo:generate_imported_dylib.sh must be run first
 go_test(
     name = "versioned_dylib_test",
     srcs = ["dylib_test.go"],
     embed = [":versioned_dylib_client"],
     rundir = ".",
-    tags = ["manual"],  # //tests/core/cgo:generate_imported_dylib.sh must be run first
+    target_compatible_with = select({
+        "@platforms//os:osx": [],
+        "@platforms//os:linux": [],
+        "//conditions:default": ["@platforms//:incompatible"],
+    }),
 )
 
 go_library(
     name = "versioned_dylib_client",
     srcs = ["dylib_client.go"],
     cdeps = select({
-        # This test exists just for versioned `.so`s on Linux,
-        # but we can reuse the above test's dylib so it passes on darwin,
-        # where filename suffixes are not used for library version.
-        "@io_bazel_rules_go//go/platform:darwin": [":darwin_imported_dylib"],
+        "@io_bazel_rules_go//go/platform:darwin": [":darwin_imported_versioned_dylib"],
         "//conditions:default": [":linux_imported_versioned_dylib"],
         # TODO(jayconrod): Support windows, skip others.
     }),
@@ -177,10 +179,43 @@ go_library(
 
 cc_import(
     name = "linux_imported_versioned_dylib",
-    shared_library = "libimported.so.2",
+    shared_library = "libversioned.so.2",
     tags = ["manual"],
 )
 
+cc_import(
+    name = "darwin_imported_versioned_dylib",
+    shared_library = "libversioned.2.dylib",
+    tags = ["manual"],
+)
+
+go_test(
+    name = "oracle_convention_darwin_dylib_test",
+    srcs = ["dylib_test.go"],
+    embed = [":oracle_convention_darwin_dylib_client"],
+    rundir = ".",
+    target_compatible_with = ["@platforms//os:macos"],
+)
+
+go_library(
+    name = "oracle_convention_darwin_dylib_client",
+    srcs = ["dylib_client.go"],
+    cdeps = [":oracle_convention_darwin_dylib"],
+    cgo = True,
+    importpath = "github.com/bazelbuild/rules_go/tests/core/cgo/dylib",
+    target_compatible_with = ["@platforms//os:macos"],
+)
+
+# //tests/core/cgo:generate_imported_dylib.sh must be run first
+cc_library(
+    name = "oracle_convention_darwin_dylib",
+    srcs = [
+        "libversioned.dylib",
+        "libversioned.dylib.2",
+    ],
+    target_compatible_with = ["@platforms//os:macos"],
+)
+
 go_test(
     name = "generated_versioned_dylib_test",
     srcs = ["dylib_test.go"],

tests/core/cgo/generate_imported_dylib.sh

@@ -7,10 +7,18 @@ cd "$(dirname "$0")"
 case "$(uname -s)" in
   Linux*)
     cc -shared -o libimported.so imported.c
-    cc -shared -o libimported.so.2 imported.c
+    cc -shared -o libversioned.so.2 imported.c
     ;;
   Darwin*)
     cc -shared -Wl,-install_name,@rpath/libimported.dylib -o libimported.dylib imported.c
+    # According to "Mac OS X For Unix Geeks", 4th Edition, Chapter 11, versioned dylib for macOS
+    # should be libversioned.2.dylib.
+    cc -shared -Wl,-install_name,@rpath/libversioned.2.dylib -o libversioned.2.dylib imported.c
+    # However, Oracle Instant Client was distributed as libclntsh.dylib.12.1 with a unversioed
+    # symlink (https://www.oracle.com/database/technologies/instant-client/macos-intel-x86-downloads.html).
+    # Let's cover this non-standard case as well.
+    cc -shared -Wl,-install_name,@rpath/libversioned.dylib.2 -o libversioned.dylib.2 imported.c
+    ln -s libversioned.dylib.2 libversioned.dylib
     ;;
   *)
     echo "Unsupported OS: $(uname -s)" >&2