Skip to content

Commit 9361a1a

Browse files
fix(pypi): resolve self-referencing extras to a real fixed point (bazel-contrib#4039)
## Summary `_resolve_extras` decides that its fixed-point loop has 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: ```python num_extras_before = len(extras) extras = extras | new_extras num_extras_after = len(new_extras) # the delta, not the merged size if num_extras_before == num_extras_after: break ``` Those are unrelated quantities, and the mismatch breaks in two separate ways. ### 1. Extras are silently dropped The loop exits early whenever the two counts happen to coincide while the set is still growing, so extras reachable only through a further round are never resolved —- and every dependency gated on them silently disappears from the generated target. The smallest reproducer is a two-hop self-extras chain: | `Requires-Dist` | requested | resolved today | expected | |---|---|---|---| | `foo[b]; extra == 'all'`, `foo[c]; extra == 'b'` | `foo[all]` | `{all, b}` | `{all, b, c}` | Anything behind `extra == 'c'` is lost, with no error. This is not limited to the first round: a chain that branches before it deepens (`all → {p, q}`, `p → r`, `r → t`) hits the same equality on round two and drops `t`. ### 2. The loop never terminates early for ordinary packages For a package with no self-referencing extras — the overwhelmingly common case — `self_reqs` is empty, so `new_extras` is always `{}` while `extras` holds at least one entry. The condition can never hold, 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: | Metric | Before | After | |---|---|---| | `_resolve_extras` self-time | 430–442 CPU-s | below profiler threshold | | Total Starlark user-function CPU | 747–759 CPU-s | 275–298 CPU-s | | Cold loading+analysis wall time (16 cores) | 82–84 s | 73–75 s | ## The fix Compare the size of the *merged* set, which is what the `before`/`after` naming already implied. The loop is monotonic, so the converged set is unchanged wherever it previously terminated correctly — this only stops it terminating too early, or not at all. ## Tests Three regression tests are added to `tests/pypi/pep508/deps_tests.bzl`, covering the three shapes that trigger the early exit: a two-hop chain, multiple requested extras, and a chain where the counts only coincide after the first round. All three fail on `main` with exactly the dropped dependency, and pass with the fix: ``` //tests/pypi/pep508:test_self_extras_chain_is_fully_resolved FAILED 1 missing: c_dep //tests/pypi/pep508:test_self_extras_chain_with_multiple_requested_extras FAILED 1 missing: c_dep //tests/pypi/pep508:test_self_extras_chain_resolved_beyond_the_first_round FAILED 1 missing: t_dep ``` The three existing tests that exercise self-extras chains (`test_self_is_ignored`, `test_self_dependencies_can_come_in_any_order`, `test_self_include_deps_from_previously_visited`) pass either way, which is why this went unnoticed. `bazel test //tests/pypi/...` is green (249/249) with the fix applied. ## Notes The condition was introduced in bazel-contrib#3527, which replaced the previous double loop with this fixed-point loop. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 36c342a commit 9361a1a

3 files changed

Lines changed: 87 additions & 2 deletions

File tree

‎news/4039.fixed.md‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
(pypi) Fixed the fixed-point loop that resolves self-referencing extras
2+
(`pkg[extra]` entries in a package's own `Requires-Dist`). The loop compared the
3+
number of extras discovered in the current round against the number known
4+
before it, rather than against the size of the merged set. As a result it could
5+
stop before every extra was resolved, silently dropping dependencies only
6+
reachable through two or more `pkg[extra]` hops, and for the common case of a
7+
package with no self-referencing extras it never converged at all, running all
8+
10000 rounds while evaluating each wheel's generated `BUILD` file
9+
([#4039](https://github.com/bazel-contrib/rules_python/pull/4039)).

‎python/private/pypi/pep508_deps.bzl‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -153,9 +153,15 @@ def _resolve_extras(self_name, reqs, extras):
153153

154154
num_extras_before = len(extras)
155155
extras = extras | new_extras
156-
num_extras_after = len(new_extras)
157156

158-
if num_extras_before == num_extras_after:
157+
# We have reached a fixed point once merging in the newly discovered
158+
# extras stops growing the set. Comparing against len(new_extras) here
159+
# compares the number of extras found in this round against the total
160+
# known before it, which are unrelated quantities: it stops early when
161+
# the two happen to be equal, dropping extras that are only reachable
162+
# through another round, and never triggers at all for the common case
163+
# of a package with no self-referencing extras.
164+
if num_extras_before == len(extras):
159165
break
160166

161167
# Poor mans set

‎tests/pypi/pep508/deps_tests.bzl‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,76 @@ def test_self_include_deps_from_previously_visited(env):
119119

120120
_tests.append(test_self_include_deps_from_previously_visited)
121121

122+
def test_self_extras_chain_is_fully_resolved(env):
123+
# 'all' pulls in 'b', which in turn pulls in 'c'. The first round discovers
124+
# exactly one new extra, which is also the number of extras known at the
125+
# start of the round, so a fixed point must not be declared yet.
126+
got = deps(
127+
"foo",
128+
requires_dist = [
129+
"bar",
130+
"foo[b]; extra == 'all'",
131+
"foo[c]; extra == 'b'",
132+
"b_dep; extra == 'b'",
133+
"c_dep; extra == 'c'",
134+
],
135+
extras = ["all"],
136+
)
137+
138+
env.expect.that_collection(got.deps).contains_exactly(["bar", "b_dep", "c_dep"])
139+
env.expect.that_dict(got.deps_select).contains_exactly({})
140+
141+
_tests.append(test_self_extras_chain_is_fully_resolved)
142+
143+
def test_self_extras_chain_with_multiple_requested_extras(env):
144+
# Two requested extras, and the first round discovers exactly two new ones
145+
# ('a' and 'b'), so the count of newly found extras again matches the number
146+
# known at the start of the round while 'c' is still undiscovered.
147+
got = deps(
148+
"foo",
149+
requires_dist = [
150+
"bar",
151+
"foo[a]; extra == 'x'",
152+
"foo[b]; extra == 'y'",
153+
"foo[c]; extra == 'a'",
154+
"a_dep; extra == 'a'",
155+
"b_dep; extra == 'b'",
156+
"c_dep; extra == 'c'",
157+
],
158+
extras = ["x", "y"],
159+
)
160+
161+
env.expect.that_collection(got.deps).contains_exactly(["bar", "a_dep", "b_dep", "c_dep"])
162+
env.expect.that_dict(got.deps_select).contains_exactly({})
163+
164+
_tests.append(test_self_extras_chain_with_multiple_requested_extras)
165+
166+
def test_self_extras_chain_resolved_beyond_the_first_round(env):
167+
# Here the counts only coincide on the second round: round one grows the set
168+
# from {all} to {all, p, q}, round two finds {p, q, r} -- three extras, which
169+
# matches the three known at the start of that round -- while 't' is only
170+
# reachable from 'r' on a third round.
171+
got = deps(
172+
"foo",
173+
requires_dist = [
174+
"bar",
175+
"foo[p]; extra == 'all'",
176+
"foo[q]; extra == 'all'",
177+
"foo[r]; extra == 'p'",
178+
"foo[t]; extra == 'r'",
179+
"p_dep; extra == 'p'",
180+
"q_dep; extra == 'q'",
181+
"r_dep; extra == 'r'",
182+
"t_dep; extra == 't'",
183+
],
184+
extras = ["all"],
185+
)
186+
187+
env.expect.that_collection(got.deps).contains_exactly(["bar", "p_dep", "q_dep", "r_dep", "t_dep"])
188+
env.expect.that_dict(got.deps_select).contains_exactly({})
189+
190+
_tests.append(test_self_extras_chain_resolved_beyond_the_first_round)
191+
122192
def _test_can_get_deps_based_on_specific_python_version(env):
123193
requires_dist = [
124194
"bar",

0 commit comments

Comments
 (0)