-
Notifications
You must be signed in to change notification settings - Fork 380
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
update-repos: add -parse_only flag #1837
base: master
Are you sure you want to change the base?
update-repos: add -parse_only flag #1837
Conversation
a5ca20f
to
bb80a30
Compare
63f2e8c
to
7421582
Compare
@fmeum this is failing with
which seems to be a rules_go issue? |
7421582
to
a718352
Compare
rules_go only depends on rules_proto if you use proto functionality, which is why it was decided at some point to not provide this dep implicitly. You may have to update to 6.0.0 manually. |
I think that's a bit strange to make a non-dev dep "optional". So I don't see a reason why rules_proto needed to be excluded here. |
6eaf515
to
a5bb8f6
Compare
a5bb8f6
to
9eb54f4
Compare
Also bump minimum go version to 1.20 and update vendor dir with `go mod vendor`.
9eb54f4
to
52147da
Compare
Adding it to the macro doesn't really help that much as the WORKSPACE setup for rules_proto requires the user to call three dep macros, which we can't do for them: https://github.com/bazelbuild/rules_proto/releases/tag/6.0.2 I would thus prefer to just leave this as is as the real solution is a (partial) migration to Bzlmod. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sluongng Could we separate out all of the dependency changes to a different PR which we merge first?
This is a bit difficult to review, and might cause unexpected downstream behavior.
I would also like to avoid making any upgrades in go.mod
file that aren't absolutely necessary.
Oh, I just saw #1838, let me take a look there. It seems like there's some CI issues |
Yeah I have separated it to #1838. Let's discuss it there. |
52147da
to
f4130c6
Compare
f4130c6
to
567d0bc
Compare
Based on #1838 so please review that first.
This PR adds a
-parse_only
flag togazelle update-repos
which will strictly generate thego_repository
repos definition by parsing thego.mod
andgo.sum
file.There are a few benefits to this approach:
Unlike the previous approach which relies on
go list
andgo download
, this approach requires no network call and thus, fastIf the
go.mod
file is generated correctly by Go tooling, the content of it should be subjected to Module graph pruning and does not include the transitive dependencies that are not needed for the build to work. As a result, we will generate fewergo_repository
repos, sometimes by 6-10x compared to the previous approach. For this to work correctly, we heavily rely on user callinggo mod tidy
to do all the necessary network calls and editgo.mod
andgo.sum
files.This brings the current
update-repos
WORKSPACE setup to be on par with the BzlMod setup, which also parsego.mod
andgo.sum
to generatego_repository
repos. Instead of implementing rules_go own parsers, like the Starlark implementation, this PR tries to reuse code fromx/mod
andx/tools
as much as possible. This madex/tools
a direct dependency of Gazelle.Similar to #1838, this PR is divided into smaller commits for easier reviewing.