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.

Sign up for GitHub

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

Jump to bottom

cc_shared_library not considering linker_inputs from rules/aspects that didn't create them #17676

Closed
oquenchil opened this issue Mar 7, 2023 · 2 comments
Closed

cc_shared_library not considering linker_inputs from rules/aspects that didn't create them #17676

oquenchil opened this issue Mar 7, 2023 · 2 comments
Labels
P1 I'll work on this now. (Assignee required) team-Rules-CPP Issues for C++ rules type: bug

Comments

@oquenchil
Copy link
Contributor

Copy pasting from: #17155 (comment)

git clone https://github.com/grpc/grpc.git
cd grpc
git checkout v1.45.1
vim BUILD and add this rule
cc_shared_library(
    name = "test",
    roots = ["rls_upb"],
    static_deps = ["//:__subpackages__",],
)
export OVERRIDE_BAZEL_VERSION=5.2.0
bazel build test --experimental_cc_shared_library
nm -C bazel-bin/libtest.so

grpc_lookup_v1_RouteLookupRequest_msginit is not there
@oquenchil oquenchil added type: bug P1 I'll work on this now. (Assignee required) team-Rules-CPP Issues for C++ rules labels Mar 7, 2023
@oquenchil oquenchil mentioned this issue Mar 7, 2023
Closed
@oquenchil
Copy link
Contributor Author

Fixed in f9008f6

@oquenchil
Copy link
Contributor Author

So this was fixed for cc_proto_library which also had issues but it seems it still doesn't work for upb.

@oquenchil oquenchil reopened this Mar 8, 2023
@oquenchil oquenchil mentioned this issue Mar 10, 2023
Closed
fweikert pushed a commit to fweikert/bazel that referenced this issue May 25, 2023
@oquenchil @fweikert
Add aspect hints to cc_shared_library
3eee220 
One reason why cc_shared_library wasn't working for upb_proto_library was because indirect cc_shared_library.deps (i.e.  linker_inputs not provided by the immediate dependency) were being dropped, that was fixed in bazelbuild@f9008f6.

A second reason why it wasn't working was because until now cc_shared_library has relied on transitive deps providing CcInfo or ProtoInfo while the upb_proto_library aspect provides a custom wrapped CcInfo.  This change fixes that via aspect hints. (It would still require a change in the upb rule implementation).

A third reason why it didn't work for upb was because to avoid a collision with the other aspects applied on proto_libraries, the upb_proto_library implementation added a suffix to the name when calling cc_common.create_linking_context(). This in turn caused the `owner` used in the `linker_inputs` to not match the labels of the targets seen by the cc_shared_library. To fix this, the hints provider accepts a list of owners.

Apart from those features, the hints provider also allows specifying a list of attributes for the cases where we don't want the result of every dependency to be used (this hasn't come up yet and it doesn't affect upb).

This idea was more or less described [here](bazelbuild#16296 (comment)).

Note: I am using the words "aspect hints" because conceptually they are. However, none of this is using the aspect_hints mechanism supported by Bazel rules. Here we are simply using providers.

Fixes bazelbuild#17676.

Closes bazelbuild#17725.

PiperOrigin-RevId: 516488095
Change-Id: I603ed8df90fef0636525d60707692930cd23fa5a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
P1 I'll work on this now. (Assignee required) team-Rules-CPP Issues for C++ rules type: bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add aspect hints to cc_shared_library oquenchil/bazel
1 participant
@oquenchil