bazel-gazelle
bazel-gazelle copied to clipboard
bazelbuild
go_repository rule restarts
What version of gazelle are you using?
0.24.0
What version of rules_go are you using?
0.30.0
What version of Bazel are you using?
5.0.0
Does this issue reproduce with the latest releases of all the above?
Yes
What operating system and processor architecture are you using?
Linux/x64
What did you do?
Clean rebuild of a large project.
What did you expect to see?
No messages about restarting fetches. Or at least none that last long enough to be visible.
What did you see instead?
Messages like
Fetching @com_github_aws_aws_sdk_go; Restarting.
From my own experience writing repository rules, there's a little bit of a gotcha which is that calls to ctx.path() on a label referencing files in a repository which hasn't been fetched yet will result in restarting execution of your rule, as documented though not super clearly and not really emphasized.
In particular, lines like https://github.com/bazelbuild/bazel-gazelle/blob/4d2e96a20ad1a32b48bd2dc5c44d0627d34063bf/internal/go_repository.bzl#L150 or https://github.com/bazelbuild/bazel-gazelle/blob/4d2e96a20ad1a32b48bd2dc5c44d0627d34063bf/internal/go_repository.bzl#L194 will result in a restart, because as the docs say
To avoid unnecessary restarts (which are expensive, as network access might have to be repeated), label arguments are prefetched, provided all label arguments can be resolved to an existing file. Note that resolving a path from a string or a label that was constructed only during execution of the function might still cause a restart.
That's not great, because that means re-running the fetch from https://github.com/bazelbuild/bazel-gazelle/blob/4d2e96a20ad1a32b48bd2dc5c44d0627d34063bf/internal/go_repository.bzl#L102 (or from other code paths).
Possible workarounds for this are to pre-fetch the result of ctx.path() at the beginning of the rule implementation function, before anything more expensive has happened, and/or for static strings add them as private attributes to the rule so that bazel can know to fetch them prior to beginning execution of the rule.
Yep, we had a bad de-opt in rules_nodejs that was similar: https://github.com/bazelbuild/rules_nodejs/issues/2620
Do you have a bazel profile like in that issue, showing the severity of this? might help to get attention on it.
Honestly we don't change our dependency versions often enough for it to be a big problem (except maybe in some of the CI builders which don't keep a persistent cache), and most go dependencies are relatively small downloads anyway (especially by comparison to nodejs, where restarting means rerunning {yarn,npm} install from scratch for the entire dependency tree as an atomic operation). Mostly pointing the issue out because it's annoyingly unnecessary, and relatively simple to fix by just moving most of the ctx.path calls to the start of the rule implementation (though that's not so easy in every case).
@adam-azarchs thanks for reporting the issue. I played around with this quite a bit, and I do not think there is much we can do to avoid the restarts. Even when I declare every Label usage as arguments to the repository rules I still see "Restarting" after running bazel sync.
Based on this statement:
To avoid unnecessary restarts (which are expensive, as network access might have to be repeated), label arguments are prefetched, provided all label arguments can be resolved to an existing file.
I am assuming because these dependencies "can not be resolved to an existing file" Bazel goes on to run the implementation for each rule, explaining why we see the "Restarting". Also the "Restarting" goes on for ~30 seconds because @go_sdk and @go_repository_tools dependencies can take a while to fetch (one downloads the whole go toolchain, the other uses go build to build Gazelle and some other tools). Fortunately, these deps should be invalidated very infrequently.
However, I did make sure no expensive computation/fetching is occurring during the "Restarts" and put out #1206 to resolve the only issue that I saw.
It seems to me like think that documentation is misleading. Pretty much any use of ctx.path seems to cause a restart, even if the argument you pass is a declared input.
@linzhp The current implementation of go_repository looks like it would cause a restart when applying patches: It calls patch(ctx) in the very end, but that ends up calling ctx.path on the patch files. We should fix this by converting all relevant attributes to paths in the beginning.
