fix(pypi): resolve self-referencing extras to a real fixed point (#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 #3527, which replaced the previous
double loop with this fixed-point loop.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>diff --git a/news/4039.fixed.md b/news/4039.fixed.md
new file mode 100644
index 0000000..d48ad91
--- /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 fd6d961..e36cead 100644
--- a/python/private/pypi/pep508_deps.bzl
+++ b/python/private/pypi/pep508_deps.bzl
@@ -153,9 +153,15 @@
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 e88acb8..0d171d8 100644
--- a/tests/pypi/pep508/deps_tests.bzl
+++ b/tests/pypi/pep508/deps_tests.bzl
@@ -119,6 +119,76 @@
_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",
