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

[Bug]: updating go deps broken with bzlmod #760

Closed
jbedard opened this issue Oct 18, 2024 · 8 comments
Closed

[Bug]: updating go deps broken with bzlmod #760

jbedard opened this issue Oct 18, 2024 · 8 comments
Assignees
Labels
bug Something isn't working

Comments

@jbedard
Copy link
Member

jbedard commented Oct 18, 2024

What happened?

Crashes or really slow when updating go deps in bzlmod.

Version

Development (host) and target OS/architectures:

Output of bazel --version:

Version of the Aspect rules, or other relevant rules from your
WORKSPACE or MODULE.bazel file:

Language(s) and/or frameworks involved:

How to reproduce

See https://aspect-build.slack.com/archives/C04281DTLH0/p1729081900853419

Any other information?

See fix in gazelle sh script: https://github.com/bazel-contrib/bazel-gazelle/blob/b160ccd46fb7c19a49530d08906165c73c343dd1/internal/gazelle.bash.in#L101-L106

@jbedard jbedard added the bug Something isn't working label Oct 18, 2024
@github-actions github-actions bot added the untriaged Requires traige label Oct 18, 2024
@r0bobo
Copy link

r0bobo commented Oct 19, 2024

I will try to create a PR next week if I find some time. Do you know how to deduce the -repo_config path of the top of your head?

@r0bobo
Copy link

r0bobo commented Oct 19, 2024

I can also add that this is not a problem with bzlmod and rules_go when you use go mod vendor for all depedencies. It only manifests when we remove vendoring and use:

# MODULE.bazel
go_deps = use_extension("@gazelle//:extensions.bzl", "go_deps")
go_deps.from_file(go_mod = "//:go.mod")

@r0bobo
Copy link

r0bobo commented Oct 21, 2024

I will try to create a PR next week if I find some time. Do you know how to deduce the -repo_config path of the top of your head?

Hmm, this is more complicated than I first thought. Does aspect configure need to run bazel first to generate the gazelle~~go_deps~bazel_gazelle_go_repository_config/WORKSPACE repository that contains the go_repository(...) declarations?
The WORKSPACE files looks something like that:

go_repository(
    name = "@gazelle~",
    importpath = "github.com/bazelbuild/bazel-gazelle",
    module_name = "gazelle",
)
go_repository(
    name = "@rules_go~",
    importpath = "github.com/bazelbuild/rules_go",
    module_name = "rules_go",
)
go_repository(
    name = "co_honnef_go_tools",
    importpath = "honnef.co/go/tools",
)
...

@r0bobo
Copy link

r0bobo commented Oct 29, 2024

Here is a repo to reproduce the slow performance behavior. The results of gazelle vs aspect configure:

time bzl configure
Updating BUILD files for go
2 BUILD files visited
0 BUILD files updated
bazelisk configure  18.23s user 3.18s system 140% cpu 15.257 total

❯ time bzl run gazelle
INFO: Analyzed target //:gazelle (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target //:gazelle up-to-date:
  bazel-bin/gazelle-runner.bash
  bazel-bin/gazelle
INFO: Elapsed time: 0.073s, Critical Path: 0.00s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action
INFO: Running command line: bazel-bin/gazelle
bazelisk run gazelle  0.10s user 0.13s system 88% cpu 0.258 total

@jbedard jbedard removed the untriaged Requires traige label Oct 30, 2024
@jbedard jbedard self-assigned this Oct 30, 2024
@jbedard jbedard closed this as completed Nov 5, 2024
@jbedard jbedard closed this as completed Nov 5, 2024
@r0bobo
Copy link

r0bobo commented Nov 6, 2024

Hmm, is this really fixed? 🤔
I see the same behavior from latest main.

@jbedard
Copy link
Member Author

jbedard commented Nov 6, 2024

It is fixed in the aspect private repo. If you're using the cli from this repo you'll have to wait till the change gets pushed here. Sorry I guess that's confusing when it gets closed from the private repo :/

@r0bobo
Copy link

r0bobo commented Nov 6, 2024 via email

jbedard added a commit that referenced this issue Nov 7, 2024
The rules_go gazelle integration changed in bzlmod such that it now
requires resolving the
[bazel_gazelle_go_repository_config](https://github.com/bazel-contrib/bazel-gazelle/blob/v0.39.1/internal/bzlmod/go_deps.bzl#L653-L672)
repository which contains a cache-able list of the `go_repository`
target data (not the actual targets, just data about them).

With regular gazelle from a `gazelle_binary` target the
`@bazel_gazelle_go_repository_config` repo path is resolved using [a
rule label
attribute](https://github.com/bazel-contrib/bazel-gazelle/blob/v0.39.1/def.bzl#L101-L104)
and [this
hack](https://github.com/bazel-contrib/bazel-gazelle/blob/v0.39.1/internal/go_repository.bzl#L313-L321).
However when we are running gazelle within `aspect configure` we need an
alternate way of resolving this path.

This PR does something similar to the [the go_repository
hack](https://github.com/bazel-contrib/bazel-gazelle/blob/v0.39.1/internal/go_repository.bzl#L313-L321),
except we must launch `bazel info output_base` to get the external
directory.

Fix #760

---

### Changes are visible to end-users: yes

- Searched for relevant documentation and updated as needed: yes
- Breaking change (forces users to change their own code or config): no
- Suggested release notes appear below: yes

The go language in `aspect configure` will now work when rules_go is
under bzlmod.

### Test plan

- Manual testing; please provide instructions so we can reproduce: run
the cli binary on a repo where rules_go is loaded with bzlmod, for
example https://github.com/r0bobo/aspect-cli-issue-760-reproduce

GitOrigin-RevId: aded45cc9e6566447383b193a201255e55e90730
jbedard added a commit that referenced this issue Nov 7, 2024
The rules_go gazelle integration changed in bzlmod such that it now
requires resolving the
[bazel_gazelle_go_repository_config](https://github.com/bazel-contrib/bazel-gazelle/blob/v0.39.1/internal/bzlmod/go_deps.bzl#L653-L672)
repository which contains a cache-able list of the `go_repository`
target data (not the actual targets, just data about them).

With regular gazelle from a `gazelle_binary` target the
`@bazel_gazelle_go_repository_config` repo path is resolved using [a
rule label
attribute](https://github.com/bazel-contrib/bazel-gazelle/blob/v0.39.1/def.bzl#L101-L104)
and [this
hack](https://github.com/bazel-contrib/bazel-gazelle/blob/v0.39.1/internal/go_repository.bzl#L313-L321).
However when we are running gazelle within `aspect configure` we need an
alternate way of resolving this path.

This PR does something similar to the [the go_repository
hack](https://github.com/bazel-contrib/bazel-gazelle/blob/v0.39.1/internal/go_repository.bzl#L313-L321),
except we must launch `bazel info output_base` to get the external
directory.

Fix #760

---

### Changes are visible to end-users: yes

- Searched for relevant documentation and updated as needed: yes
- Breaking change (forces users to change their own code or config): no
- Suggested release notes appear below: yes

The go language in `aspect configure` will now work when rules_go is
under bzlmod.

### Test plan

- Manual testing; please provide instructions so we can reproduce: run
the cli binary on a repo where rules_go is loaded with bzlmod, for
example https://github.com/r0bobo/aspect-cli-issue-760-reproduce

GitOrigin-RevId: aded45cc9e6566447383b193a201255e55e90730
jbedard added a commit that referenced this issue Nov 7, 2024
The rules_go gazelle integration changed in bzlmod such that it now
requires resolving the
[bazel_gazelle_go_repository_config](https://github.com/bazel-contrib/bazel-gazelle/blob/v0.39.1/internal/bzlmod/go_deps.bzl#L653-L672)
repository which contains a cache-able list of the `go_repository`
target data (not the actual targets, just data about them).

With regular gazelle from a `gazelle_binary` target the
`@bazel_gazelle_go_repository_config` repo path is resolved using [a
rule label
attribute](https://github.com/bazel-contrib/bazel-gazelle/blob/v0.39.1/def.bzl#L101-L104)
and [this
hack](https://github.com/bazel-contrib/bazel-gazelle/blob/v0.39.1/internal/go_repository.bzl#L313-L321).
However when we are running gazelle within `aspect configure` we need an
alternate way of resolving this path.

This PR does something similar to the [the go_repository
hack](https://github.com/bazel-contrib/bazel-gazelle/blob/v0.39.1/internal/go_repository.bzl#L313-L321),
except we must launch `bazel info output_base` to get the external
directory.

Fix #760

---

### Changes are visible to end-users: yes

- Searched for relevant documentation and updated as needed: yes
- Breaking change (forces users to change their own code or config): no
- Suggested release notes appear below: yes

The go language in `aspect configure` will now work when rules_go is
under bzlmod.

### Test plan

- Manual testing; please provide instructions so we can reproduce: run
the cli binary on a repo where rules_go is loaded with bzlmod, for
example https://github.com/r0bobo/aspect-cli-issue-760-reproduce

GitOrigin-RevId: aded45cc9e6566447383b193a201255e55e90730
@jbedard
Copy link
Member Author

jbedard commented Nov 8, 2024

This will be in the next release which is currently publishing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants