search

aspect-build / aspect-cli

correct, fast, usable: choose three
https://aspect.build/cli
Apache License 2.0
93 stars 21 forks source link

[Bug]: `aspect configure` behaves inconsistently #774

Open walkerburgin opened 1 week ago

walkerburgin commented 1 week ago

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 ad74a54410d2ece088712e9bb47f9a871b939c99.

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

jbedard commented 1 week ago

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 https://github.com/aspect-build/aspect-cli/commit/ad74a54410d2ece088712e9bb47f9a871b939c99 was bazel configure extremely slow like https://github.com/aspect-build/aspect-cli/issues/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 commented 1 week ago

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 updated
➜  bazel 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 commented 1 week ago

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 commented 1 week ago

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