Fix cgo rpath computation for prebuilt dynamic libraries

.bazelci/presubmit.yml

@@ -154,8 +154,12 @@ platforms:
     - "-@test_chdir_remote//sub:go_default_test"
     - "-//tests/core/cgo:dylib_client"
     - "-//tests/core/cgo:dylib_test"
+    - "-//tests/core/cgo:generated_dylib_client"
+    - "-//tests/core/cgo:generated_dylib_test"
     - "-//tests/core/cgo:versioned_dylib_client"
     - "-//tests/core/cgo:versioned_dylib_test"
+    - "-//tests/core/cgo:generated_versioned_dylib_client"
+    - "-//tests/core/cgo:generated_versioned_dylib_test"
     - "-//tests/core/cross:proto_test"
     - "-//tests/core/go_embed_data:go_default_test"
     - "-//tests/core/go_embed_data:go_default_library"
@@ -264,10 +268,14 @@ platforms:
     - "-@org_golang_x_tools//cmd/splitdwarf/internal/macho:macho_test"
     - "-@test_chdir_remote//sub:go_default_test"
     - "-//tests:buildifier_test" # requires bash
-    - "-//tests/core/cgo:dylib_test"
     - "-//tests/core/cgo:dylib_client"
-    - "-//tests/core/cgo:versioned_dylib_test"
+    - "-//tests/core/cgo:dylib_test"
+    - "-//tests/core/cgo:generated_dylib_client"
+    - "-//tests/core/cgo:generated_dylib_test"
     - "-//tests/core/cgo:versioned_dylib_client"
+    - "-//tests/core/cgo:versioned_dylib_test"
+    - "-//tests/core/cgo:generated_versioned_dylib_client"
+    - "-//tests/core/cgo:generated_versioned_dylib_test"
     - "-//tests/core/coverage:coverage_test"
     - "-//tests/core/cross:proto_test"
     - "-//tests/core/go_binary:go_default_test"

.gitignore

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

go/private/actions/link.bzl

@@ -128,9 +128,10 @@ def emit_link(
     # the bazel execroot. Most binaries are only dynamically linked against
     # system libraries though.
     cgo_rpaths = sorted(collections.uniq([
-        rpath.flag(go, d, relative_to = executable)
+        f
         for d in archive.cgo_deps.to_list()
         if has_shared_lib_extension(d.basename)
+        for f in rpath.flags(go, d, executable = executable)
     ]))
     extldflags.extend(cgo_rpaths)
 

go/private/rpath.bzl

@@ -17,29 +17,43 @@ load(
     "paths",
 )
 
-def _rpath(go, library, relative_to = None):
-    """Returns the rpath of a library, possibly relative to another file."""
-    if not relative_to:
-        return paths.dirname(library.short_path)
+def _rpath(go, library, executable = None):
+    """Returns the potential rpaths of a library, possibly relative to another file."""
+    if not executable:
+        return [paths.dirname(library.short_path)]
 
-    # Go back to the workspace root from the executable file
-    depth = relative_to.short_path.count("/")
-    back_to_root = paths.join(*([".."] * depth))
     origin = go.mode.goos == "darwin" and "@loader_path" or "$ORIGIN"
 
-    # Then walk back to the library's short path
-    return paths.join(origin, back_to_root, paths.dirname(library.short_path))
+    # Accomodate for two kinds of executable paths.
+    rpaths = []
+
+    # 1. Where the executable is inside its own .runfiles directory.
+    #  This is the case for generated libraries as well as remote builds.
+    #   a) go back to the workspace root from the executable file in .runfiles
+    depth = executable.short_path.count("/")
+    back_to_root = paths.join(*([".."] * depth))
+
+    #   b) then walk back to the library's short path
+    rpaths.append(paths.join(origin, back_to_root, paths.dirname(library.short_path)))
+
+    # 2. Where the executable is outside the .runfiles directory:
+    #  This is the case for local pre-built libraries, as well as local
+    #  generated libraries.
+    runfiles_dir = paths.basename(executable.short_path) + ".runfiles"
+    rpaths.append(paths.join(origin, runfiles_dir, go._ctx.workspace_name, paths.dirname(library.short_path)))
+
+    return rpaths
 
-def _flag(go, *args, **kwargs):
-    """Returns the linker flag rpath for a library."""
-    return "-Wl,-rpath," + _rpath(go, *args, **kwargs)
+def _flags(go, *args, **kwargs):
+    """Returns the rpath linker flags for a library."""
+    return ["-Wl,-rpath," + p for p in _rpath(go, *args, **kwargs)]
 
 def _install_name(f):
     """Returns the install name for a dylib on macOS."""
     return f.short_path
 
 rpath = struct(
-    flag = _flag,
+    flags = _flags,
     install_name = _install_name,
     rpath = _rpath,
 )

go/private/rules/binary.bzl

@@ -55,7 +55,7 @@ def new_cc_import(
         alwayslink = False,
         linkopts = []):
     if dynamic_library:
-        linkopts = linkopts + [rpath.flag(go, dynamic_library)]
+        linkopts = linkopts + rpath.flags(go, dynamic_library)
     return CcInfo(
         compilation_context = cc_common.create_compilation_context(
             defines = defines,

tests/core/cgo/BUILD.bazel

@@ -73,6 +73,7 @@ go_test(
     srcs = ["dylib_test.go"],
     embed = [":dylib_client"],
     rundir = ".",
+    tags = ["manual"], # //tests/core/cgo:generate_imported_dylib.sh must be run first
 )
 
 go_library(
@@ -85,6 +86,7 @@ go_library(
     }),
     cgo = True,
     importpath = "github.com/bazelbuild/rules_go/tests/core/cgo/dylib",
+    tags = ["manual"],
 )
 
 cc_import(
@@ -93,22 +95,53 @@ cc_import(
     tags = ["manual"],
 )
 
+cc_import(
+    name = "linux_imported_dylib",
+    shared_library = ":libimported.so",
+    tags = ["manual"],
+)
+
+go_test(
+    name = "generated_dylib_test",
+    srcs = ["dylib_test.go"],
+    embed = [":generated_dylib_client"],
+    rundir = ".",
+)
+
+go_library(
+    name = "generated_dylib_client",
+    srcs = ["dylib_client.go"],
+    cdeps = select({
+        "@io_bazel_rules_go//go/platform:darwin": [":darwin_imported_generated_dylib"],
+        "//conditions:default": [":linux_imported_generated_dylib"],
+        # TODO(jayconrod): Support windows, skip others.
+    }),
+    cgo = True,
+    importpath = "github.com/bazelbuild/rules_go/tests/core/cgo/dylib",
+)
+
+cc_import(
+    name = "darwin_imported_generated_dylib",
+    shared_library = ":libimported_generated.dylib",
+    tags = ["manual"],
+)
+
 cc_binary(
-    name = "libimported.dylib",
+    name = "libimported_generated.dylib",
     srcs = ["imported.c"],
-    linkopts = ["-Wl,-install_name,@rpath/libimported.dylib"],
+    linkopts = ["-Wl,-install_name,@rpath/libimported_generated.dylib"],
     linkshared = True,
     tags = ["manual"],
 )
 
 cc_import(
-    name = "linux_imported_dylib",
-    shared_library = ":libimported.so",
+    name = "linux_imported_generated_dylib",
+    shared_library = ":libimported_generated.so",
     tags = ["manual"],
 )
 
 cc_binary(
-    name = "libimported.so",
+    name = "libimported_generated.so",
     srcs = ["imported.c"],
     linkshared = True,
     tags = ["manual"],
@@ -119,6 +152,7 @@ go_test(
     srcs = ["dylib_test.go"],
     embed = [":versioned_dylib_client"],
     rundir = ".",
+    tags = ["manual"], # //tests/core/cgo:generate_imported_dylib.sh must be run first
 )
 
 go_library(
@@ -134,17 +168,46 @@ go_library(
     }),
     cgo = True,
     importpath = "github.com/bazelbuild/rules_go/tests/core/cgo/dylib",
+    tags = ["manual"],
 )
 
-cc_library(
+cc_import(
     name = "linux_imported_versioned_dylib",
-    srcs = [":libimported.so.2"],
+    shared_library = "libimported.so.2",
+    tags = ["manual"],
+)
+
+go_test(
+    name = "generated_versioned_dylib_test",
+    srcs = ["dylib_test.go"],
+    embed = [":generated_versioned_dylib_client"],
+    rundir = ".",
+)
+
+go_library(
+    name = "generated_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_generated_dylib"],
+        "//conditions:default": [":linux_imported_generated_versioned_dylib"],
+        # TODO(jayconrod): Support windows, skip others.
+    }),
+    cgo = True,
+    importpath = "github.com/bazelbuild/rules_go/tests/core/cgo/dylib",
+)
+
+cc_library(
+    name = "linux_imported_generated_versioned_dylib",
+    srcs = [":libimported_generated.so.2"],
     linkstatic = False,
     tags = ["manual"],
 )
 
 cc_binary(
-    name = "libimported.so.2",
+    name = "libimported_generated.so.2",
     srcs = ["imported.c"],
     linkshared = True,
     tags = ["manual"],

tests/core/cgo/README.rst

@@ -10,18 +10,23 @@ opts_test
 Checks that different sets of options are passed to C and C++ sources in a
 ``go_library`` with ``cgo = True``.
 
-dylib_test
-----------
+(generated_)?(versioned_)?dylib_test
+------------------------------------
 
 Checks that Go binaries can link against dynamic C libraries. Some libraries
 (especially those provided with ``cc_import``) may only have dynamic versions,
 and we should be able to link against them and find them at run-time.
 
-dylib_test
-----------
+The non ``generated_`` tests are manual. The ``generate_imported_dylib.sh``
+script must be run before running the tests themselves.
 
-Checks that Go binaries can link against dynamic C libraries that are only
-available as a versioned shared library, like ``libfoo.so.1``.
+The ``generated_`` variants check that Go binaries can link against dynamic C
+libraries that are generated by another rule, rather than being included in the
+source tree.
+
+The ``versioned_`` variants check that Go binaries can link against dynamic C
+libraries that are only available as a versioned shared library, like
+``libfoo.so.1``, as used on Linux.
 
 cc_libs_test
 ------------

tests/core/cgo/generate_imported_dylib.sh

@@ -0,0 +1,18 @@
+#!/usr/bin/env bash
+
+set -exo pipefail
+
+cd "$(dirname "$0")"
+
+case "$(uname -s)" in
+  Linux*)
+    cc -shared -o libimported.so imported.c
+    cc -shared -o libimported.so.2 imported.c
+    ;;
+  Darwin*)
+    cc -shared -Wl,-install_name,@rpath/libimported.dylib -o libimported.dylib imported.c
+    ;;
+  *)
+    echo "Unsupported OS: $(uname -s)" >&2
+    exit 1
+esac