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

Do not pass --Clink-arg=-l for libstd and libtest #1500

Merged
merged 7 commits into from
Aug 5, 2022

Conversation

scentini
Copy link
Collaborator

@scentini scentini commented Aug 5, 2022

Fixes #1374

We skip adding -Clink-arg=-l... for libstd and libtest from the standard library, as these two libraries are present both as an .rlib and a .so format. On linux, Rustc adds a -Bdynamic to the linker command line before the libraries specified with -Clink-arg, which leads to us linking against the .sos but not putting the corresponding value to the runtime library search paths, which results in a "cannot open shared object file: No such file or directory" error at exectuion time. We can fix this by adding a -Clink-arg=-Bstatic on linux, but we don't have that option for macos. The proper solution for this issue would be to remove libtest-{hash}.so and libstd-{hash}.so from the toolchain. However, it is not enough to change the toolchain's rust_std_{...} filegroups here:

name = "rust_std-{target_triple}",
because rustc manages to escape the sandbox and still finds them at linking time. We need to modify the repository rules to erase those files completely. This PR should be a good workaround until we get do to the proper thing though.

This PR also fixes the following issues for Windows:

  • get_lib_name() didn't work properly on windows for libc
  • -Clink-arg should point to the library name with extension included for windows.

@scentini scentini changed the title Fix #1374 Do not pass --Clink-arg=-l for libstd and libtest Aug 5, 2022
@scentini scentini marked this pull request as ready for review August 5, 2022 12:44
Copy link
Collaborator

@krasimirgg krasimirgg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you!

@scentini scentini enabled auto-merge (squash) August 5, 2022 13:13
@scentini scentini merged commit 4e5fac5 into bazelbuild:main Aug 5, 2022
@scentini scentini deleted the libstd_libtest branch August 5, 2022 13:20
scentini added a commit that referenced this pull request Aug 9, 2022
#1500 added an additional `for_windows` parameter to `get_lib_name`. I missed the fact that we also pass that function to `map_each` here: https://github.com/bazelbuild/rules_rust/blob/main/rust/private/rustc.bzl#L1671
and as such, this code does not always work correctly (we don't get to pass the `for_windows` parameter, and internally at Google it ended up evaluating to `True` on Linux builds).

I tried to avoid flattening the `cc_toolchain.dynamic_runtime_lib` and `cc_toolchain.static_runtime_lib` depsets by using a lambda:
```
args.add_all(
    cc_toolchain.dynamic_runtime_lib(feature_configuration = feature_configuration),
    map_each = lambda x: get_lib_name(x, for_windows = toolchain.os.startswith("windows)),
    format_each = "-ldylib=%s",
)
```

However it looks like such usage of lambdas is not allowed:
```
Error in add_all: to avoid unintended retention of analysis data structures,
the map_each function (declared at ...) must be declared by a top-level def statement
```

So instead of `get_lib_name` we now have `get_lib_name_default` and `get_lib_name_for_windows`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible regression causing linking of proc_macro
2 participants