Propagate linkopts from native deps #264
Conversation
hlopko
requested review from
acmcarther,
damienmg and
mfarrugi
as code owners
| @@ -114,10 +116,20 @@ def collect_deps(deps, toolchain): | |||
|
|
|||
| # TODO: We could let the user choose how to link, instead of always preferring to link static libraries. | |||
| libs = get_libs_for_static_executable(dep) | |||
| # TODO: This is changing the order of linker inputs, it shouldn't. | |||
There was a problem hiding this comment.
The fix is to iterate through libs once, right?
There was a problem hiding this comment.
Correct. I didn't want to do that in this PR, but it should be fairly straightforward.
| dylibs = [l for l in libs.to_list() if l.basename.endswith(toolchain.dylib_ext)] | ||
| staticlibs = [l for l in libs.to_list() if l.basename.endswith(toolchain.staticlib_ext)] | ||
| transitive_dylibs = depset(transitive = [transitive_dylibs, depset(dylibs)]) | ||
| transitive_staticlibs = depset(transitive = [transitive_staticlibs, depset(staticlibs)]) | ||
| linkopts.extend(dep[CcInfo].linking_context.user_link_flags) |
There was a problem hiding this comment.
Doesn't this also need to be building up transitive_linkopts when rust -> rust -> c?
| @@ -397,6 +410,7 @@ def add_native_link_flags(args, dep_info): | |||
| args.add_all(native_libs, map_each = _get_dirname, uniquify = True, format_each = "-Lnative=%s") | |||
| args.add_all(dep_info.transitive_dylibs, map_each = get_lib_name, format_each = "-ldylib=%s") | |||
| args.add_all(dep_info.transitive_staticlibs, map_each = get_lib_name, format_each = "-lstatic=%s") | |||
| args.add_all(dep_info.linkopts, uniquify = False) | |||
There was a problem hiding this comment.
Why is uniquify false? Because it already came from a depset?
hlopko
force-pushed
the
propagate_linkopts
branch
from
11b0249 to
1eae40f
Compare
|
Marcel left the Bazel team. closing. |
|
Ouch :) FTR, it's the newborn that is the reason, not the team transfer :) I still plan to work on the rules when the time allows. |
|
Ok no worries :) just reopen the pr if you want to revive it.
…On Tue, May 19, 2020, 9:00 AM Marcel Hlopko ***@***.***> wrote:
Ouch :) FTR, it's the newborn that is the reason, not the team transfer :)
I still plan to work on the rules when the time allows.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#264 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA4MO76IBVRBOPO2CHZMUPDRSIVA7ANCNFSM4JINCKGA>
.
|
|
i was wondering if this will be worked on in the near future? |
|
also, is there any workaround for this issue for now? |
|
can we reopen this issue? this is not resolved, right? |
|
This is not an issue, this is PR and @hlopko had no time to work on it so far (in addition to having moved out the Bazel team). |
This PR ensures that
linkoptsattributes from dependencies such ascc_librarymake it to therust_binarylinker invocation.Not-so-great but still motivating example is that with this PR we can make openssl-sys crate work with system installed openssl just by pushing additional_deps
cc_libraries:I updated skylib and made use of its unittest.bzl framework for adding the unittest.
And since I was there, I added one TODO regarding libraries to link ordering.