From 65e4f0e75b5a74e7b7025db736b3621de5b45e94 Mon Sep 17 00:00:00 2001 From: Timothy Gu Date: Sat, 17 Apr 2021 14:15:04 -0700 Subject: [PATCH] Assume cgo dynamically linked libraries exist in runfiles Previously, we assumed that any dynamically linked libraries are in execroot. This may not be the case if the library is NOT generated. However, the library should always be in runfiles. Fixes #2867 --- .bazelci/presubmit.yml | 12 +++- .gitignore | 1 + go/private/actions/link.bzl | 3 +- go/private/rpath.bzl | 40 ++++++++---- go/private/rules/binary.bzl | 2 +- tests/core/cgo/BUILD.bazel | 79 ++++++++++++++++++++--- tests/core/cgo/README.rst | 17 +++-- tests/core/cgo/generate_imported_dylib.sh | 18 ++++++ 8 files changed, 141 insertions(+), 31 deletions(-) create mode 100755 tests/core/cgo/generate_imported_dylib.sh diff --git a/.bazelci/presubmit.yml b/.bazelci/presubmit.yml index b182cfacae..5ddfcde1b8 100644 --- a/.bazelci/presubmit.yml +++ b/.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" diff --git a/.gitignore b/.gitignore index a6ef824c1f..c98766f173 100644 --- a/.gitignore +++ b/.gitignore @@ -1 +1,2 @@ /bazel-* +/tests/core/cgo/libimported.* diff --git a/go/private/actions/link.bzl b/go/private/actions/link.bzl index 1fd472aee8..5a44dc0809 100644 --- a/go/private/actions/link.bzl +++ b/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) diff --git a/go/private/rpath.bzl b/go/private/rpath.bzl index b5b7ad46ee..53435e39b9 100644 --- a/go/private/rpath.bzl +++ b/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, ) diff --git a/go/private/rules/binary.bzl b/go/private/rules/binary.bzl index c7fed7fe23..525169d272 100644 --- a/go/private/rules/binary.bzl +++ b/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, diff --git a/tests/core/cgo/BUILD.bazel b/tests/core/cgo/BUILD.bazel index fd8dfec486..d205044cc3 100644 --- a/tests/core/cgo/BUILD.bazel +++ b/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"], diff --git a/tests/core/cgo/README.rst b/tests/core/cgo/README.rst index 34e971635a..a385cab383 100644 --- a/tests/core/cgo/README.rst +++ b/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 ------------ diff --git a/tests/core/cgo/generate_imported_dylib.sh b/tests/core/cgo/generate_imported_dylib.sh new file mode 100755 index 0000000000..5c272ef444 --- /dev/null +++ b/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