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.
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 031b066..18be4de 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -67,6 +67,10 @@
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 ff18887..20d79ba 100644
--- a/python/private/pypi/simpleapi_download.bzl
+++ b/python/private/pypi/simpleapi_download.bzl
@@ -71,16 +71,21 @@
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 @@
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 953df5c..9a6b7ca 100644
--- a/tests/pypi/simpleapi_download/simpleapi_download_tests.bzl
+++ b/tests/pypi/simpleapi_download/simpleapi_download_tests.bzl
@@ -23,13 +23,10 @@
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 @@
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 @@
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 @@
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 @@
""",
])
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 = {}
