Properly support target-specific capabilities #267
Conversation
Some features with the "supports_*" naming style were moved to be tool capabilites despite being target capabilities rather than toolchain tool capabilities. This change restores them to their original location so they can be enabled at the cc_toolchain level instead.
|
If I understand correctly, then would we not just want to remove them entirely (or define the corresponding config settings for users)? For example: config_setting(
name = "supports_dynamic_linker",
# Or whatever condition on the target platform would match this?
values = {"cpu": "arm"},
)
cc_args(
name = "dynamic_link_args",
args = select({
":supports_dynamic_linker": ["dynamic_link_args"],
"//conditions:default": ["static_link_args"],
}),
) |
|
Was in the middle of writing up more thoughts in a comment. :) We do need to keep these around since these features affects how Bazel handles compilation in various ways:
I do still like the protection of preventing "implies", so I'm sort of tempted to create a
|
Hadn't considered that, that's a good point
That does seem like the right approach for as long as we're stuck transpiling to the legacy toolchain mechanism, though we probably want to do what I mentioned above once we're no longer doing that. I'm ok with just submitting this for now and we can discuss that later if it'd help you move forward.
I'd quite strongly advocate against this, because Edit: Just realized that if you do it from |
That's what I was starting to lean towards. I'll rework this CL to add a
Yeah, that alone is a great reason not to go down that route. Crossing wires across exec/target platform makes things extra confusing. |
This reverts commit 272f417.
Changes supports_pic and supports_dynamic_linker features to cc_target_capability rules since the capability is tied to target platform capabilities rather than tool capabilities.
armandomontanez
changed the title
armandomontanez
requested review from
trybka,
pzembrod,
comius and
c-mita
as code owners
|
Should be ready for review now, sorry for the churn. :) |
| @@ -10,10 +11,10 @@ cc_tool_capability( | |||
| name = "supports_interface_shared_libraries", | |||
| ) | |||
|
|
|||
| cc_tool_capability( | |||
| cc_target_capability( | |||
There was a problem hiding this comment.
Do we want to consider putting these into two seperate packages //cc/toolchains/capabilities/tool and //cc/toolchains/capabilities/target
| @@ -119,6 +119,12 @@ FeatureInfo = provider( | |||
| "allowlist_include_directories": "(depset[DirectoryInfo]) Include directories implied by this feature that should be allowlisted in Bazel's include checker", | |||
| }, | |||
| ) | |||
|
|
|||
| FeatureImplyabilityInfo = provider( | |||
There was a problem hiding this comment.
Any feature that returns FeatureSetInfo should be implyable, as they can be toggled by the toolchain via known_features and enabled_features.
I'd prefer you removed FeatureImplyabilityInfo and just made cc_target_capability not return a FeatureSetInfo, and added a seperate target_capabilities attribute to the toolchain configuration, as it's neither a known feature nor an enabled feature.
There was a problem hiding this comment.
Do we really want to add a dedicated attribute for something we've already decided is an anti-pattern though?
To fully expand this problem space(sans hidden behaviors that are undocumented):
supports_pic: platform specific, controlled by a feature, has interactions with--force_pic.supports_dynamic_linker: platform specific, controlled by a feature.supports_start_end_lib: tool-dependent, controlled by a feature and--start_end_lib.supports_interface_shared_libraries: tool dependent, controlled by a feature and--interface_shared_objects.supports_header_parsing: tool dependent, controlled by an attribute oncc_toolchain, marked as deprecated in its usage in code.supports_param_files: tool dependent, controlled by an attribute oncc_toolchain.
There's a bunch of fragmentation here. I'm happy with cc_tool_capability, with the minor caveat that clearly some tool capabilities can't cleanly be expressed that way today.
It's starting to feel like we have other paths for these two oddball cases that are target-specific:
- Make the fact that they're features purely an implementation detail, and just expose them as direct bool attributes of a
cc_toolchainconfiguration a lasupports_header_parsing. Users won't be able to express feature relationships with these any more, but I'm not seeing any reason that they should or any instances where Bazel relies on this being possible. - Maybe they should just be
bool_flags, which lets platforms control the behavior directly at the expense of losing any correlation with the toolchain configuration. This seems like the most "normal" Bazel paradigm, but is by far the most divergent pattern because of how it would require projects to change their flags (and/or attach flags to platforms). - Treat them as features, and just make them no-op
cc_features (rather thancc_external_feature, which implies a definition exists externally that shouldn't be clobbered). This is the least elegant, but the most widely compatible with the current ecosystem.
@comius This relates to starlarkification a bit. Is there a long-term vision for how users should control implementation behavior of cc rules for things like this?
There was a problem hiding this comment.
Do we really want to add a dedicated attribute for something we've already decided is an anti-pattern though?
I think that the fact that they're an antipattern is precisely why they should get special treatment. It'd be a lot easier to migrate from cc_target_capability if they weren't mixed in with other features.
There's a bunch of fragmentation here. I'm happy with
cc_tool_capability, with the minor caveat that clearly some tool capabilities can't cleanly be expressed that way today.It's starting to feel like we have other paths for these two oddball cases that are target-specific:
- Make the fact that they're features purely an implementation detail, and just expose them as direct bool attributes of a
cc_toolchainconfiguration a lasupports_header_parsing. Users won't be able to express feature relationships with these any more, but I'm not seeing any reason that they should or any instances where Bazel relies on this being possible.
This seems quite reasonable to me. This could possibly be merged with option 2 where if you set supports_pic = true in the toolchain config rule, it'd transition to the flag supports_pic = True.
- Maybe they should just be
bool_flags, which lets platforms control the behavior directly at the expense of losing any correlation with the toolchain configuration. This seems like the most "normal" Bazel paradigm, but is by far the most divergent pattern because of how it would require projects to change their flags (and/or attach flags to platforms).
I like this option, because it would translate the best to a starlarkified world. It might make them a little more awkward to use for now, but this way people would do selects on them, which matches how we'd expect them to be used when they're fully expressed in starlark.
- Treat them as features, and just make them no-op
cc_features (rather thancc_external_feature, which implies a definition exists externally that shouldn't be clobbered). This is the least elegant, but the most widely compatible with the current ecosystem.
I'm not a fan of this option, mainly because anyone using it improperly would give themselves a nightmare for when they do get starlarkified.
| # features from implying these kinds of rules. | ||
| return [ | ||
| ft, | ||
| FeatureSetInfo(label = ctx.label, features = depset([ft])), |
There was a problem hiding this comment.
As mentioned above, I'd prefer that this returns neither a FeatureInfo or a FeatureSetInfo.
Changes
supports_picandsupports_dynamic_linkerfeatures tocc_target_capabilityrules since the capability is tied to target platform properties rather than tool properties.