rustc_compile_action now requires an explicit attr param
#886
Conversation
UebelAndre
changed the title
rustc.bzlrustc_compile_action now requires explicit attr parameter
UebelAndre
changed the title
rustc_compile_action now requires explicit attr parameterrustc_compile_action now requires an explicit attr parameter
UebelAndre
changed the title
rustc_compile_action now requires an explicit attr parameterrustc_compile_action now requires an explicit attr param
| @@ -92,7 +92,7 @@ def _build_script_impl(ctx): | |||
| # Pull in env vars which may be required for the cc_toolchain to work (e.g. on OSX, the SDK version). | |||
| # We hope that the linker env is sufficient for the whole cc_toolchain. | |||
| cc_toolchain, feature_configuration = find_cc_toolchain(ctx) | |||
| _, _, linker_env = get_linker_and_args(ctx, cc_toolchain, feature_configuration, None) | |||
| _, _, linker_env = get_linker_and_args(ctx, ctx.attr, cc_toolchain, feature_configuration, None) | |||
There was a problem hiding this comment.
Oh thanks for this cleanup. I didn't want to force you to do it in this PR, but thank you! :) I'd recommend going a bit further and only pass attributes that are really needed. Here get_linker_and_args actually only needs deps, so I'd pass those instead of the full ctx.attr.
No need to do it in this PR, but I'd also try (as much as reasonable) to not pass ctx around unnecessarily. Here get_linker_and_args only uses it for getting ctx.fragments.cpp.linkopts, so I'd probably pass linkopts. In some situations it may be more reasonable to pass the cpp fragment.
| @@ -220,6 +220,7 @@ def _rust_proto_compile(protos, descriptor_sets, imports, crate_name, ctx, is_gr | |||
|
|
|||
| return rustc_compile_action( | |||
| ctx = ctx, | |||
| attr = ctx.attr, | |||
There was a problem hiding this comment.
Yeah changing this to only needed attributes is a huge change, and I agree that it should be done in a separate PR, not in this one. 👍
| @@ -204,7 +206,7 @@ def get_linker_and_args(ctx, cc_toolchain, feature_configuration, rpaths): | |||
|
|
|||
| # Add linkopt's from dependencies. This includes linkopts from transitive | |||
| # dependencies since they get merged up. | |||
| for dep in ctx.attr.deps: | |||
| for dep in getattr(attr, "deps", []): | |||
There was a problem hiding this comment.
And by making deps separate function parameter we can make it mandatory, and we can skip gettattr.
| out_binary = getattr(ctx.attr, "out_binary") | ||
| # TODO: Remove after some resolution to | ||
| # https://github.com/bazelbuild/rules_rust/issues/771 | ||
| out_binary = getattr(attr, "out_binary", False) |
|
|
||
| return providers | ||
|
|
||
| def _is_dylib(dep): | ||
| return not bool(dep.static_library or dep.pic_static_library) | ||
|
|
||
| def establish_cc_info(ctx, crate_info, toolchain, cc_toolchain, feature_configuration): | ||
| def establish_cc_info(ctx, attr, crate_info, toolchain, cc_toolchain, feature_configuration): |
There was a problem hiding this comment.
Again, this can be a separate PR, but here I'd pass ctx.actions separately, as well as deps, as well as deps, label, and out_binary
|
I've opened #889 to track the additional comments here. Seems like there's a decent amount of things that could be updated so to make sure that gets done and things are as consistent as possible going forward, we should discuss what we want the code to look like 😄 |
In addition to some slight cleanup of using keyword arguments in more places,
rustc_compile_action, an internal function, now requires that attributes be passed explicitly and no longer usesctx.attr.