diff --git a/make_profiler/parser.py b/make_profiler/parser.py index 59bfd86..efbd0a2 100644 --- a/make_profiler/parser.py +++ b/make_profiler/parser.py @@ -147,11 +147,11 @@ def parse_target(token: Tuple[Tokens, str]): rest = rest.strip() if '|' in rest: deps_part, order_part = rest.split('|', 1) - order_deps = sorted(order_part.strip().split()) if order_part.strip() else [] + order_deps = order_part.strip().split() if order_part.strip() else [] else: deps_part = rest order_deps = [] - deps = sorted(deps_part.strip().split()) if deps_part.strip() else [] + deps = deps_part.strip().split() if deps_part.strip() else [] body = parse_body() ast.append( @@ -192,9 +192,22 @@ def parse_body() -> List[Tuple[Tokens, str]]: def get_dependencies_influences(ast: List[Tuple[Tokens, Dict[str, Any]]]): + """Build the dependency graph consumed by :func:`export_dot`. + + Repeated explicit single-colon rules merge and deduplicate their + prerequisites in first-seen order. A prerequisite seen normally is + normal everywhere, even if another rule lists it as order-only. Pattern + rules remain alternatives, so their latest definition overwrites the + stored prerequisite lists rather than merging them. Both graph directions + describe that same stored alternative. + + Returns ``(dependencies, influences, order_only, indirect_influences)``. + ``order_only`` contains only prerequisites that were never normal; + ``export_dot`` uses it to hide otherwise-unconnected order-only nodes. + """ dependencies = {} + dependency_membership = {} influences = collections.defaultdict(set) - order_only = set() indirect_influences = collections.defaultdict(set) alias_map = {} @@ -209,6 +222,15 @@ def get_dependencies_influences(ast: List[Tuple[Tokens, Dict[str, Any]]]): def alias(name: str) -> str: return alias_map.get(name, name) + # A declared target remains a graph node even when a later pattern-rule + # alternative drops its last incoming edge. ``descendants`` below walks + # every node, so deleting such a node can mutate ``influences`` mid-walk. + declared_targets = { + alias(item['target']) + for item_t, item in ast + if item_t == Tokens.target and item['target'] != '.PHONY' + } + for item_t, item in ast: if item_t != Tokens.target: continue @@ -220,7 +242,45 @@ def alias(name: str) -> str: if target in ('.PHONY',): continue - dependencies[target] = [deps, order_deps] + # Pattern rules are alternatives, not repeated explicit rules. Keep + # the base overwrite representation instead of applying explicit-rule + # prerequisite accumulation to them. Remove reverse edges from the + # superseded alternative so critical-path traversal never reaches a + # dependency absent from the forward graph. + is_pattern_rule = any('%' in name for name in item.get('all_targets', [])) + if is_pattern_rule: + previous_deps, _previous_order_deps = dependencies.get(target, [[], []]) + for dep in set(previous_deps) - set(deps): + influences[dep].discard(target) + + normal_dep_names = set(deps) + order_deps = [dep for dep in order_deps if dep not in normal_dep_names] + dependencies[target] = [deps, order_deps] + influences[target] + for dep in deps: + influences[dep].add(target) + for dep in order_deps: + influences[dep] + continue + + # GNU Make combines prerequisites from repeated single-colon rules. + # Keep the reverse graph aligned with the forward influences below so + # critical-path traversal can return through every incoming edge. + existing_deps, existing_order_deps = dependencies.setdefault(target, [[], []]) + if target not in dependency_membership: + dependency_membership[target] = (set(existing_deps), set(existing_order_deps)) + known_dep_names, known_order_dep_names = dependency_membership[target] + for dep in deps: + if dep in known_order_dep_names: + known_order_dep_names.remove(dep) + if dep not in known_dep_names: + existing_deps.append(dep) + known_dep_names.add(dep) + for dep in order_deps: + if dep in known_dep_names or dep in known_order_dep_names: + continue + existing_order_deps.append(dep) + known_order_dep_names.add(dep) # influences influences[target] @@ -228,7 +288,40 @@ def alias(name: str) -> str: influences[k].add(target) for k in order_deps: influences[k] - order_only.update(order_deps) + + # Promotions above only update membership sets: removing an item from the + # ordered list for every promotion would repeatedly scan that list. Filter + # each target once instead, retaining the first-seen order of surviving + # order-only prerequisites. + for target, (_known_deps, known_order_deps) in dependency_membership.items(): + existing_order_deps = dependencies[target][1] + dependencies[target][1] = [ + dep for dep in existing_order_deps if dep in known_order_deps + ] + + # Classify prerequisites from the completed forward graph so a superseded + # pattern alternative cannot keep a stale normal or order-only node. + normal_dependencies = { + dependency + for normal_deps, _order_deps in dependencies.values() + for dependency in normal_deps + } + order_only = { + dependency + for _normal_deps, order_deps in dependencies.values() + for dependency in order_deps + } + order_only.difference_update(normal_dependencies) + + # Pattern replacements leave empty reverse nodes behind until the complete + # forward graph is known. Clean them once here instead of repeatedly + # rescanning all dependencies for every replacement. Declared targets and + # live normal/order-only prerequisites must remain graph nodes even if they + # have no outgoing influence edges. + live_nodes = declared_targets | normal_dependencies | order_only + for node in list(influences): + if not influences[node] and node not in live_nodes: + del influences[node] # Cache previously calculated descendants to avoid quadratic behaviour on # large graphs. ``descendants(target)`` returns a set of all nodes reachable @@ -250,4 +343,3 @@ def descendants(target): ) return dependencies, influences, order_only, indirect_influences - \ No newline at end of file diff --git a/tests/test_dot_export.py b/tests/test_dot_export.py index 0c3e232..f26bd37 100644 --- a/tests/test_dot_export.py +++ b/tests/test_dot_export.py @@ -1,5 +1,5 @@ import io -from make_profiler.dot_export import export_dot +from make_profiler.dot_export import critical_path, export_dot from make_profiler import parser def build_sample(): @@ -45,6 +45,357 @@ def test_critical_path_handles_final_targets(): assert 'subgraph cluster_tools' in data +def test_critical_path_handles_disconnected_components(): + """Disconnected Makefile components retain their complete critical paths.""" + ast = parser.parse(io.StringIO('all: input\ninput:\norphan:\n')) + dependencies, influences, _order_only, _indirect = parser.get_dependencies_influences(ast) + + critical, _ = critical_path(influences, dependencies, {'input', 'orphan'}, {}) + + assert critical == {'all', 'input', 'orphan'}, ( + "Disconnected components must propagate late starts through their own " + f"valid reverse edges; got {critical!r} from {dependencies!r}." + ) + + +def test_repeated_target_rules_merge_dependencies_for_critical_path(): + """Repeated GNU Make rules must preserve every dependency branch.""" + ast = parser.parse(io.StringIO('all: a\nall: b\na:\nb:\n')) + dependencies, influences, _order_only, _indirect = parser.get_dependencies_influences(ast) + + critical, _ = critical_path(influences, dependencies, {'a', 'b'}, {}) + + assert dependencies['all'] == [['a', 'b'], []], ( + "Repeated single-colon rules must combine normal prerequisites in the " + f"reverse graph; got {dependencies['all']!r}." + ) + assert critical == {'a', 'all', 'b'}, ( + "Both equally long branches of repeated target 'all' must stay " + f"critical; got {critical!r} from {dependencies!r}." + ) + + +def test_repeated_target_rules_preserve_dependency_order_and_deduplication(): + """Repeated GNU Make rules must keep first-seen prerequisite order.""" + ast = parser.parse( + io.StringIO( + 'all: c a | stamp\n' + 'all: b a | stamp cache\n' + 'a:\n' + 'b:\n' + 'c:\n' + 'stamp:\n' + 'cache:\n' + ) + ) + dependencies, influences, order_only, _indirect = parser.get_dependencies_influences(ast) + + assert dependencies['all'] == [['c', 'a', 'b'], ['stamp', 'cache']], ( + "Repeated target rules must deduplicate prerequisites while preserving " + f"their first-seen source order; got {dependencies['all']!r}." + ) + assert influences['a'] == {'all'}, ( + "Normal prerequisite 'a' should influence 'all' exactly once after " + f"deduplication; got {influences['a']!r}." + ) + assert influences['b'] == {'all'}, ( + "Normal prerequisite 'b' should remain connected to 'all'; got " + f"{influences['b']!r}." + ) + assert influences['c'] == {'all'}, ( + "Normal prerequisite 'c' should remain connected to 'all'; got " + f"{influences['c']!r}." + ) + assert 'all' not in influences['stamp'], ( + "Order-only prerequisite 'stamp' must not become a normal reverse " + f"edge; got {influences['stamp']!r}." + ) + assert order_only == {'stamp', 'cache'}, ( + "Order-only prerequisites must still be tracked separately; got " + f"{order_only!r}." + ) + + +def test_repeated_target_rules_preserve_source_order(): + """Repeated rules retain GNU Make's prerequisite encounter order.""" + ast = parser.parse(io.StringIO('all: z a\nall: y\nz:\na:\ny:\n')) + dependencies, _influences, _order_only, _indirect = parser.get_dependencies_influences(ast) + + assert dependencies['all'] == [['z', 'a', 'y'], []], ( + "Repeated target rules must retain source prerequisite order, matching " + f"GNU Make's z, a, y processing order; got {dependencies['all']!r}." + ) + + +def test_pattern_rule_alternatives_keep_graph_directions_consistent(): + """A stored pattern alternative must have matching forward and reverse edges.""" + ast = parser.parse(io.StringIO('%.o: %.c\n%.o: %.s\n')) + dependencies, influences, order_only, indirect = parser.get_dependencies_influences(ast) + + assert dependencies['%.o'] == [['%.s'], []], ( + "Pattern prerequisites must not be accumulated by the repeated-rule " + f"merge; got {dependencies['%.o']!r}." + ) + assert influences['%.s'] == {'%.o'}, ( + "The stored pattern alternative must retain its reverse edge; got " + f"{influences!r}." + ) + assert '%.c' not in influences, ( + "A superseded pattern alternative must not leave a reverse edge that " + f"is absent from dependencies; got {influences!r}." + ) + + f = io.StringIO() + export_dot(f, influences, dependencies, order_only, {}, indirect, {}) + data = f.getvalue() + + assert '"%.s" -> "%.o"' in data and '"%.c" -> "%.o"' not in data, ( + "DOT export must render only the stored alternative instead of a " + f"conjunctive pattern graph; got {data!r}." + ) + + +def test_pattern_order_only_prerequisite_is_registered_for_export(): + """Only the stored pattern alternative contributes order-only nodes.""" + ast = parser.parse( + io.StringIO( + '%.o: | obsolete\n' + '%.o: | stamp\n' + 'all: %.o | stamp\n' + 'stamp:\n' + ) + ) + dependencies, influences, order_only, _indirect = parser.get_dependencies_influences(ast) + + assert dependencies['%.o'] == [[], ['stamp']], ( + "The current pattern rule must retain its order-only prerequisite; " + f"got {dependencies['%.o']!r}." + ) + assert 'stamp' in influences and influences['stamp'] == set(), ( + "Pattern order-only prerequisites must be registered as graph nodes; " + f"got {influences!r}." + ) + assert order_only == {'stamp'}, ( + "Only the current pattern order-only prerequisite must remain " + f"globally classified; got {order_only!r} from {dependencies!r}." + ) + assert 'obsolete' not in influences, ( + "A superseded pattern order-only prerequisite must not remain in the " + f"graph; got influences={influences!r}." + ) + + f = io.StringIO() + export_dot(f, influences, dependencies, order_only, {}, _indirect, {}) + + +def test_pattern_normal_prerequisite_replaced_by_order_only_has_no_dot_edge(): + """A pattern prerequisite changing category must lose its normal edge.""" + ast = parser.parse( + io.StringIO('%.o: shared\n%.o: | shared\nall: %.o\nshared:\n') + ) + dependencies, influences, order_only, indirect = parser.get_dependencies_influences(ast) + + assert dependencies['%.o'] == [[], ['shared']], ( + "The stored pattern alternative must retain shared only as order-only; " + f"got {dependencies['%.o']!r}." + ) + assert influences['shared'] == set(), ( + "Replacing a normal pattern prerequisite with order-only must remove " + f"its reverse edge; got {influences!r}." + ) + assert order_only == {'shared'}, ( + "The replacement prerequisite must be globally order-only; got " + f"{order_only!r} from {dependencies!r}." + ) + + f = io.StringIO() + export_dot(f, influences, dependencies, order_only, {}, indirect, {}) + data = f.getvalue() + + assert 'shared -> "%.o"' not in data, ( + "DOT must not render a normal edge for an order-only pattern " + f"prerequisite; got {data!r}." + ) + + +def test_pattern_normal_prerequisite_wins_over_same_rule_order_only_entry(): + """A prerequisite cannot be both normal and order-only for one pattern.""" + ast = parser.parse(io.StringIO('%.o: shared | shared\nall: %.o\nshared:\n')) + dependencies, influences, order_only, _indirect = parser.get_dependencies_influences(ast) + + assert dependencies['%.o'] == [['shared'], []], ( + "A normal pattern prerequisite must remove its duplicate order-only " + f"entry; got dependencies={dependencies!r}." + ) + assert influences['shared'] == {'%.o'}, ( + "The shared prerequisite must retain exactly its normal reverse edge; " + f"got influences={influences!r}." + ) + assert order_only == set(), ( + "A normal pattern prerequisite cannot remain globally order-only; got " + f"order_only={order_only!r} from dependencies={dependencies!r}." + ) + + +def test_pattern_alternative_cleanup_keeps_cross_pattern_order_only_node(): + """Replacing one pattern alternative must not erase another pattern's node.""" + ast = parser.parse( + io.StringIO( + '%.a: | shared\n' + '%.b: shared\n' + '%.b: other\n' + 'all: %.a %.b\n' + 'other:\n' + ) + ) + dependencies, influences, order_only, indirect = parser.get_dependencies_influences(ast) + + assert dependencies['%.a'] == [[], ['shared']], ( + "The unrelated pattern's order-only prerequisite must survive replacing " + f"%.b's alternative; got dependencies={dependencies!r}." + ) + assert influences['shared'] == set(), ( + "A live order-only prerequisite needs an empty graph node even after " + f"another pattern drops its normal edge; got influences={influences!r}." + ) + assert order_only == {'shared'}, ( + "Only the still-live cross-pattern prerequisite should be classified " + f"order-only; got order_only={order_only!r}, dependencies={dependencies!r}." + ) + + f = io.StringIO() + export_dot(f, influences, dependencies, order_only, {}, indirect, {}) + data = f.getvalue() + + assert 'shared -> "%.a"' not in data and 'shared -> "%.b"' not in data, ( + "A prerequisite that is live only as order-only must not retain a normal " + f"edge from the superseded pattern alternative; got DOT={data!r}." + ) + + +def test_pattern_alternative_cleanup_keeps_declared_target_node(): + """Replacing an alternative must not remove a declared prerequisite node.""" + ast = parser.parse( + io.StringIO( + 'src:\n' + 'foo: src\n' + '%.a: foo\n' + '%.a: bar\n' + 'all: %.a\n' + 'bar:\n' + ) + ) + dependencies, influences, order_only, indirect = parser.get_dependencies_influences(ast) + + assert dependencies['%.a'] == [['bar'], []], ( + "The latest pattern alternative must replace the old prerequisite; got " + f"dependencies={dependencies!r}." + ) + assert influences['foo'] == set(), ( + "A declared target must remain an empty node after its stale pattern " + f"edge is removed; got influences={influences!r}." + ) + + f = io.StringIO() + export_dot(f, influences, dependencies, order_only, {}, indirect, {}) + data = f.getvalue() + + assert 'foo -> "%.a"' not in data, ( + "The superseded normal edge must not be exported; got DOT={data!r}." + ) + assert 'foo [label=foo' in data, ( + "The declared target must remain exportable after pattern replacement; " + f"got DOT={data!r}." + ) + + +def test_normal_pattern_prerequisite_dominates_explicit_order_only_export(): + """A normal pattern prerequisite must never be globally order-only.""" + ast = parser.parse(io.StringIO('%.o: shared\nall: %.o | shared\nshared:\n')) + dependencies, influences, order_only, indirect = parser.get_dependencies_influences(ast) + + assert order_only == set(), ( + "A normal pattern prerequisite must promote the shared prerequisite " + f"globally; got order_only={order_only!r} for {dependencies!r}." + ) + + f = io.StringIO() + export_dot(f, influences, dependencies, order_only, {}, indirect, {}) + data = f.getvalue() + + assert 'subgraph cluster_order_only' not in data, ( + "export_dot must not emit an order-only cluster once 'shared' is " + f"promoted by the pattern rule; got {data!r}." + ) + assert 'shared [label=shared' in data, ( + "export_dot must retain the promoted prerequisite as a visible node; " + f"got {data!r}." + ) + + +def test_normal_prerequisite_dominates_order_only_on_repeated_rules(): + """GNU Make treats an overlapping normal prerequisite as normal.""" + ast = parser.parse(io.StringIO('all: input | stamp\nall: stamp\ninput:\nstamp:\n')) + dependencies, influences, order_only, _indirect = parser.get_dependencies_influences(ast) + + assert dependencies['all'] == [['input', 'stamp'], []], ( + "Normal prerequisites must absorb an overlapping order-only entry; " + f"got {dependencies['all']!r}." + ) + assert order_only == set(), ( + "A prerequisite promoted to normal must not remain order-only; got " + f"{order_only!r}." + ) + assert influences['stamp'] == {'all'}, ( + "The promoted prerequisite must retain its normal reverse edge; got " + f"{influences['stamp']!r}." + ) + + +def test_order_only_promotions_preserve_remaining_first_seen_order(): + """Promotion must filter order-only prerequisites without reordering survivors.""" + ast = parser.parse( + io.StringIO( + 'all: | alpha beta gamma\n' + 'all: beta zeta | delta\n' + 'alpha:\n' + 'beta:\n' + 'gamma:\n' + 'zeta:\n' + 'delta:\n' + ) + ) + dependencies, influences, order_only, _indirect = parser.get_dependencies_influences(ast) + + assert dependencies['all'] == [['beta', 'zeta'], ['alpha', 'gamma', 'delta']], ( + "Promoted 'beta' must move to normal prerequisites while remaining " + "order-only prerequisites retain their first-seen order; got " + f"{dependencies['all']!r}." + ) + assert influences['beta'] == {'all'}, ( + "Promoted 'beta' must provide one normal reverse edge; got " + f"{influences['beta']!r}." + ) + assert order_only == {'alpha', 'gamma', 'delta'}, ( + "Only unpromoted prerequisites may remain globally order-only; got " + f"{order_only!r}." + ) + + +def test_critical_path_keeps_shorter_branch_noncritical(): + """A complete reverse graph must not make genuinely shorter paths critical.""" + ast = parser.parse(io.StringIO('all: slow fast\nslow:\nfast:\n')) + dependencies, influences, _order_only, _indirect = parser.get_dependencies_influences(ast) + timing = {'slow': {'timing_sec': 5}, 'fast': {'timing_sec': 1}} + + critical, _ = critical_path(influences, dependencies, {'slow', 'fast'}, timing) + + assert critical == {'all', 'slow'}, ( + "Only the longest branch should be critical after reverse-edge " + f"propagation; got {critical!r} with timing {timing!r}." + ) + + def test_example_makefile_from_readme(): with open('test/example.mk', encoding='utf-8') as fh: ast = parser.parse(fh)