Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: use http_archive instead of git_repository #490

Merged
merged 1 commit into from
Oct 4, 2019
Merged

Conversation

goaway
Copy link
Contributor

@goaway goaway commented Oct 4, 2019

Description: This will hopefully allow for both faster and more reliable builds. http_archive contains built-in retry logic, and pulls down smaller artifacts (no history).
Risk Level: Low
Testing: Local and CI

Signed-off-by: Mike Schore [email protected]

@keith
Copy link
Member

keith commented Oct 4, 2019

FWIW git_repository also doesn't pull down history https://github.com/bazelbuild/bazel/blob/49ea9589f0255b0250cb4438cab88eceb91be62d/tools/build_defs/repo/git_worker.bzl#L52 and there is 1 (mostly theoretical but has happened once before) risk with using http_archive. The problem is that GitHub's tars are generated as needed, so when you lock the sha, if they update their version of tar, the sha could change. This happened at one point when they did an OS update on their machines. But realistically it's probably fine to ignore this and fix it if that happens.

@goaway
Copy link
Contributor Author

goaway commented Oct 4, 2019

Good points. Even if we are avoiding pulling down history (though my read of that code is that we're maybe still pulling down some unless shallow_since is set really precisely?), the main win (hopefully) here will be the built-in retry logic in http_archive helping us around the connection flakiness we've seen in azure/github actions.

Good callout on the potential sha issue. Something to keep an eye out for.

@keith
Copy link
Member

keith commented Oct 4, 2019

We should be setting shallow_since because otherwise Bazel warns too. But I think this is safe to test!

@@ -40,11 +39,11 @@ http_file(
urls = ["https://github.com/google/xctestrunner/releases/download/0.2.10/ios_test_runner.par"],
)

git_repository(
http_archive(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend not switching to http_archive without updating Bazel to 0.29.x, there were important connection/stream retry fixes in 0.29.x:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! We're on 0.29.1 :)

I did consider listing the same URL multiple times, but read somewhere that it would retry (like 8 times?) regardless. (I didn't read the current source to confirm this.)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, didn't notice you've updated Bazel yesterday lol

Listing same URL multiple times might work, but I also haven't checked if it dedups them before attempting the download

@goaway
Copy link
Contributor Author

goaway commented Oct 4, 2019

Thank you @keith and @artem-zinnatullin!

@goaway goaway merged commit 18a67e2 into master Oct 4, 2019
@goaway goaway deleted the ms/http-archive branch October 4, 2019 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants