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",