Skip to content

fix: feature sanity check in rule based toolchains #318

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

Closed
wants to merge 1 commit into from

Conversation

lukasoyen
Copy link

This check ensures feature names are not duplicated. It does this by collecting all features. Including those from capabilities from tools from the tool_map.

If we now annotate a gcc tool with @rules_cc//cc/toolchains/capabilities:supports_pic and that tool is used for c_compile_actions, cpp_compile_actions and link_actions, the capability is collected multiple times and so is the feature.

If we extend the test here, we exclude the override check on the same feature.

Copy link

google-cla bot commented Jan 27, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

This check ensures feature names are not duplicated. It does this by collecting all features. Including those from capabilities from tools from the `tool_map`.

If we now annotate a `gcc` tool with `@rules_cc//cc/toolchains/capabilities:supports_pic` and that tool is used for `c_compile_actions`, `cpp_compile_actions` and `link_actions`, the capability is collected multiple times and so is the feature.

If we extend the test here, we exclude the override check on the same feature.
@lukasoyen
Copy link
Author

Alternative #344

@armandomontanez
Copy link
Collaborator

#344 has merged, so closing this one.

@armandomontanez
Copy link
Collaborator

Oops, GC'd a little too early.

@lukasoyen
Copy link
Author

Solved another way in 4d4fc07

@lukasoyen lukasoyen closed this Jul 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants