Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

linking a rust_binary that depends on two cc_libraries with the same name fails #840

Open
krasimirgg opened this issue Jul 16, 2021 · 1 comment · Fixed by #841
Open
Assignees

Comments

@krasimirgg
Copy link
Collaborator

krasimirgg commented Jul 16, 2021

This depends on #825. In particular, trying out this example without #825 will run into the linker errors described there and this example assumes using at least the beta rustc. I'm using Linux with ld.

Consider a case where a rust_binary (transitively) depends on two cc_libraryes that have the same name. In the wild targets with common names like utils, or status could occur in unrelated packages.

Like in this example where //same_names:rbin depends on //same_names/x:exc and //same_names/y:exc:

same_names
├── BUILD
├── rbin.rs
├── x
│   ├── BUILD
│   └── exc.cc
└── y
    ├── BUILD
    └── exc.cc
# same_names/BUILD
load("@rules_rust//rust:defs.bzl", "rust_binary")

rust_binary(
    name = "rbin",
    srcs = ["rbin.rs"],
    deps = [
        "//same_names/x:exc",
        "//same_names/y:exc",
    ],
)
// same_names/rbin.rs
use std::os::raw::c_int;

extern "C" {
    pub fn cx() -> c_int;
    pub fn cy() -> c_int;
}

fn main() {
    println!("hi {} {}", unsafe { cx() }, unsafe { cy() });
}
# same_names/x/BUILD
package(default_visibility = ["//visibility:public"])

cc_library(
    name = "exc",
    srcs = ["exc.cc"],
)
// same_names/x/exc.cc
extern "C" int cx() {
  return 17;
}
# same_names/y/BUILD
package(default_visibility = ["//visibility:public"])

cc_library(
    name = "exc",
    srcs = ["exc.cc"],
)
// same_names/y/exc.cc
extern "C" int cy() {
  return 113;
}

Running bazel build //same_names:rbin fails with a linker error like:

bin::main: error: undefined reference to 'cy'

The problem is that the way we construct linker command line arguments for native dependencies doesn't work when they have the same name. For each transitive native dependency, we pass its package directory as -Lnative= and its name as -lstatic= to rustc. These get translated to -Lpackage and -lname linker arguments. In the example above, the rustc arguments are like:

...
 '-Lnative=bazel-out/k8-fastbuild/bin/same_names/x'
 '-Lnative=bazel-out/k8-fastbuild/bin/same_names/y' 
 '-lstatic=exc' 
 '-lstatic=exc'
...

and get translated as linker arguments like:

...
 '-Lbazel-out/k8-fastbuild/bin/same_names/x'
 '-Lbazel-out/k8-fastbuild/bin/same_names/y' 
 '-lexc' 
 '-lexc'
...

Now the problem is that the linker always picks up the first libexc.a library that it finds on the path, in this case same_names/x/libexc.a, and hence the undefined reference errors for the symbol defined in same_names/y/libexc.a.

The way cc rules deal with this is that they just pass the full path to the library to the linker, AFAIK. I think this is a good way to address this and potentially some other naming incompatibilities across cc and rust rules.

Under the native-link-modifiers RFC, the relative order of -l and -Clink-arg(s) is preserved, so it should be OK to replace the current linker args pattern:

...
 '-Lnative=bazel-out/k8-fastbuild/bin/same_names/x'
 '-Lnative=bazel-out/k8-fastbuild/bin/same_names/y' 
 '-lstatic=exc' 
 '-lstatic=exc'
...

with something like:

...
 '-Clink-arg=bazel-out/k8-fastbuild/bin/same_names/x/libexc.a'
 '-Clink-arg=bazel-out/k8-fastbuild/bin/same_names/y/libexc.a'
...
@krasimirgg
Copy link
Collaborator Author

We've got some issues with the approach from #841, rooted in that the relative order of linker args derived from rustc -l and -Clink-arg(s) is not preserved, e.g., #841 (comment).

Longer term we hope that the native_link_modifiers rustc RFC rust-lang/rust#81490 will give us the necessary options to preserve the relative order.

For now the plan is to logically revert #841, so rules_rust goes back to use -lstatic for native dependencies. As a follow-up, we plan to address the issues with ambiguous native libraries (this issue) by constructing disambiguating symlinks. I'll hold up until #1134 is merged since the disambiguating symlinks feature touches much of the same code.

@krasimirgg krasimirgg reopened this Feb 18, 2022
krasimirgg added a commit to krasimirgg/rules_rust that referenced this issue Feb 21, 2022
This reverts rules_rust to use `-Lnative/-lstatic` instead of library paths
per
bazelbuild#840 (comment).
krasimirgg added a commit to krasimirgg/rules_rust that referenced this issue Feb 21, 2022
krasimirgg added a commit to krasimirgg/rules_rust that referenced this issue Feb 21, 2022
Depends on bazelbuild#1147.

This addresses bazelbuild#840 by
creating symlinks to ambiguous native dependencies of a rust_binary that
would otherwise cause the link to fail, as in the examples added.
mwu-tow pushed a commit to enso-org/ci-build that referenced this issue May 11, 2022
[Linking a rust_binary that depends on two cc_libraries with the same name fails](bazelbuild/rules_rust#840). This bug is breaking cross-compiles of the `enso-cloud` crates.

Cause:
1. The `enso-cloud-deploy` crate depends on the `ide-ci` crate.
2. The `ide-ci` crate shares a workspace with `enso-build`.
3. Both `ide-ci` and `enso-build` depend on `octocrab` (this crate).
4. `octocrab` uses `reqwest` with default features enabled.
5. `reqwest`'s default features provide TLS through the `native-tls` crate.
6. The `native-tls` crate depends on `openssl`, which depends on the `libcrypto` and `libssl` native libraries.
7. Depending on `libcrypto`/`libssl` twice breaks the build.

This PR switches `octocrab` to using `reqwest` with `rustls` instead of `native-tls`. This removes the `openssl` dependency entirely.

Closes https://www.pivotaltracker.com/story/show/182135510.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant