Add support for optional cc_toolchain #3665
Conversation
UebelAndre
force-pushed
the
linker
branch
3 times, most recently
from
7cc8279 to
ff94a0b
Compare
UebelAndre
force-pushed
the
linker
branch
6 times, most recently
from
c7dccbf to
95a3d34
Compare
UebelAndre
force-pushed
the
linker
branch
2 times, most recently
from
0f56d2a to
92d42c4
Compare
|
@illicitonion @krasimirgg curious about your thoughts here 🙏 |
| if include: | ||
| env["INCLUDE"] = include | ||
| # Defaults for cxx flags. | ||
| env["CC"] = "${{pwd}}/{}".format(ctx.executable._fallback_cc.path) |
There was a problem hiding this comment.
nit: can we define these inside the build_script_runner executable to avoid passing extra env vars we don't need?
| name = "no_ar", | ||
| srcs = ["no_binary.rs"], | ||
| edition = "2021", | ||
| rustc_env = { |
There was a problem hiding this comment.
nit: Can we take the busybox approach? build a single copy of this, symlink it a few times, and then look at argv[0] to get the name it was invoked with?
rust/private/repository_utils.bzl
Outdated
| linker_label = "None" | ||
| linker_type = "None" | ||
| if include_linker: | ||
| linker_label = "\"//:rust-lld\"" |
There was a problem hiding this comment.
might be a bit cleaner to do
linker_label = None
if include_linker:
linker_label = "//:rust-lld"
...
template.format(linker_label = repr(linker_label))
| skip_next = False | ||
| continue | ||
|
|
||
| # Strip -Wl, prefix if using direct driver (it's only for compiler drivers) |
There was a problem hiding this comment.
in my mind this is another +1 to switching to cc_common.link :)
rust/private/rustc.bzl
Outdated
| cc_toolchain.static_runtime_lib(feature_configuration = feature_configuration), | ||
| map_each = get_lib_name, | ||
| format_each = "-lstatic=%s", | ||
| map_each = _get_dirname, |
There was a problem hiding this comment.
do we actually need to use these -L or can we just pass static_runtime_lib directly so it expands to the actual libs
| ) | ||
|
|
||
| def toolchain_linker_preference(): | ||
| """A flag to control which linker is preferred for linking Rust binaries. |
There was a problem hiding this comment.
can you expand this a bit to explain the difference between this setting and the cc_common.link one? i.e. what's the difference between enabling that and setting this one to cc? I'm worried that we're getting so many different linker options
There was a problem hiding this comment.
maybe the options should be "rust means rust linker, cc means use cc_common.link" so there's fewer possibilities?
There was a problem hiding this comment.
Yeah, this parameter is a bit awkward. The idea is that even if you have both provided rust_toolchain.linker and a cc_toolchain is configured, that you can be explicit about what linker you'd like to use. The primary use case is just to force the use of rust_toolchain.linker even when a cc_toolchain is defined.
rust/toolchain.bzl
Outdated
|
|
||
| sysroot_linker = _symlink_sysroot_bin(ctx, name, dest, linker_bin) | ||
| sysroot_linker_files = _symlink_sysroot_tree(ctx, name, linker, linker[DefaultInfo].default_runfiles.files) | ||
| direct_files.extend([sysroot_linker]) |
There was a problem hiding this comment.
nit: direct_files.append, same on the line below
rust/toolchain.bzl
Outdated
| # `target` cfg is used so a linker can be chose based on the target | ||
| # platform. Linker binaries are still required to be runnable in the | ||
| # `exec` configuration. | ||
| cfg = "target", |
There was a problem hiding this comment.
this doesn't seem right, I think what you want is set cfg = "exec" but instantiate it as
linker = select({
"@platforms//...": ...
})
and, interestingly enough, the select will be evaluated against the target platform, because it's resolved before the exec transition is applied. See cerisier/toolchains_llvm_bootstrapped@aaeeacd#diff-5e02a5ab7afca2441282a65cca097c8fade25a1d6d9ffbb71b920c19a1e58babR32-R39 and the corresponding explanation on the Bazel issue
2 participants
Changes:
rust_allocator_librariesrule implementation into it's own filemandatory = Falsefor@bazel_tools//tools/cpp:toolchain_typetoolchains.find_cc_toolchainreturnsNonelinkeras an optional attribute torust_toolchain. This is expected to berust-lldfor now.@rules_rust//rust/settings:toolchain_linker_preferencewhich in the eventrust_toolchain.linkerand acc_toolchainare present, chooses the linker to use. (Defaults tocc).