From 18cd4c6fc36ff3a4ba57614c7d23fedef0f40700 Mon Sep 17 00:00:00 2001 From: Vasilii Muravev Date: Wed, 12 Aug 2026 10:12:12 +0000 Subject: [PATCH] fix(pypi): resolve self-referencing extras to a real fixed point The loop in _resolve_extras that resolves `pkg[extra]` entries from a package's own Requires-Dist decided it had converged by comparing num_extras_before -- the size of the extras set at the start of the round -- against len(new_extras), the number of extras discovered during that round. Those are unrelated quantities, and the mismatch breaks in two separate ways. It stops early whenever the two happen to be equal while the set is still growing, so extras reachable only through a further round are never resolved and every dependency gated on them is silently dropped. The smallest case is a two-hop chain: given `foo[b]; extra == 'all'` and `foo[c]; extra == 'b'`, requesting `foo[all]` resolves to {all, b} and loses everything behind `extra == 'c'`. This is not limited to the first round -- a chain that branches before it deepens hits the same equality later. Conversely, for a package with no self-referencing extras -- the overwhelmingly common case -- new_extras is always empty while the set holds at least one entry, so the condition never holds and the loop runs all 10000 rounds, allocating a dict each time, while evaluating the generated BUILD file of every wheel in the build. On a ~52k-package repository this dominated loading-phase Starlark CPU: _resolve_extras alone accounted for 430-442 CPU-s, and total Starlark user-function CPU fell from 747-759 CPU-s to 275-298 CPU-s once fixed, worth roughly 11% of cold loading+analysis wall time on a 16-core machine. Compare the size of the merged set instead, which is what the before/after naming already implied. The loop is monotonic, so the converged set is unchanged wherever it previously terminated correctly. The condition was introduced in #3527. The three existing tests that exercise self-extras chains pass either way, so this also adds regression tests for the three shapes that trigger the early exit. Co-Authored-By: Claude Opus 5 (1M context) --- news/4039.fixed.md | 9 ++++ python/private/pypi/pep508_deps.bzl | 10 ++++- tests/pypi/pep508/deps_tests.bzl | 70 +++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 2 deletions(-) create mode 100644 news/4039.fixed.md diff --git a/news/4039.fixed.md b/news/4039.fixed.md new file mode 100644 index 0000000000..d48ad91b24 --- /dev/null +++ b/news/4039.fixed.md @@ -0,0 +1,9 @@ +(pypi) Fixed the fixed-point loop that resolves self-referencing extras +(`pkg[extra]` entries in a package's own `Requires-Dist`). The loop compared the +number of extras discovered in the current round against the number known +before it, rather than against the size of the merged set. As a result it could +stop before every extra was resolved, silently dropping dependencies only +reachable through two or more `pkg[extra]` hops, and for the common case of a +package with no self-referencing extras it never converged at all, running all +10000 rounds while evaluating each wheel's generated `BUILD` file +([#4039](https://github.com/bazel-contrib/rules_python/pull/4039)). diff --git a/python/private/pypi/pep508_deps.bzl b/python/private/pypi/pep508_deps.bzl index fd6d961bf5..e36ceadbd5 100644 --- a/python/private/pypi/pep508_deps.bzl +++ b/python/private/pypi/pep508_deps.bzl @@ -153,9 +153,15 @@ def _resolve_extras(self_name, reqs, extras): num_extras_before = len(extras) extras = extras | new_extras - num_extras_after = len(new_extras) - if num_extras_before == num_extras_after: + # We have reached a fixed point once merging in the newly discovered + # extras stops growing the set. Comparing against len(new_extras) here + # compares the number of extras found in this round against the total + # known before it, which are unrelated quantities: it stops early when + # the two happen to be equal, dropping extras that are only reachable + # through another round, and never triggers at all for the common case + # of a package with no self-referencing extras. + if num_extras_before == len(extras): break # Poor mans set diff --git a/tests/pypi/pep508/deps_tests.bzl b/tests/pypi/pep508/deps_tests.bzl index e88acb8c56..0d171d8e95 100644 --- a/tests/pypi/pep508/deps_tests.bzl +++ b/tests/pypi/pep508/deps_tests.bzl @@ -119,6 +119,76 @@ def test_self_include_deps_from_previously_visited(env): _tests.append(test_self_include_deps_from_previously_visited) +def test_self_extras_chain_is_fully_resolved(env): + # 'all' pulls in 'b', which in turn pulls in 'c'. The first round discovers + # exactly one new extra, which is also the number of extras known at the + # start of the round, so a fixed point must not be declared yet. + got = deps( + "foo", + requires_dist = [ + "bar", + "foo[b]; extra == 'all'", + "foo[c]; extra == 'b'", + "b_dep; extra == 'b'", + "c_dep; extra == 'c'", + ], + extras = ["all"], + ) + + env.expect.that_collection(got.deps).contains_exactly(["bar", "b_dep", "c_dep"]) + env.expect.that_dict(got.deps_select).contains_exactly({}) + +_tests.append(test_self_extras_chain_is_fully_resolved) + +def test_self_extras_chain_with_multiple_requested_extras(env): + # Two requested extras, and the first round discovers exactly two new ones + # ('a' and 'b'), so the count of newly found extras again matches the number + # known at the start of the round while 'c' is still undiscovered. + got = deps( + "foo", + requires_dist = [ + "bar", + "foo[a]; extra == 'x'", + "foo[b]; extra == 'y'", + "foo[c]; extra == 'a'", + "a_dep; extra == 'a'", + "b_dep; extra == 'b'", + "c_dep; extra == 'c'", + ], + extras = ["x", "y"], + ) + + env.expect.that_collection(got.deps).contains_exactly(["bar", "a_dep", "b_dep", "c_dep"]) + env.expect.that_dict(got.deps_select).contains_exactly({}) + +_tests.append(test_self_extras_chain_with_multiple_requested_extras) + +def test_self_extras_chain_resolved_beyond_the_first_round(env): + # Here the counts only coincide on the second round: round one grows the set + # from {all} to {all, p, q}, round two finds {p, q, r} -- three extras, which + # matches the three known at the start of that round -- while 't' is only + # reachable from 'r' on a third round. + got = deps( + "foo", + requires_dist = [ + "bar", + "foo[p]; extra == 'all'", + "foo[q]; extra == 'all'", + "foo[r]; extra == 'p'", + "foo[t]; extra == 'r'", + "p_dep; extra == 'p'", + "q_dep; extra == 'q'", + "r_dep; extra == 'r'", + "t_dep; extra == 't'", + ], + extras = ["all"], + ) + + env.expect.that_collection(got.deps).contains_exactly(["bar", "p_dep", "q_dep", "r_dep", "t_dep"]) + env.expect.that_dict(got.deps_select).contains_exactly({}) + +_tests.append(test_self_extras_chain_resolved_beyond_the_first_round) + def _test_can_get_deps_based_on_specific_python_version(env): requires_dist = [ "bar",