From 4d17a1c96e1b1027fb0aef59ca4dcf2b8eaeb6a1 Mon Sep 17 00:00:00 2001 From: Ignas Anikevicius <240938+aignas@users.noreply.github.com> Date: Sun, 15 Mar 2026 13:59:53 +0900 Subject: [PATCH] fix(pypi): propagate fails if overrides are passed only one index is used Before this PR there would be confusing failures when downloader config is set to disallow certain values or when the authentication is not setup properly. This is a small fix towards a better goal state where we set `allow_fail = False` in cases where we know that we have to succeed to download metadata from that particular URL. The use-cases covered: - Only one index_url is passed to `pip.parse`. - `index_url_overrides` are passed which means that we should fail if there are insufficient overrides. The downside to this is that it is really hard to return custom error messages telling the user what to do, but on the flip side, the failures coming from bazel itself might be more descriptive in the case of outh-misconfiguration or bazel downloader configuration settings. Work towards #2632 and #3260. --- CHANGELOG.md | 4 + python/private/pypi/simpleapi_download.bzl | 14 +-- .../simpleapi_download_tests.bzl | 87 +++++++++++++++++-- 3 files changed, 92 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e5748704b6..ac5c8146df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -67,6 +67,10 @@ END_UNRELEASED_TEMPLATE Other changes: * (pypi) Update dependencies used for `compile_pip_requirements`, building sdists in the `whl_library` rule and fetching wheels using `pip`. +* (pypi) We will set `allow_fail` to `False` if the {attr}`experimental_index_url_overrides` is set + to a non-empty value. This means that failures will be no-longer cached in this particular case. + ([#3260](https://github.com/bazel-contrib/rules_python/issues/3260) and + [#2632](https://github.com/bazel-contrib/rules_python/issues/2632)) {#v0-0-0-fixed} ### Fixed diff --git a/python/private/pypi/simpleapi_download.bzl b/python/private/pypi/simpleapi_download.bzl index ff18887ec1..20d79ba9b4 100644 --- a/python/private/pypi/simpleapi_download.bzl +++ b/python/private/pypi/simpleapi_download.bzl @@ -71,16 +71,21 @@ def simpleapi_download( for p, i in (attr.index_url_overrides or {}).items() } - download_kwargs = {} - if bazel_features.external_deps.download_has_block_param: - download_kwargs["block"] = not parallel_download - # NOTE @aignas 2024-03-31: we are not merging results from multiple indexes # to replicate how `pip` would handle this case. contents = {} index_urls = [attr.index_url] + attr.extra_index_urls read_simpleapi = read_simpleapi or _read_simpleapi + download_kwargs = {} + if bazel_features.external_deps.download_has_block_param: + download_kwargs["block"] = not parallel_download + + if len(index_urls) == 1 or index_url_overrides: + download_kwargs["allow_fail"] = False + else: + download_kwargs["allow_fail"] = True + input_sources = attr.sources found_on_index = {} @@ -225,7 +230,6 @@ def _read_simpleapi(ctx, url, attr, cache, versions, get_auth = None, **download url = [real_url], output = output, auth = get_auth(ctx, [real_url], ctx_attr = attr), - allow_fail = True, **download_kwargs ) diff --git a/tests/pypi/simpleapi_download/simpleapi_download_tests.bzl b/tests/pypi/simpleapi_download/simpleapi_download_tests.bzl index 953df5c107..e43041e9b1 100644 --- a/tests/pypi/simpleapi_download/simpleapi_download_tests.bzl +++ b/tests/pypi/simpleapi_download/simpleapi_download_tests.bzl @@ -23,13 +23,14 @@ _tests = [] def _test_simple(env): calls = [] - def read_simpleapi(ctx, url, versions, attr, cache, get_auth, block): + def read_simpleapi(ctx, url, versions, attr, cache, get_auth, block, allow_fail): _ = ctx # buildifier: disable=unused-variable _ = attr _ = cache _ = get_auth _ = versions env.expect.that_bool(block).equals(False) + env.expect.that_bool(allow_fail).equals(True) calls.append(url) if "foo" in url and "main" in url: return struct( @@ -96,13 +97,14 @@ def _test_fail(env): calls = [] fails = [] - def read_simpleapi(ctx, url, versions, attr, cache, get_auth, block): + def read_simpleapi(ctx, url, versions, attr, cache, get_auth, block, allow_fail): _ = ctx # buildifier: disable=unused-variable _ = attr _ = cache _ = get_auth _ = versions env.expect.that_bool(block).equals(False) + env.expect.that_bool(allow_fail).equals(True) calls.append(url) if "foo" in url: return struct( @@ -130,9 +132,7 @@ def _test_fail(env): report_progress = lambda _: None, ), attr = struct( - index_url_overrides = { - "foo": "invalid", - }, + index_url_overrides = {}, index_url = "main", extra_index_urls = ["extra"], sources = {"bar": None, "baz": None, "foo": None}, @@ -149,7 +149,7 @@ def _test_fail(env): Failed to download metadata of the following packages from urls: { "bar": ["main", "extra"], - "foo": "invalid", + "foo": ["main", "extra"], } If you would like to skip downloading metadata for these packages please add 'simpleapi_skip=[ @@ -159,15 +159,86 @@ If you would like to skip downloading metadata for these packages please add 'si """, ]) env.expect.that_collection(calls).contains_exactly([ - "invalid/foo/", + "main/foo/", "main/bar/", "main/baz/", - "invalid/foo/", + "extra/foo/", "extra/bar/", ]) _tests.append(_test_fail) +def _test_allow_fail_single_index(env): + calls = [] + fails = [] + + def read_simpleapi(ctx, *, url, versions, attr, cache, get_auth, block, allow_fail): + _ = ctx # buildifier: disable=unused-variable + _ = attr + _ = cache + _ = get_auth + _ = versions + env.expect.that_bool(block).equals(False) + env.expect.that_bool(allow_fail).equals(False) + calls.append(url) + return struct( + output = struct( + sdists = {"deadbeef": url.strip("/").split("/")[-1]}, + whls = {"deadb33f": url.strip("/").split("/")[-1]}, + sha256s_by_version = {"fizz": url.strip("/").split("/")[-1]}, + ), + success = True, + ) + + contents = simpleapi_download( + ctx = struct( + getenv = {}.get, + report_progress = lambda _: None, + ), + attr = struct( + index_url_overrides = { + "foo": "extra", + }, + index_url = "main", + extra_index_urls = [], + sources = {"bar": None, "baz": None, "foo": None}, + envsubst = [], + ), + cache = pypi_cache(), + parallel_download = True, + read_simpleapi = read_simpleapi, + _fail = fails.append, + ) + + env.expect.that_collection(fails).contains_exactly([]) + env.expect.that_collection(calls).contains_exactly([ + "main/bar/", + "main/baz/", + "extra/foo/", + ]) + env.expect.that_dict(contents).contains_exactly({ + "bar": struct( + index_url = "main/bar/", + sdists = {"deadbeef": "bar"}, + sha256s_by_version = {"fizz": "bar"}, + whls = {"deadb33f": "bar"}, + ), + "baz": struct( + index_url = "main/baz/", + sdists = {"deadbeef": "baz"}, + sha256s_by_version = {"fizz": "baz"}, + whls = {"deadb33f": "baz"}, + ), + "foo": struct( + index_url = "extra/foo/", + sdists = {"deadbeef": "foo"}, + sha256s_by_version = {"fizz": "foo"}, + whls = {"deadb33f": "foo"}, + ), + }) + +_tests.append(_test_allow_fail_single_index) + def _test_download_url(env): downloads = {}