cc_proto_library doesn't get linked for binary with dynamic_deps

[eagleonhill]

Description of the bug:

Because cc_proto_library doesn't use deps for linking dependencies, it's never part of GraphNodeInfo.
This happens even the cc_binary has empty list of dynamic_deps

As a result,

bazel/src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl

Line 383 in f79ec3d

(link_statically_labels, link_dynamically_labels) = _separate_static_and_dynamic_link_libraries(graph_structure_aspect_nodes, can_be_linked_dynamically)

it's not statically nor dynamically linked.

bazel/src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl

Line 393 in f79ec3d

if owner not in link_dynamically_labels and (owner in link_statically_labels or str(ctx.label) == owner):

Such libraries are excluded for build.

I made a local workaround by removing the 2nd term, that assumes all unknown targets are staticaly linked.

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

No response

Which operating system are you running Bazel on?

ubuntu

What is the output of bazel info release?

5.2.0

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response

[eagleonhill]

Actually, this bug isn't just cc_proto_libray. Using cmake rule from foreign_rules_cc also won't work.

[sgowroji]

Hi @eagleonhill, Could you please provide minimal steps to reproduce the above issue. Thanks!

[oquenchil]

I am unable to reproduce this for cc_proto_library. If you have a cc_binary that depends on a cc_proto_library whose proto_library depends on another one, everything works as expected. The Graph structure aspect is propagated along the edges of the deps attribute of the proto_library rule (the aspect is applied on the CcProtoAspect aspect itself). So to know exactly what was going on in your case, I'd need a repro.

In any case, the bug report is useful, I can certainly believe this happens to the cmake for foreign_rules_cc. The problem is this PR ff927f7 which was a red-herring and not the actual reason for #16296 (comment) .

We can restore the change that propagated across all attributes. Also the static_deps attribute has been removed from the cc_shared_library rule 9815b76 so everything remaining (not coming from dynamic_deps) will be linked statically anyway, we still need to aspect to look at the LINKABLE_MORE_THAN_ONCE hint though. We can even make it more efficient so that propagation only happens when the CcInfo provider is advertised. I will start working on that.

The cmake rule will only have to advertise CcInfo if it doesn't already. There won't be a need for any other hints.

[oquenchil]

We can't actually have required_providers on the aspect either because of protos and aspects. So it will inefficiently propagate across more than is needed but it will work for all cases.

[fmeum]

@bazel-io flag

[iancha1992]

@bazel-io fork 6.4.0

[iancha1992]

@oquenchil @hvadehra @fmeum We tried to cherry-pick this to release-6.4.0, but there were some conflicts.

1. src/main/starlark/builtins_bzl/common/cc/experimental_cc_shared_library.bzl
   Currently, below is missing in the release-6.4.0 branch

if hasattr(ctx.rule.attr, "deps") and type(ctx.rule.attr.deps) != "Target":
        for dep in ctx.rule.attr.deps:
            if GraphNodeInfo in dep:
                children.append(dep[GraphNodeInfo])

Can you please take a look? Thank you!

cc: @bazelbuild/triage

[hvadehra]

cc @buildbreaker2021

[oquenchil]

I put everything from head cc_shared_library into 6.3. There have been no changes to cc_shared_library since then.

[Wyverald]

Just a heads up that 6.4.0rc1 will be cut next Monday, so this needs to be cherry-picked before then if we want to backport it into 6.4.0.

[fmeum]

This was done in #19534