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]: aspect configure behaves inconsistently #774

Open
walkerburgin opened this issue Nov 10, 2024 · 4 comments
Open

[Bug]: aspect configure behaves inconsistently #774

walkerburgin opened this issue Nov 10, 2024 · 4 comments
Labels
bug Something isn't working untriaged Requires traige

Comments

@walkerburgin
Copy link

What happened?

Bumping aspect-cli to 2024.45.21 and running bazel configure locally resulted in diffs that looked like this:

go_library(
     # ...
     deps = [
-        "@com_github_fatih_color//:go_default_library",
-        "@com_github_spf13_cobra//:go_default_library",
+        "@com_github_fatih_color//:color",
+        "@com_github_spf13_cobra//:cobra",
     ],
     # ...
)

Running bazel configure on that commit on GitHub actions (ubuntu-latest) fails and produces the inverse diff, and I haven't been able to figure out why.

I bisected the source of the original diff to ad74a54.

Version

Development (host) and target OS/architectures:

Output of bazel --version:

Bazelisk version: 1.23.0
Aspect CLI OSS version: 2024.45.21-2252fc6 (with local changes)
Build label: 7.3.1
Build target: @@//src/main/java/com/google/devtools/build/lib/bazel:BazelServer
Build time: Mon Aug 19 16:41:27 2024 (1724085687)
Build timestamp: 1724085687
Build timestamp as int: 1724085687

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

bazel_dep(name = "rules_go", version = "0.50.1")
bazel_dep(name = "gazelle", version = "0.39.1")

Language(s) and/or frameworks involved: go

How to reproduce

- Clone https://github.com/walkerburgin/aspect-configure-repro
- Run `bazel configure`. Observe (hopefully?) that the BUILD files are up to date
- Look at that repository's GitHub actions (e.g. [here](https://github.com/walkerburgin/aspect-configure-repro/actions/runs/11761213195/job/32762696326)) and observe that running `bazel configure` fails.

Any other information?

No response

@walkerburgin walkerburgin added the bug Something isn't working label Nov 10, 2024
@github-actions github-actions bot added the untriaged Requires traige label Nov 10, 2024
@jbedard
Copy link
Member

jbedard commented Nov 10, 2024

Thanks for such a detailed bug report, and for bisecting!

I'm not sure why local vs CI would be different. All I can think of is one (local vs CI) has a) the go_deps stuff cached and it is not being invalided properly, or b) is reading the config for those go_deps differently.

A few questions:

  1. before upgrading to include ad74a54 was bazel configure extremely slow like [Bug]: updating go deps broken with bzlmod #760 (badly) describes?
  2. If you try doing clean --expunge does anything change? Local or CI?

Does adding these directives change anything?

# gazelle:go_naming_convention import
# gazelle:go_naming_convention_external import

It's possible only the _external one is needed. This is mainly just for being explicit to make sure bazel configure and go_deps are using the same config for external deps - so import_alias or go_default_library might also work if we just need to be explicit.

@walkerburgin
Copy link
Author

Yeah, running bazel configure is noticeably faster after the upgrade to 2024.45.21.

Running clean --expunge locally reproduces the behavior I was seeing on CI (back to go_default_library)

ASPECT_CLI_LOG_DEBUG=trace bazel configure
2024/11/09 18:59:17 Found bazel_gazelle_go_repository_config(s): /private/var/tmp/_bazel_walkerburgin/492fc5f288b88071131e05c5241cd082/external/gazelle~~go_deps~bazel_gazelle_go_repository_config/WORKSPACE
Updating BUILD files for go
37 BUILD files visited
0 BUILD files updatedbazel clean --expunge
INFO: Starting clean (this may take a while). Consider using --async if the clean takes more than several minutes.ASPECT_CLI_LOG_DEBUG=trace bazel configure
2024/11/09 19:02:12 No bazel_gazelle_go_repository_config found in output_base: /private/var/tmp/_bazel_walkerburgin/492fc5f288b88071131e05c5241cd082
Updating BUILD files for go
37 BUILD files visited
9 BUILD files updated

Adding the directives explicitly got the CI build and the local build to agree 👍

@jbedard
Copy link
Member

jbedard commented Nov 10, 2024

Yeah, running bazel configure is noticeably faster after the upgrade to 2024.45.21.

The way the rules_go gazelle plugin works under bzlmod requires essentially a bazel query to find the magic gazelle~~go_deps~bazel_gazelle_go_repository_config repository which is essentially just a cached list of all go modules. Putting it in a repo means bazel will fully cache it, remote-cache (and exec?) work with it etc.

But that is the first time any gazelle logic has had to know the location of a repo like that which is why the fix in the aspect-cli was required. I hope to try to improve that more in the future.

Running clean --expunge locally reproduces the behavior I was seeing on CI (back to go_default_library)

Unfortunately I don't understand why 😢

Adding the directives explicitly got the CI build and the local build to agree 👍

Are you unblocked then? I think this is still an issue but unless people are blocked I assume I won't have time to prioritize it...

@walkerburgin
Copy link
Author

walkerburgin commented Nov 10, 2024

Yep, I'm unblocked! Thanks for the help.

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

No branches or pull requests

2 participants