Add aspect hints to cc_shared_library #17725
Conversation
|
@fmeum Could you please take a look? |
|
Is there a concern aspect hints aren't enabled by default? #14327 |
|
Nope, I am calling this aspect hint because conceptually it is, but it doesn't need the aspect hints mechanism supported by Bazel. It's just a regular provider. |
| else: | ||
| targets_to_be_linked_statically_map[node_label] = node.linkable_more_than_once | ||
| must_add_children = False | ||
| # The seen count is used to track a programmatic error and fail if it |
There was a problem hiding this comment.
nit: Add _ to match the variable name, this is more consistent with the rest of the comment.
| targets_to_be_linked_statically_map[owner_str] = node.linkable_more_than_once | ||
| must_add_children = True | ||
|
|
||
| if seen_count and not_seen_count: |
There was a problem hiding this comment.
not_seen_count will always be more than 0 if node.owners is not empty. Basically you can get rid of not_seen_count variable and rewrite this if as:
if len(node.owners) > 0 and seen_count:
# Do something
I do not think having not_seen_count as a separate variable makes sense since it is just a proxy check if node.owners is empty or not.
Also seen_count and not_seen_count(if you decide to keep it) can be boolean instead of int.
There was a problem hiding this comment.
I can change it to booleans but must keep the two variables (discussed offline).
| attributes = dir(ctx.rule.attr) | ||
| owners = [ctx.label] | ||
| if CcSharedLibraryHintInfo in target: | ||
| if hasattr(target[CcSharedLibraryHintInfo], "attributes"): |
There was a problem hiding this comment.
These can be simplified via:
attributes = getattr(target[CcSharedLibraryHintInfo], "attributes", dir(ctx.rule.attr))
owners = getattr(target[CcSharedLibraryHintInfo], "owners", [ctx.label])
| "For these cases, since the cc_shared_library cannot guess, the rule author should "+ | ||
| "provide a hint with the owners of the linker inputs. If the value of owners is not set, then " + | ||
| "ctx.label will be used. If the rule author passes a list and they want ctx.label plus some other " + | ||
| "label then they will have to add ctx.label explicitly. Always make sure that as much as possible of " + |
There was a problem hiding this comment.
The "as much as possible" part is a bit vague. Is it possible to explain more precisely which parts are relevant (repo name, package, prefix of name, ...)?
There was a problem hiding this comment.
Made it a bit more explicit.
| seen_count = 0 | ||
| not_seen_count = 0 | ||
| for owner in node.owners: | ||
| owner_str = str(owner) |
There was a problem hiding this comment.
Label is Starlark-hashable, so you should be able to key the dictionaries by owner rather than str(owner). While the dictionaries aren't retained, you would potentially save a bunch of garbage string allocations.
There was a problem hiding this comment.
Thank you. I added a TODO for this. That change would be a bit intrusive for this PR since the dictionaries passed/return by this method are used in many places.
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
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 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
ownerused in thelinker_inputsto 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.
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 #17676.