Skip to content

Add excludes to cc_action_type_set #669

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

Open
keith wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add excludes to cc_action_type_set #669

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion cc/toolchains/actions.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,13 @@ cc_action_type(
def _cc_action_type_set_impl(ctx):
if not ctx.attr.actions and not ctx.attr.allow_empty:
fail("Each cc_action_type_set must contain at least one action type.")
actions = collect_action_types(ctx.attr.actions)
if ctx.attr.excludes:
excludes = {a: True for a in collect_action_types(ctx.attr.excludes).to_list()}
actions = depset([a for a in actions.to_list() if a not in excludes])
return [ActionTypeSetInfo(
label = ctx.label,
actions = collect_action_types(ctx.attr.actions),
actions = actions,
)]

cc_action_type_set = rule(
Expand Down Expand Up @@ -95,6 +99,10 @@ cc_action_type_set(
mandatory = True,
doc = "A list of cc_action_type or cc_action_type_set",
),
"excludes": attr.label_list(
providers = [ActionTypeSetInfo],
doc = "A list of cc_action_type or cc_action_type_set to exclude from the action set. Applied after accumulating all actions.",
),
"allow_empty": attr.bool(default = False),
},
provides = [ActionTypeSetInfo],
Expand Down
26 changes: 14 additions & 12 deletions cc/toolchains/actions/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,22 +233,24 @@ cc_action_type_set(
],
)

# This should match `compile_actions` with `lto_backend` omitted, because `lto_backend` actions
# do not instantiate the full set of build variables.
cc_action_type_set(
name = "source_compile_actions",
actions = [
":linkstamp_compile",
":cpp_compile",
":compile_actions",
],
excludes = [
# `lto_backend` actions do not instantiate the full set of build variables.
":lto_backend",
],
)

cc_action_type_set(
name = "compile_actions_without_header_parsing",
Copy link
Copy Markdown
Collaborator

@armandomontanez armandomontanez Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a little overkill since we can just do this:

cc_action_type_set(
    name = "source_compile_actions",
    actions = [
        ":compile_actions_without_header_parsing",
        ":cpp_header_parsing",
    ],
)

I'm not convinced the set of actions is unruly enough to justify extending the API, but it's possible there are more complex usages that could justify it.

I'd prefer for the case of fixing this bug we keep the solution simple.

Copy link
Copy Markdown
Collaborator

@armandomontanez armandomontanez Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which immediately pushes the complexity further down... let me see if there's a path to cleaning this up without making a huge mess.

Copy link
Copy Markdown
Member Author

@keith keith Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think one nice thing about the exclusion api is it's much more clear looking at a target definition like this that it's not missing anything else, where in the current source_compile_actions just looking at the definition you can't easily spot the things that are being omitted, and you also can't spot things that were accidentally left out as new actions are addedd

Copy link
Copy Markdown
Collaborator

@armandomontanez armandomontanez Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Purely accumulating doesn't look too bad after a bit of cleanup: #686

I could be convinced that excludes is justified if there are further compelling examples that live outside of rules_cc.

actions = [
":compile_actions",
],
excludes = [
":cpp_header_parsing",
":cpp_module_compile",
":cpp_module_codegen",
":clif_match",
":objcpp_compile",
":preprocess_assemble",
":c_compile_actions",
":assembly_actions",
":objc_compile",
],
)

Expand Down
15 changes: 13 additions & 2 deletions cc/toolchains/args/compiler_input_flags/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,29 @@ load("//cc/toolchains:feature.bzl", "cc_feature")

cc_feature(
name = "feature",
args = [":flags"],
args = [
":flags",
":header_parsing_args",
],
overrides = "//cc/toolchains/features/legacy:compiler_input_flags",
visibility = ["//visibility:public"],
)

cc_args(
name = "flags",
actions = ["//cc/toolchains/actions:compile_actions"],
actions = ["//cc/toolchains/actions:compile_actions_without_header_parsing"],
args = [
"-c",
"{source_file}",
],
format = {"source_file": "//cc/toolchains/variables:source_file"},
requires_not_none = "//cc/toolchains/variables:source_file",
)

cc_args(
name = "header_parsing_args",
actions = ["//cc/toolchains/actions:cpp_header_parsing"],
args = ["{source_file}"],
format = {"source_file": "//cc/toolchains/variables:source_file"},
requires_not_none = "//cc/toolchains/variables:source_file",
)
25 changes: 25 additions & 0 deletions tests/rule_based_toolchain/actions/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,31 @@ util.helper_target(
visibility = ["//tests/rule_based_toolchain:__subpackages__"],
)

util.helper_target(
cc_action_type_set,
name = "all_compile_except_c",
actions = [
":c_compile",
":cpp_compile",
],
excludes = [
":c_compile",
],
visibility = ["//tests/rule_based_toolchain:__subpackages__"],
)

util.helper_target(
cc_action_type_set,
name = "all_compile_except_c_nested",
actions = [
":all_compile",
],
excludes = [
":c_compile",
],
visibility = ["//tests/rule_based_toolchain:__subpackages__"],
)

analysis_test_suite(
name = "test_suite",
targets = TARGETS,
Expand Down
12 changes: 12 additions & 0 deletions tests/rule_based_toolchain/actions/actions_test.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,24 @@ def _test_action_types_impl(env, targets):
targets.cpp_compile.label,
])

def _test_action_type_set_excludes_impl(env, targets):
env.expect.that_target(targets.all_compile_except_c).provider(ActionTypeSetInfo) \
.actions().contains_exactly([targets.cpp_compile.label])

def _test_action_type_set_excludes_nested_impl(env, targets):
env.expect.that_target(targets.all_compile_except_c_nested).provider(ActionTypeSetInfo) \
.actions().contains_exactly([targets.cpp_compile.label])

TARGETS = [
":c_compile",
":cpp_compile",
":all_compile",
":all_compile_except_c",
":all_compile_except_c_nested",
]

TESTS = {
"actions_test": _test_action_types_impl,
"action_type_set_excludes_test": _test_action_type_set_excludes_impl,
"action_type_set_excludes_nested_test": _test_action_type_set_excludes_nested_impl,
}