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

update-repos: add -parse_only flag #1837

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

sluongng
Copy link
Contributor

@sluongng sluongng commented Jul 3, 2024

Based on #1838 so please review that first.

This PR adds a -parse_only flag to gazelle update-repos which will strictly generate the go_repository repos definition by parsing the go.mod and go.sum file.

There are a few benefits to this approach:

  1. Unlike the previous approach which relies on go list and go download, this approach requires no network call and thus, fast

  2. If 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 fewer go_repository repos, sometimes by 6-10x compared to the previous approach. For this to work correctly, we heavily rely on user calling go mod tidy to do all the necessary network calls and edit go.mod and go.sum files.

  3. This brings the current update-repos WORKSPACE setup to be on par with the BzlMod setup, which also parse go.mod and go.sum to generate go_repository repos. Instead of implementing rules_go own parsers, like the Starlark implementation, this PR tries to reuse code from x/mod and x/tools as much as possible. This made x/tools a direct dependency of Gazelle.

Similar to #1838, this PR is divided into smaller commits for easier reviewing.

@sluongng sluongng force-pushed the sluongng/pruned-graph-update-repos branch 2 times, most recently from a5ca20f to bb80a30 Compare July 5, 2024 10:28
@sluongng sluongng force-pushed the sluongng/pruned-graph-update-repos branch 3 times, most recently from 63f2e8c to 7421582 Compare July 5, 2024 19:29
@sluongng
Copy link
Contributor Author

sluongng commented Jul 5, 2024

@fmeum this is failing with

(19:31:14) ERROR: error loading package '@@io_bazel_rules_go//proto': at /var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/ec321eb2cc2d0f8f91b676b6d4c66c29/external/io_bazel_rules_go/proto/compiler.bzl:20:5: cannot load '@@rules_proto//proto:proto_common.bzl': no such file

which seems to be a rules_go issue? rules_proto needs to be part of go_rules_dependencies? 🤔

@sluongng sluongng force-pushed the sluongng/pruned-graph-update-repos branch from 7421582 to a718352 Compare July 5, 2024 20:45
@fmeum
Copy link
Member

fmeum commented Jul 6, 2024

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.

@sluongng
Copy link
Contributor Author

sluongng commented Jul 8, 2024

I think that's a bit strange to make a non-dev dep "optional".
The deps are usually declared with maybe and will only be lazily downloaded when used.

So I don't see a reason why rules_proto needed to be excluded here.

@sluongng sluongng force-pushed the sluongng/pruned-graph-update-repos branch 3 times, most recently from 6eaf515 to a5bb8f6 Compare July 8, 2024 19:29
@sluongng sluongng force-pushed the sluongng/pruned-graph-update-repos branch from a5bb8f6 to 9eb54f4 Compare July 8, 2024 19:31
@sluongng sluongng force-pushed the sluongng/pruned-graph-update-repos branch from 9eb54f4 to 52147da Compare July 8, 2024 19:50
@sluongng sluongng changed the title update-repos use pruned graph update-repos: add -parse_only flag Jul 8, 2024
@sluongng sluongng marked this pull request as ready for review July 8, 2024 20:03
@fmeum
Copy link
Member

fmeum commented Jul 9, 2024

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.

Copy link
Contributor

@tyler-french tyler-french left a 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.

@tyler-french
Copy link
Contributor

@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

@sluongng
Copy link
Contributor Author

sluongng commented Jul 9, 2024

Yeah I have separated it to #1838. Let's discuss it there.

@sluongng sluongng force-pushed the sluongng/pruned-graph-update-repos branch from 52147da to f4130c6 Compare July 15, 2024 12:10
@sluongng sluongng force-pushed the sluongng/pruned-graph-update-repos branch from f4130c6 to 567d0bc Compare July 15, 2024 12:25
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