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

use crate_info.deps in establish_cc_info #1543

Merged
merged 2 commits into from
Sep 9, 2022
Merged

Conversation

krasimirgg
Copy link
Collaborator

@krasimirgg krasimirgg commented Sep 8, 2022

This avoids the dependence on ctx.attr, allowing custom deps registered via custom transform_deps to be propagated correctly. I believe this should be a non-functional change for rules_rust.
Internally, we have a slightly modified rust_library that has cc_deps attribute which is processed by https://github.com/google/crubit, which posts extra deps via transform_deps. Currently, for obvious reasons, doesn't get processed in establish_cc_info.

This avoids the dependence on ctx.attr, allowing custom deps
registered via custom transform_deps to be propagated correctly.

I believe this should be a non-functional change for rules_rust.
@scentini scentini closed this Sep 8, 2022
@scentini scentini reopened this Sep 8, 2022
@krasimirgg krasimirgg marked this pull request as ready for review September 8, 2022 13:57
Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

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

I think this seems fine. Would you be able to link the crubit modified version of the rules so I can better understand this change?

@scentini
Copy link
Collaborator

scentini commented Sep 8, 2022

We are patching the actual rust_{library, binary, test} internally, so it's not easy to export it, but I can summarize what we do:

  1. We add a cc_deps attribute that has the rust_bindings_from_cc_aspect attached to it.
  2. We replace the transform_deps invocations with a custom function that transforms the RustBindingsFromCcInfo provider produced by the aspect into DepVariantInfos. It looks something like this:

def collect_transformed_deps(ctx):
    """Creates DepVariantInfos from all dependencies and from the rust_bindings_from_cc_aspect.

    Args:
        ctx (ctx): The target's context object.

    Returns:
        list[DepVariantInfo]: a list of DepVariantInfos that this target depends on.
    """
    deps = [DepVariantInfo(
        crate_info = dep[CrateInfo] if CrateInfo in dep else None,
        dep_info = dep[DepInfo] if DepInfo in dep else None,
        build_info = dep[BuildInfo] if BuildInfo in dep else None,
        cc_info = dep[CcInfo] if CcInfo in dep else None,
    ) for dep in ctx.attr.deps]

    if hasattr(ctx.attr, "cc_deps"):
        for dep in ctx.attr.cc_deps:
            if RustBindingsFromCcInfo in dep:
                deps.append(dep[RustBindingsFromCcInfo].dep_variant_info)
                deps.append(
                    DepVariantInfo(
                        cc_info = dep[RustBindingsFromCcInfo].cc_info,
                        crate_info = None,
                        dep_info = None,
                        build_info = None,
                    ),
                )

    return deps

Long term plan is to make rules_rust API stable enough and extensible enough so that we don't need to patch the rules but rather extend them.

@scentini scentini merged commit f73d1d6 into bazelbuild:main Sep 9, 2022
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.

None yet

3 participants