Transitive shared libraries not added to rpath of otherwise static binaries

[cbarrete]

This is sort of a follow up to https://github.com/facebook/buck2/issues/571.

When building a target with link_style = "static", where a transitive dependency is a shared library due to its usage of preferred_linkage = "shared", this shared library does not get added to the rpath of the top-level target. In other words, given binary -> static.a -> shared.so, shared.so will not be available to binary.

Here is a minimal reproduction on top of a buck2 init --git with the latest release:

diff --git a/BUCK b/BUCK
index 1cb6b38..94b9ee5 100644
--- a/BUCK
+++ b/BUCK
@@ -1,7 +1,20 @@
-# A list of available rules and their signatures can be found here: https://buck2.build/docs/api/rules/
+cxx_binary(
+    name = "test",
+    srcs = ["main.cpp"],
+    deps = [":static_lib"],
+    link_style = "static",
+)
+
+cxx_library(
+    name = "static_lib",
+    srcs = ["static_lib.cpp"],
+    deps = [":shared_lib"],
+)

-genrule(
-    name = "hello_world",
-    out = "out.txt",
-    cmd = "echo BUILT BY BUCK2> $OUT",
+cxx_library(
+    name = "shared_lib",
+    srcs = ["shared_lib.cpp"],
+    preferred_linkage = "shared",
 )
+
diff --git a/main.cpp b/main.cpp
new file mode 100644
index 0000000..01d671a
--- /dev/null
+++ b/main.cpp
@@ -0,0 +1,6 @@
+void static_lib_fn();
+
+int main()
+{
+    static_lib_fn();
+}
diff --git a/shared_lib.cpp b/shared_lib.cpp
new file mode 100644
index 0000000..6451836
--- /dev/null
+++ b/shared_lib.cpp
@@ -0,0 +1,3 @@
+void dynamic_lib_fn()
+{
+}
diff --git a/static_lib.cpp b/static_lib.cpp
new file mode 100644
index 0000000..39e88ae
--- /dev/null
+++ b/static_lib.cpp
@@ -0,0 +1,6 @@
+void dynamic_lib_fn();
+
+void static_lib_fn()
+{
+    dynamic_lib_fn();
+}

Running readelf -d $(buck2 build --show-full-simple-output :test) shows that the resulting binary has no rpath because link_style was set to static. As expected, buck2 run :test will fail due to the shared library not being found.

Is this just a current limitation of the prelude, or is there something fundamentally wrong with my approach here?

[cbarrete]

Tagging @cjhopman since you already had the answers last time, I hope you don't mind.

[cbarrete]

I did some digging and it seems to come from

    # Only setup a shared library symlink tree when shared linkage or link_groups is used
    gnu_use_link_groups = cxx_is_gnu(ctx) and link_group_mappings
    shlib_deps = []
    if link_strategy == LinkStrategy("shared") or gnu_use_link_groups:
        shlib_deps = (
            [d.shared_library_info for d in link_deps] +
            [d.shared_library_info for d in impl_params.extra_link_roots]
        )

in cxx_executable (cxx/cxx_executable.bzl).

Removing the check seems to do what I want, but that might break something elsewhere? The alternative seems to be to use link groups but:

1. I don't know what those are and couldn't really find documentation about them
2. They seem to only be supported with GCC, which is a dealbreaker for us

Can I get some context regarding why this check was added? If it's some sort of optimization, I would suggest removing it, unless:

1. it was proven to be slowing things down in static builds
2. there's another way of handling the use-case for this issue

[cjhopman]

Personally I think it's just a bug and would love to see us fix it.

But, it's actually more that the behavior in the condition is wrong, but for the shared link case it's less wrong. Okay, what's going on? The d.shared_library_info there ends up being basically a list of all the shared libraries in the graph for every target that can produce a shared library (basically, every target with preferred_linkage = "any" or "shared" ends up with an entry in the shared_library_info).

When using link_style = "static" (actually if used anywhere in the graph, not just at the top level like that condition checks) that list of libraries might include a bunch that you don't actually use (because it would include ones from targets with preferred_linkage = any even though you consume those as shared libs).

The right way to do this is that we should be tracking up the actual shared lib deps of each shared lib, rather than just stuffing everything into shared_library_info and using that.

[pollend]

I'm running into this issue any suggestions to workaround this. I guess i have to modify prelude?

I ended up adding a bodge to prelude to get this working. I'm just manually passing in shared deps, not the correct solution but its a stopgap for the moment. hopefully something official could be done to address this problem.

diff --git a/cxx/cxx.bzl b/cxx/cxx.bzl
index a505763f..90a41274 100644
--- a/cxx/cxx.bzl
+++ b/cxx/cxx.bzl
@@ -63,6 +63,7 @@ load(
     "extract_soname_from_shlib",
     "merge_shared_libraries",
     "to_soname",
+    "traverse_shared_library_info"
 )
 load("@prelude//linking:strip.bzl", "strip_debug_info")
 load("@prelude//linking:types.bzl", "Linkage")
@@ -718,6 +719,12 @@ def cxx_test_impl(ctx: AnalysisContext) -> list[Provider]:
     params = CxxRuleConstructorParams(
         rule_type = "cxx_test",
         headers_layout = cxx_get_regular_cxx_headers_layout(ctx),
+        extra_shared_libs = traverse_shared_library_info(
+            merge_shared_libraries(
+                actions = ctx.actions,
+                deps = [dep.get(SharedLibraryInfo) for dep in ctx.attrs.extra_shared_deps],
+            ),
+        ),
         srcs = get_srcs_with_flags(ctx),
         link_group_info = link_group_info,
         auto_link_group_specs = get_auto_link_group_specs(ctx, link_group_info),
diff --git a/decls/cxx_rules.bzl b/decls/cxx_rules.bzl
index dc0a6f13..025b14b5 100644
--- a/decls/cxx_rules.bzl
+++ b/decls/cxx_rules.bzl
@@ -855,6 +855,7 @@ cxx_test = prelude_rule(
         native_common.link_group_deps() |
         native_common.link_group_public_deps_label() |
         {
+            "extra_shared_deps": attrs.list(attrs.dep(), default = []),
             "additional_coverage_targets": attrs.list(attrs.source(), default = []),
             "contacts": attrs.list(attrs.string(), default = []),
             "cxx_runtime_type": attrs.option(attrs.enum(CxxRuntimeType), default = None),