Skip to content

Commit

Permalink
Fix recursive behavior for implementation_deps
Browse files Browse the repository at this point in the history
  • Loading branch information
martis42 committed Nov 1, 2023
1 parent 6209dba commit 09d0ab3
Show file tree
Hide file tree
Showing 7 changed files with 64 additions and 10 deletions.
21 changes: 14 additions & 7 deletions src/aspect/dwyu.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,19 @@ def _do_ensure_private_deps(ctx):
"""
return ctx.rule.kind == "cc_library" and ctx.attr._use_implementation_deps

def _gather_transitive_reports(ctx):
"""
In recursive operation mode we have to propagate the DWYU report files of all transitive dependencies to ensure
the DWYU actions run for all targets. Not doing this will cause DWYU not being executed at all as without returning
the report files Bazel prunes the DWYU actions.
"""
reports = []
if ctx.attr._recursive:
reports.extend([dep[OutputGroupInfo].cc_dwyu_output for dep in ctx.rule.attr.deps])
if hasattr(ctx.rule.attr, "implementation_deps"):
reports.extend([dep[OutputGroupInfo].cc_dwyu_output for dep in ctx.rule.attr.implementation_deps])
return reports

def dwyu_aspect_impl(target, ctx):
"""
Implementation for the "Depend on What You Use" (DWYU) aspect.
Expand Down Expand Up @@ -225,11 +238,5 @@ def dwyu_aspect_impl(target, ctx):
arguments = [args],
)

# TODO recursion broken for implementation_deps
if ctx.attr._recursive:
transitive_reports = [dep[OutputGroupInfo].cc_dwyu_output for dep in ctx.rule.attr.deps]
else:
transitive_reports = []
accumulated_reports = depset(direct = [report_file], transitive = transitive_reports)

accumulated_reports = depset(direct = [report_file], transitive = _gather_transitive_reports(ctx))
return [OutputGroupInfo(cc_dwyu_output = accumulated_reports)]
7 changes: 6 additions & 1 deletion src/aspect/factory.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,12 @@ def dwyu_aspect_factory(
Returns:
Configured DWYU aspect
"""
attr_aspects = ["deps"] if recursive else []
attr_aspects = []
if recursive:
if use_implementation_deps:
attr_aspects = ["implementation_deps", "deps"]
else:
attr_aspects = ["deps"]
aspect_config = [config] if config else []
aspect_target_mapping = [target_mapping] if target_mapping else []
return aspect(
Expand Down
17 changes: 17 additions & 0 deletions test/aspect/execute_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,23 @@
cmd=TestCmd(target="//test/aspect/recursion:dwyu_recursive_main"),
expected=ExpectedResult(success=False, unused_public_deps=["//test/aspect/recursion:e"]),
),
TestCase(
name="recursive_fail_transitively_using_implementation_deps",
cmd=TestCmd(
target="//test/aspect/recursion:use_impl_deps",
aspect="//test/aspect/recursion:aspect.bzl%recursive_impl_deps_aspect",
extra_args=["--experimental_cc_implementation_deps"],
),
expected=ExpectedResult(success=False, unused_public_deps=["//test/aspect/recursion:e"]),
),
TestCase(
name="rule_recursive_failure_using_implementation_deps",
cmd=TestCmd(
target="//test/aspect/recursion:dwyu_recursive_use_impl_deps",
extra_args=["--experimental_cc_implementation_deps"],
),
expected=ExpectedResult(success=False, unused_public_deps=["//test/aspect/recursion:e"]),
),
TestCase(
name="complex_includes",
cmd=TestCmd(target="//test/aspect/complex_includes:all", aspect=DEFAULT_ASPECT),
Expand Down
13 changes: 12 additions & 1 deletion test/aspect/recursion/BUILD
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
load("//test/aspect/recursion:rule.bzl", "dwyu_rule_direct", "dwyu_rule_recursive")
load("//test/aspect/recursion:rule.bzl", "dwyu_rule_direct", "dwyu_rule_recursive", "dwyu_rule_recursive_with_impl_deps")

# Has a diamond dependency towards C through A and B
# main specifies its dependencies correctly, but C has an error
Expand Down Expand Up @@ -51,3 +51,14 @@ cc_library(
name = "e",
hdrs = ["e.h"],
)

cc_library(
name = "use_impl_deps",
srcs = ["use_impl_deps.cpp"],
implementation_deps = [":a"],
)

dwyu_rule_recursive_with_impl_deps(
name = "dwyu_recursive_use_impl_deps",
deps = [":use_impl_deps"],
)
1 change: 1 addition & 0 deletions test/aspect/recursion/aspect.bzl
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
load("//:defs.bzl", "dwyu_aspect_factory")

recursive_aspect = dwyu_aspect_factory(recursive = True)
recursive_impl_deps_aspect = dwyu_aspect_factory(recursive = True, use_implementation_deps = True)
9 changes: 8 additions & 1 deletion test/aspect/recursion/rule.bzl
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
load("//test/aspect:aspect.bzl", "dwyu_default_aspect")
load("//test/aspect/recursion:aspect.bzl", "recursive_aspect")
load("//test/aspect/recursion:aspect.bzl", "recursive_aspect", "recursive_impl_deps_aspect")

def _dwyu_rule_impl(ctx):
# gather artifacts to make sure the aspect is executed
Expand All @@ -19,3 +19,10 @@ dwyu_rule_recursive = rule(
"deps": attr.label_list(aspects = [recursive_aspect]),
},
)

dwyu_rule_recursive_with_impl_deps = rule(
implementation = _dwyu_rule_impl,
attrs = {
"deps": attr.label_list(aspects = [recursive_impl_deps_aspect]),
},
)
6 changes: 6 additions & 0 deletions test/aspect/recursion/use_impl_deps.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
#include "test/aspect/recursion/a.h"

int doSth()
{
return 42;
}

0 comments on commit 09d0ab3

Please sign in to comment.