Circular dependency in Bazel deps fails with cryptic error

[mh-park]

This is a followup to #2583; in a situation where there is no cyclic dependency in Go, but a cyclic dependency in Bazel, we see errors that look irrelevant to the problem. Essentially, if some transitive dependency "over-provides" dependencies and one of those dependencies trigger a cycle, this returns an error that says an import is missing.

Previously, in prior versions of rules_go, the "over-provision" was okay; Bazel would handle these cases safely and build without triggering a cycle.

For the sake of creating a simple case, the example uses a go_default_library rule as the example transitive dependency, but in the case that this is a rule that generates code based on Go source code (e.g. a GoMock rule, go_proto_library rule w/ custom compiler plugins), the exact depset is sometimes difficult to compute via Gazelle.

What's the exact expected behavior here, and what is the best course of action to fix a situation like this?

What version of rules_go are you using?

0.24.3

What version of gazelle are you using?

0.22.0

What version of Bazel are you using?

3.3.0

Does this issue reproduce with the latest releases of all the above?

Yes

What operating system and processor architecture are you using?

Darwin amd64

Any other potentially useful information about your toolchain?

What did you do?

$ git clone git@github.com:mh-park/bazel_examples.git
$ cd bazel_examples
$ git checkout e367c2de13fd592738a22d99e2ee553196e91e62
$ bazel build //imports:go_default_test

What did you expect to see?

Build completed successfully

What did you see instead?

ERROR: /Users/minho.park/gocode/src/minho.park/bazel_examples/imports/BUILD.bazel:10:1: GoCompilePkg imports/go_default_test.internal.recompileinternal.a failed (Exit 1) builder failed: error executing command bazel-out/host/bin/external/go_sdk/builder compilepkg -sdk external/go_sdk -installsuffix darwin_amd64 -src imports/client.go -src imports/client_test.go -importpath ... (remaining 17 argument(s) skipped)

Use --sandbox_debug to see verbose messages from the sandbox
compilepkg: missing strict dependencies:
	/private/var/tmp/_bazel_minho.park/d616cad23559aa092c0847892084ee98/sandbox/darwin-sandbox/85/execroot/__main__/imports/client_test.go: import of "github.com/minho.park/bazel_examples/imports/testutils"
No dependencies were provided.
Check that imports in Go sources match importpath attributes in deps.
Target //imports:go_default_test failed to build
Use --verbose_failures to see the command lines of failed build steps.

[jayconrod]

Reproduced with a small modification: in imports/testutils/BUILD.bazel, the importpath attribute should be "github.com/minho.park/bazel_examples/imports/testutils" so it matches the import string.

I guess the resolution here would be, when recompiling the test's internal archive, pass the builder a list of direct dependencies that are being recompiled and must not be imported. The builder could report a specific error if any of those packages are actually imported.

[mh-park]

Sorry for the hiccup and thanks for looking into this!

This would also be an issue for go_library rules as well.

$ git clone git@github.com:mh-park/bazel_examples.git
$ cd bazel_examples
$ git checkout 1c9a1577766a82b1412007aecdae30ef55c87595
$ bazel build //imports:go_default_library
ERROR: /Users/minho.park/gocode/src/minho.park/bazel_examples/imports/BUILD.bazel:3:1: in go_library rule //imports:go_default_library: cycle in dependency graph:
.-> //imports:go_default_library
|   //imports/testutils:go_default_library
`-- //imports:go_default_library
This cycle occurred because of a configuration option
ERROR: Analysis of target '//imports:go_default_library' failed; build aborted
INFO: Elapsed time: 0.128s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded, 0 targets configured)

I don't have full insight to the rules_go repo here; would your proposed change solve this case as well?

[jayconrod]

For go_library, that error is legitimate. Bazel forbids circular dependencies between configured targets. It will reports that error when loading build files, before running rule implementations, so rules_go can't do anything about that.

Incidentally, although that matches Go's import behavior pretty well, it causes problems for other languages like Java that allow circular imports between packages. Java developers need to put multiple packages in a single java_library to deal with that (or fix their import graph).

[mh-park]

There's one thing I'm struggling to understand. Why is go_test able to prune out unused / cyclic dependencies before running rule implementations, while go_library is not? Is the same logic not implementable in go_library? I do understand that it's an expensive operation for go_test; is that why we'd rather not do it in go_library, or is there another reason to why it's not possible?

[jayconrod]

With go_test, there is no cycle in Bazel's configured target graph. The test depends on another library, which depends on the library under test. The library under test doesn't depend on the test, so the rule implementations run. The builder detects the cycle, knowing that the library under test is embedded into the internal test package.

That can't be done with go_library. There's a cycle in the configured target graph, and that has to be fixed.

[mh-park]

I'm finally understanding the full picture here. I'm going to try to describe this as exactly as possible so we're on the same page.

In the example above, let's say //imports/testutil:go_default_library is actually some custom Go rule that we have internally, and it's impossible for us to determine whether //imports/testutil:go_default_library depends on //imports:go_default_library, so we always add it as a dependency. Upon actually building //imports/testutil:go_default_library and inspecting the output, we see that it doesn't actually need it as an import, but it's impossible to determine that until we actually inspect it.

Prior to #2579 and rules_go 0.23.6, //imports/testutil:go_default_test built fine. I think you briefly touch on why in #2583 (comment), and it seems like there's no real solution for us unless we can compute the exact depset for //imports/testutil:go_default_library. My understanding is that we were reliant on a bug, and its fix is causing our rules to break; is that understanding correct?

[linzhp]

Should be fixed by #3422