-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Add aspect hints to cc_shared_library #17725
Changes from 5 commits
6165d17
2f469c9
096b902
7e4c566
8b5c17a
e5613bd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,7 +37,7 @@ GraphNodeInfo = provider( | |
"Nodes in the graph of shared libraries.", | ||
fields = { | ||
"children": "Other GraphNodeInfo from dependencies of this target", | ||
"label": "Label of the target visited", | ||
"owners": "Owners of the linker inputs in the targets visited", | ||
"linkable_more_than_once": "Linkable into more than a single cc_shared_library", | ||
}, | ||
) | ||
|
@@ -54,6 +54,41 @@ CcSharedLibraryInfo = provider( | |
}, | ||
) | ||
|
||
CcSharedLibraryHintInfo = provider( | ||
doc = """ | ||
This provider should be used by rules that provide C++ linker inputs and | ||
want to guide what the cc_shared_library uses. The reason for this may be | ||
for example because the rule is not providing a standard provider like | ||
CcInfo or ProtoInfo or because the rule does not want certain attributes | ||
to be used for linking into shared libraries. It may also be needed if the | ||
rule is using non-standard linker_input.owner names. | ||
|
||
Propagation of the cc_shared_library aspect will always happen via all | ||
attributes that provide either CcInfo, ProtoInfo or | ||
CcSharedLibraryHintInfo, the hints control whether the result of that | ||
propagation actually gets used. | ||
""", | ||
fields = { | ||
"attributes": ("[String] - If not set, the aspect will use the result of every " + | ||
"dependency that provides CcInfo, ProtoInfo or CcSharedLibraryHintInfo. " + | ||
"If empty list, the aspect will not use the result of any dependency. If " + | ||
"the list contains a list of attribute names, the aspect will only use the " + | ||
"dependencies corresponding to those attributes as long as they provide CcInfo, " + | ||
"ProtoInfo or CcSharedLibraryHintInfo"), | ||
"owners": ("[Label] - cc_shared_library will know which linker_inputs to link based on the owners "+ | ||
"field of each linker_input. Most rules will simply use the ctx.label but certain " + | ||
"APIs like cc_common.create_linker_input(owner=) accept any label. " + | ||
"cc_common.create_linking_context_from_compilation_outputs() accepts a `name` which " + | ||
"will then be used to create the owner of the linker_input together with ctx.package." + | ||
"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 " + | ||
"the original ctx.label (including name) is kept as part of the owner." | ||
) | ||
}, | ||
) | ||
|
||
# For each target, find out whether it should be linked statically or | ||
# dynamically. | ||
def _separate_static_and_dynamic_link_libraries( | ||
|
@@ -72,16 +107,45 @@ def _separate_static_and_dynamic_link_libraries( | |
break | ||
|
||
node = all_children[i] | ||
node_label = str(node.label) | ||
|
||
if node_label in seen_labels: | ||
continue | ||
seen_labels[node_label] = True | ||
|
||
if node_label in can_be_linked_dynamically: | ||
targets_to_be_linked_dynamically_set[node_label] = True | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Add _ to match the variable name, this is more consistent with the rest of the comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
# happens. Every value in node.owners presumably corresponds to a | ||
# linker_input in the same exact target. Therefore if we have seen any | ||
# of the owners already, then we must have also seen all the other | ||
# owners in the same node. Viceversa when we haven't seen them yet. If | ||
# both of these values are non-zero after the loop, the most likely | ||
# reason would be a bug in the implementation. It could potentially be | ||
# triggered by users if they use owner labels that do not keep most of | ||
# the ctx.label.package and ctx.label.name which then clash with other | ||
# target's owners (unlikely). For now though if the error is | ||
# triggered, it's reasonable to require manual revision by | ||
# the cc_shared_library implementation owners. | ||
seen_count = 0 | ||
not_seen_count = 0 | ||
for owner in node.owners: | ||
owner_str = str(owner) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
if owner_str in seen_labels: | ||
seen_count += 1 | ||
continue | ||
|
||
not_seen_count += 1 | ||
|
||
|
||
seen_labels[owner_str] = True | ||
|
||
if owner_str in can_be_linked_dynamically: | ||
targets_to_be_linked_dynamically_set[owner_str] = True | ||
else: | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can change it to booleans but must keep the two variables (discussed offline). |
||
fail("Your build has triggered a programmatic error in the cc_shared_library rule. " | ||
+ "Please file an issue in https://github.com/bazelbuild/bazel") | ||
|
||
if must_add_children: | ||
all_children.extend(node.children) | ||
|
||
return (targets_to_be_linked_statically_map, targets_to_be_linked_dynamically_set) | ||
|
@@ -209,20 +273,22 @@ def _find_top_level_linker_input_labels( | |
break | ||
|
||
node = nodes_to_check[i] | ||
node_label = str(node.label) | ||
if node_label in linker_inputs_to_be_linked_statically_map: | ||
has_code_to_link = False | ||
for linker_input in linker_inputs_to_be_linked_statically_map[node_label]: | ||
if _contains_code_to_link(linker_input): | ||
top_level_linker_input_labels_set[node_label] = True | ||
has_code_to_link = True | ||
break | ||
|
||
if not has_code_to_link: | ||
nodes_to_check.extend(node.children) | ||
elif node_label not in targets_to_be_linked_dynamically_set: | ||
# This can happen when there was a target in the graph that exported other libraries' | ||
# linker_inputs but didn't contribute any linker_input of its own. | ||
must_add_children = False | ||
for owner in node.owners: | ||
owner_str = str(owner) | ||
if owner_str in linker_inputs_to_be_linked_statically_map: | ||
must_add_children = True | ||
for linker_input in linker_inputs_to_be_linked_statically_map[owner_str]: | ||
if _contains_code_to_link(linker_input): | ||
top_level_linker_input_labels_set[owner_str] = True | ||
must_add_children = False | ||
break | ||
elif owner_str not in targets_to_be_linked_dynamically_set: | ||
# This can happen when there was a target in the graph that exported other libraries' | ||
# linker_inputs but didn't contribute any linker_input of its own. | ||
must_add_children = True | ||
|
||
if must_add_children: | ||
nodes_to_check.extend(node.children) | ||
|
||
return top_level_linker_input_labels_set | ||
|
@@ -590,10 +656,20 @@ def _cc_shared_library_impl(ctx): | |
def _graph_structure_aspect_impl(target, ctx): | ||
children = [] | ||
|
||
attributes = dir(ctx.rule.attr) | ||
owners = [ctx.label] | ||
if CcSharedLibraryHintInfo in target: | ||
if hasattr(target[CcSharedLibraryHintInfo], "attributes"): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These can be simplified via:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. Thanks. |
||
attributes = target[CcSharedLibraryHintInfo].attributes | ||
|
||
if hasattr(target[CcSharedLibraryHintInfo], "owners"): | ||
owners = target[CcSharedLibraryHintInfo].owners | ||
|
||
|
||
# Collect graph structure info from any possible deplike attribute. The aspect | ||
# itself applies across every deplike attribute (attr_aspects is *), so enumerate | ||
# over all attributes and consume GraphNodeInfo if available. | ||
for fieldname in dir(ctx.rule.attr): | ||
for fieldname in attributes: | ||
deps = getattr(ctx.rule.attr, fieldname, None) | ||
if type(deps) == "list": | ||
for dep in deps: | ||
|
@@ -609,17 +685,16 @@ def _graph_structure_aspect_impl(target, ctx): | |
for tag in ctx.rule.attr.tags: | ||
if tag == LINKABLE_MORE_THAN_ONCE: | ||
linkable_more_than_once = True | ||
|
||
return [GraphNodeInfo( | ||
label = ctx.label, | ||
owners = owners, | ||
children = children, | ||
linkable_more_than_once = linkable_more_than_once, | ||
)] | ||
|
||
graph_structure_aspect = aspect( | ||
attr_aspects = ["*"], | ||
required_providers = [[CcInfo], [ProtoInfo]], | ||
required_aspect_providers = [[CcInfo]], | ||
required_providers = [[CcInfo], [ProtoInfo], [CcSharedLibraryHintInfo]], | ||
required_aspect_providers = [[CcInfo], [CcSharedLibraryHintInfo]], | ||
implementation = _graph_structure_aspect_impl, | ||
) | ||
|
||
|
@@ -654,3 +729,4 @@ merge_cc_shared_library_infos = _merge_cc_shared_library_infos | |
build_link_once_static_libs_map = _build_link_once_static_libs_map | ||
build_exports_map_from_only_dynamic_deps = _build_exports_map_from_only_dynamic_deps | ||
throw_linked_but_not_exported_errors = _throw_linked_but_not_exported_errors | ||
separate_static_and_dynamic_link_libraries = _separate_static_and_dynamic_link_libraries |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
// Copyright 2023 The Bazel Authors. All rights reserved. | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
int indirect_dep3() { return 0; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made it a bit more explicit.