Add fallback to default url #433
Conversation
|
This could lead to non-deterministic versions of bazel being used, right? |
The fallback will use the same version as the version in failed downloading |
|
But the binary could be different? Unless I'm misreading and it does the same sha check at both locations, then disregard my comments. |
Yeah, I see what you mean. I have updated the code, do you think it is correct this time? I assume the bazel file downloaded from the default url will not be corrupted. |
| expected_hash, actual_hash | ||
| ) | ||
| ) | ||
| # Exiting with a special exit code not used by Bazel, so the calling process may retry based on that. | ||
| # https://docs.bazel.build/versions/0.21.0/guide.html#what-exit-code-will-i-get | ||
| sys.exit(22) |
There was a problem hiding this comment.
Why remove this exit code? It seems like we need it in case the caller retries on this exit code
| @@ -324,16 +338,22 @@ def download_bazel_into_directory(version, is_commit, directory): | |||
| sha256_hash.update(byte_block) | |||
| actual_hash = sha256_hash.hexdigest() | |||
| if actual_hash != expected_hash: | |||
| os.remove(destination_path) | |||
There was a problem hiding this comment.
Why keep the destination_path folder? Doesn't this mean the binary binary downloaded is corrupted?
We should probably remove the folder and create it again to download the fallback URL
| download(fallback_url+".sha256", sha256_path) | ||
| os.remove(destination_path) | ||
| download(fallback_url,destination_path) | ||
| os.chmod(destination_path, 0o755) |
There was a problem hiding this comment.
If the download fails for the exit URL, we should probably exit with code 22.
bazelisk.py
Outdated
| download(fallback_url+".sha256", sha256_path) | ||
| os.remove(destination_path) | ||
| download(fallback_url,destination_path) |
There was a problem hiding this comment.
What if the fallback URL download fail? Should we have a try catch to raise an exception as well?
|
@brentleyjones Can you please review this PR? |
bazelisk.py
Outdated
| @@ -306,6 +306,8 @@ def download_bazel_into_directory(version, is_commit, directory): | |||
|
|
|||
| sha256_path = destination_path + ".sha256" | |||
| expected_hash = "" | |||
| matcher=re.compile(r"(\d*\.\d*(?:\.\d*)?)(rc\d+)?") | |||
There was a problem hiding this comment.
Can you please format the changes? I think running black should work.
There was a problem hiding this comment.
Have run black to reformat. Can you please take a look? Thanks!
There was a problem hiding this comment.
@fweikert Hi, is there anything else I need to do for this PR? Appreciate a lot if could be merged.
uberzzr
force-pushed
the
addFallbackToDefaultUrl
branch
from
3b8682f
to
2e46381
Compare
When BAZELISK_BASE_URL is set and bazelisk fails to download Bazel from that URL, it would either throw an HTTPError or exit with 22. It should instead fallback to the default releases.bazel.build url.
Fixes #431