fix(pypi): allow index probes to fail on root path#3770
Conversation
There was a problem hiding this comment.
Code Review
This pull request modifies the _get_dist_urls function in simpleapi_download.bzl to allow download failures by passing allow_fail = True and skipping unsuccessful results. A review comment correctly identifies that this change will break existing tests because the mock functions used in the test suite do not yet accept the allow_fail keyword argument, requiring an update to the mock signatures.
| parse_index = True, | ||
| versions = {pkg: None for pkg in sources}, | ||
| block = block, | ||
| allow_fail = True, |
There was a problem hiding this comment.
Adding allow_fail = True here will cause existing tests to fail. The mock read_simpleapi functions in tests/pypi/simpleapi_download/simpleapi_download_tests.bzl (specifically in _test_simple and _test_index_overrides) do not accept the allow_fail keyword argument.
You should update those mock signatures to include **kwargs or explicitly accept allow_fail to maintain compatibility with this change.
There was a problem hiding this comment.
Requesting a better solution if it exists, and if the fix is not obvious, or simple, I am happy with closing this (tbh low effort) PR. I created this because this worked for our use case.
There was a problem hiding this comment.
Another fix could be to just not call the index at all if there is only one index url given.
In rules_python v2.0.0, the module extension introduced a step to fetch the list of available packages from each index to create a mapping and optimize downloads. This is implemented in
python/private/pypi/simpleapi_download.bzlin_get_dist_urls. It unconditionally attempts to download the root page of every index inindex_urls(withparse_index = True).If an extra index does not support root listing (e.g., it's just a file server hosting wheels and returns 404 Not Found for the root path), the build fails with an IOException.
This change passes
allow_fail = Truetoread_simpleapiwhen probing indexes, and checksresult.successbefore accessingresult.output, skipping failed indexes.Fixes #3769