fix(pypi): propagate fails if overrides are passed only one index is used (#3666)

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.

Author: aignas

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

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
     )
 

tests/pypi/simpleapi_download/simpleapi_download_tests.bzl

@@ -23,13 +23,10 @@ _tests = []
 def _test_simple(env):
     calls = []
 
-    def read_simpleapi(ctx, url, versions, attr, cache, get_auth, block):
-        _ = ctx  # buildifier: disable=unused-variable
-        _ = attr
-        _ = cache
-        _ = get_auth
-        _ = versions
+    def read_simpleapi(ctx, url, versions, attr, cache, get_auth, block, allow_fail):
+        _ = ctx, attr, cache, get_auth, versions  # buildifier: disable=unused-variable
         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 +93,10 @@ def _test_fail(env):
     calls = []
     fails = []
 
-    def read_simpleapi(ctx, url, versions, attr, cache, get_auth, block):
-        _ = ctx  # buildifier: disable=unused-variable
-        _ = attr
-        _ = cache
-        _ = get_auth
-        _ = versions
+    def read_simpleapi(ctx, url, versions, attr, cache, get_auth, block, allow_fail):
+        _ = ctx, attr, cache, get_auth, versions  # buildifier: disable=unused-variable
         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 +124,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 +141,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 +151,82 @@ 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, attr, cache, get_auth, versions  # buildifier: disable=unused-variable
+        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 = {}