go_test: better diagnostic needed for test dependency cycle

[rickystewart]

What version of rules_go are you using?

0.24.9, but I've also checked 0.25.1 and determined that this doesn't fix the bug.

What version of gazelle are you using?

493b9adf67665beede36502c2094496af9f245a3, but as above I've checked with 0.22.3 and that didn't fix it.

What version of Bazel are you using?

bazel 3.7.1-homebrew

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

Yes for the first two, but I didn't check with the latest Bazel.

What operating system and processor architecture are you using?

macOS 10.15.7, x86_64

Any other potentially useful information about your toolchain?

Nothing to note.

What did you do?

The error can be reproduced at the latest master of https://github.com/cockroachdb/cockroach (as of this writing, 39893f8b75243932263e55cfce9e056673be245a) with bazel test pkg/util/uuid:uuid_test. (Sorry, I wasn't able to isolate a smaller repro yet. 😬) Running this command generates the error I've pasted below, namely, the pkg/testutils/skip package isn't being linked with the test sources.

I've verified that the importpaths and dependencies all seem correct. Nonetheless, running with --sandbox_debug --verbose_failures demonstrates that the skip package isn't even being passed to the compiler:

  (cd /private/var/tmp/_bazel_ricky/be70b24e7357091e16c49d70921b7985/sandbox/darwin-sandbox/10379/execroot/cockroach && \
  exec env - \
    APPLE_SDK_PLATFORM=MacOSX \
    APPLE_SDK_VERSION_OVERRIDE=11.1 \
    CGO_ENABLED=1 \
    DEVELOPER_DIR=/Applications/Xcode.app/Contents/Developer \
    GOARCH=amd64 \
    GOOS=darwin \
    GOPATH='' \
    GOROOT=external/go_sdk \
    GOROOT_FINAL=GOROOT \
    PATH=external/local_config_cc:/bin:/usr/bin \
    SDKROOT=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.1.sdk \
    TMPDIR=/var/folders/tk/7wfswg457lqbj05wtbm4ldpm0000gp/T/ \
    XCODE_VERSION_OVERRIDE=12.4.0.12D4e \
  /usr/bin/sandbox-exec -f /private/var/tmp/_bazel_ricky/be70b24e7357091e16c49d70921b7985/sandbox/darwin-sandbox/10379/sandbox.sb /var/tmp/_bazel_ricky/install/87029e13053f87e11688f60f380161fb/process-wrapper '--timeout=0' '--kill_delay=15' bazel-out/host/bin/external/go_sdk/builder compilepkg -sdk external/go_sdk -installsuffix darwin_amd64 -src pkg/util/uuid/codec.go -src pkg/util/uuid/generator.go -src pkg/util/uuid/sql.go -src pkg/util/uuid/uuid.go -src pkg/util/uuid/uuid_wrapper.go -src pkg/util/uuid/benchmark_fast_test.go -src pkg/util/uuid/codec_test.go -src pkg/util/uuid/generator_test.go -src pkg/util/uuid/sql_test.go -src pkg/util/uuid/uuid_test.go -arc 'github.com/cockroachdb/cockroach/pkg/util/timeutil=github.com/cockroachdb/cockroach/pkg/util/timeutil=bazel-out/darwin-fastbuild/bin/pkg/util/timeutil/timeutil.x' -arc 'github.com/cockroachdb/errors=github.com/cockroachdb/errors=bazel-out/darwin-fastbuild/bin/external/com_github_cockroachdb_errors/errors.x' -arc 'github.com/cockroachdb/cockroach/pkg/util/syncutil=github.com/cockroachdb/cockroach/pkg/util/syncutil=bazel-out/darwin-fastbuild/bin/pkg/util/syncutil/syncutil.x' -arc 'github.com/cockroachdb/cockroach/pkg/util/uint128=github.com/cockroachdb/cockroach/pkg/util/uint128=bazel-out/darwin-fastbuild/bin/pkg/util/uint128/uint128.x' -importpath github.com/cockroachdb/cockroach/pkg/util/uuid -p github.com/cockroachdb/cockroach/pkg/util/uuid -package_list bazel-out/host/bin/external/go_sdk/packages.txt -o bazel-out/darwin-fastbuild/bin/pkg/util/uuid/uuid_test.internal.recompileinternal.a -x bazel-out/darwin-fastbuild/bin/pkg/util/uuid/uuid_test.internal.recompileinternal.x -testfilter exclude -gcflags '' -asmflags '') sandbox-exec failed: error executing command 
  (cd /private/var/tmp/_bazel_ricky/be70b24e7357091e16c49d70921b7985/sandbox/darwin-sandbox/10379/execroot/cockroach && \
  exec env - \
    APPLE_SDK_PLATFORM=MacOSX \
    APPLE_SDK_VERSION_OVERRIDE=11.1 \
    CGO_ENABLED=1 \
    DEVELOPER_DIR=/Applications/Xcode.app/Contents/Developer \
    GOARCH=amd64 \
    GOOS=darwin \
    GOPATH='' \
    GOROOT=external/go_sdk \
    GOROOT_FINAL=GOROOT \
    PATH=external/local_config_cc:/bin:/usr/bin \
    SDKROOT=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.1.sdk \
    TMPDIR=/var/folders/tk/7wfswg457lqbj05wtbm4ldpm0000gp/T/ \
    XCODE_VERSION_OVERRIDE=12.4.0.12D4e \
  /usr/bin/sandbox-exec -f /private/var/tmp/_bazel_ricky/be70b24e7357091e16c49d70921b7985/sandbox/darwin-sandbox/10379/sandbox.sb /var/tmp/_bazel_ricky/install/87029e13053f87e11688f60f380161fb/process-wrapper '--timeout=0' '--kill_delay=15' bazel-out/host/bin/external/go_sdk/builder compilepkg -sdk external/go_sdk -installsuffix darwin_amd64 -src pkg/util/uuid/codec.go -src pkg/util/uuid/generator.go -src pkg/util/uuid/sql.go -src pkg/util/uuid/uuid.go -src pkg/util/uuid/uuid_wrapper.go -src pkg/util/uuid/benchmark_fast_test.go -src pkg/util/uuid/codec_test.go -src pkg/util/uuid/generator_test.go -src pkg/util/uuid/sql_test.go -src pkg/util/uuid/uuid_test.go -arc 'github.com/cockroachdb/cockroach/pkg/util/timeutil=github.com/cockroachdb/cockroach/pkg/util/timeutil=bazel-out/darwin-fastbuild/bin/pkg/util/timeutil/timeutil.x' -arc 'github.com/cockroachdb/errors=github.com/cockroachdb/errors=bazel-out/darwin-fastbuild/bin/external/com_github_cockroachdb_errors/errors.x' -arc 'github.com/cockroachdb/cockroach/pkg/util/syncutil=github.com/cockroachdb/cockroach/pkg/util/syncutil=bazel-out/darwin-fastbuild/bin/pkg/util/syncutil/syncutil.x' -arc 'github.com/cockroachdb/cockroach/pkg/util/uint128=github.com/cockroachdb/cockroach/pkg/util/uint128=bazel-out/darwin-fastbuild/bin/pkg/util/uint128/uint128.x' -importpath github.com/cockroachdb/cockroach/pkg/util/uuid -p github.com/cockroachdb/cockroach/pkg/util/uuid -package_list bazel-out/host/bin/external/go_sdk/packages.txt -o bazel-out/darwin-fastbuild/bin/pkg/util/uuid/uuid_test.internal.recompileinternal.a -x bazel-out/darwin-fastbuild/bin/pkg/util/uuid/uuid_test.internal.recompileinternal.x -testfilter exclude -gcflags '' -asmflags '')

I haven't identified the exact source of the failure yet, but this does seem to be related in some way to the indirect dependency that the test has on pkg/util:util_go_proto. To demonstrate this, you can see this commit, which strips out a bunch of unnecessary stuff, and still reproduces the failure. However, commenting out the embed = [":util_go_proto"], line in pkg/util/BUILD.bazel suddenly causes the test to pass.

What did you expect to see?

No build failure.

What did you see instead?

The error looks like this:

cockroach$ bazel test pkg/util/uuid:all
INFO: Analyzed 2 targets (1 packages loaded, 58 targets configured).
INFO: Found 1 target and 1 test target...
ERROR: /Users/ricky/go/src/github.com/cockroachdb/cockroach/pkg/util/uuid/BUILD.bazel:22:8: GoCompilePkg pkg/util/uuid/uuid_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 pkg/util/uuid/codec.go -src pkg/util/uuid/generator.go -src pkg/util/uuid/sql.go -src ... (remaining 37 argument(s) skipped)
Use --sandbox_debug to see verbose messages from the sandbox builder failed: error executing command bazel-out/host/bin/external/go_sdk/builder compilepkg -sdk external/go_sdk -installsuffix darwin_amd64 -src pkg/util/uuid/codec.go -src pkg/util/uuid/generator.go -src pkg/util/uuid/sql.go -src ... (remaining 37 argument(s) skipped)
Use --sandbox_debug to see verbose messages from the sandbox
compilepkg: missing strict dependencies:
	/private/var/tmp/_bazel_ricky/be70b24e7357091e16c49d70921b7985/sandbox/darwin-sandbox/10223/execroot/cockroach/pkg/util/uuid/codec_test.go: import of "github.com/cockroachdb/cockroach/pkg/testutils/skip"
No dependencies were provided.

[rickystewart]

I have a smaller repro of the same issue here. As above, commenting out the embed = [":util_go_proto"] line in pkg/util/BUILD.bazel unexpectedly causes pkg/util/uuid:uuid_test to be able to build again.

[jayconrod]

My guess is that this is caused by a a circular import. If you change the package name in pkg/util/uuid/codec_test.go from uuid to uuid_test, then the build gets a bit further (fails with a different message).

An internal test (files without a _test suffix in the package name) may not import the library being tested, directly or indirectly. However, an external test (with a _test suffix) is allowed to do that. Packages imported by the external test that transitively import the library under test will be recompiled against the internal test package (which includes the library under test's source files together with the internal test source files). The recompiled packages are not passed as dependencies when the internal test package is compiled, since it can't use those packages.

Unfortunately it's hard to provide good diagnostics here. The recompilation logic has to be done during the analysis stage, since we need to create an action to recompile each package. But we can't do any I/O, so we don't actually know which source files are internal or external or whether there's an import cycle.

[rickystewart]

So, assuming that there is a circular import, how do I track it down, and how do I repair it?

I assume that we'll have to rewrite some of our BUILD files to distinguish more clearly between internal and external tests, which we're currently not doing at all. But even after we've done that, from what you're saying it seems there's another issue with a circular import that will cause build breakages? I'm wondering if there is a strategy for identifying those problematic cycles myself, given that Gazelle/rules_go aren't capable of doing so.

[jayconrod]

Normally, I'd recommend bazel query or bazel cquery using the somepath or allpaths operator. When I tried it on Friday, it was finding a path through a proto compiler tool which isn't really what you want. That might still be useful, but it may require more finesse than I had time to apply.

For internal and external tests, there's usually no need to distinguish the two in build files since the files are filtered and split up at execution time. You can split them into separate go_test targets manually, but Gazelle will only help manage one go_test target per directory.

Avoiding import cycles (in tests or otherwise) is the main issue. I don't really have a general strategy to recommend there. The go command detects them and reports them in detail. Bazel reports dependency cycles in targets. But the full view of the test dependency graph is lost in translation, so it's difficult for rules_go to provide better diagnostics.

[rickystewart]

I think my confusion comes from the fact that this project already builds fine with the go command (i.e., that's how it currently builds, but we're migrating to Bazel using Gazelle and rules_go). I assume that if there were an import cycle, then go test of the package in question would fail with an informative error message, which it doesn't. Maybe my understanding of how these tools work is incorrect, but your comment doesn't really seem to contradict that, either.

cockroach$ go test ./pkg/util/uuid
ok  	github.com/cockroachdb/cockroach/pkg/util/uuid	0.585s

[bduffany]

I ran into this issue and came up with a minimal repro, in case it's helpful for anyone trying to address this issue: https://github.com/bduffany/rules_go_bug_repro

Here's the structure:

.
├── BUILD
├── go.mod
├── lib
│   ├── BUILD
│   ├── lib.go         # go_library(name = "lib", srcs = ["lib.go"])
│   └── lib_test.go    # go_test(name="lib_test", embed = [":lib"], deps = ["//testutil"])
├── testutil
│   ├── BUILD
│   └── testutil.go    # go_library(name = "testutil", srcs = ["testutil.go"], deps = ["//lib"])
└── WORKSPACE

The error message is:

compilepkg: missing strict dependencies:
        /home/b/.cache/bazel/_bazel_b/85d775283e81fce9dc063ae8708d0f83/sandbox/linux-sandbox/7/execroot/rules_go_repro/lib/lib_test.go: import of "example.com/owner/repo/testutil"
No dependencies were provided.
Check that imports in Go sources match importpath attributes in deps.

But go test succeeds:

go test example.com/owner/repo/lib
ok      example.com/owner/repo/lib      0.001s

[jayconrod]

@bduffany go test only succeeds because testutil doesn't import lib. If you add an import matching the dependency in go_library, it fails with this message:

# example.com/owner/repo/lib
package example.com/owner/repo/lib (test)
	imports example.com/owner/repo/testutil
	imports example.com/owner/repo/lib: import cycle not allowed in test
FAIL	example.com/owner/repo/lib [setup failed]

[jayconrod]

I think what's needed here is a better error message. When go_test builds the internal test package, it doesn't pass in dependencies declared in go_test that can't be imported by the test due to dependency cycles. So we get this message like the dependency wasn't specified at all.

It would be better to pass in those dependencies using a different flag, so that the builder can print a helpful error if they're imported.

[rickystewart]

Filed cockroachdb/cockroach#60441 to fix the issue on our end. I was having trouble finding the cyclic dependency because it was only in the deps attribute of our custom go_proto_compiler.

[Malinskiy]

I've faced this issue twice in the last couple of weeks, would be nice to provide better error messaging even if it's just a hint along the lines of

compilepkg: missing strict dependencies:
        /home/b/.cache/bazel/_bazel_b/85d775283e81fce9dc063ae8708d0f83/sandbox/linux-sandbox/7/execroot/rules_go_repro/lib/lib_test.go: import of "example.com/owner/repo/testutil"
...
...
It's possible this is caused by a circular dependency

[fmeum]

If anyone wants to get started on a PR based on #2808 (comment), I can provide support.