Rules_go: go_test: linker conflict when external test indirectly imports library under test

[robfig]

EDIT by jayconrod on 2020-07-21: Previous title was "Conflicting package heights due to external test"

The error messages for this issue now look like this:

 package conflict error: k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/api/equality: package imports k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/apis/meta/v1
      was compiled with: //staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library
    but was linked with: //staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_test
This sometimes happens when an external test (package ending with _test)
imports a package that imports the library being tested. This is not supported.

---

Using rules_go 0.16.5, I got the "conflicting package heights" error from a mix of internal and external tests, even though I believe that should be supported (ea0abdd2f467522ff1ae1f72c5a89982eb65d210). I'm not doing anything unusual in my BUILD or Go files, and almost everything was auto-generated by Gazelle. Upon fixing it, Gazelle prints warnings that I shouldn't be separating the targets.

Error log:

INFO: Build options have changed, discarding analysis cache.
INFO: Analysed 85 targets (260 packages loaded, 8534 targets configured).
INFO: Found 85 targets...
ERROR: /Users/robfig/alpha/gocode/src/corp/it/apps/zendesk/BUILD.bazel:17:1: GoCompile gocode/src/corp/it/apps/zendesk/darwin_amd64_stripped/go_default_test%/corp/it/apps/zendesk_test.a failed (Exit 1) compile failed: error executing command
  (cd /private/var/tmp/_bazel_robfig/1db08896ecff7e5e96a745981c3b77e6/execroot/corp && \
  exec env - \
    APPLE_SDK_PLATFORM='' \
    APPLE_SDK_VERSION_OVERRIDE='' \
    CGO_ENABLED=1 \
    GOARCH=amd64 \
    GOOS=darwin \
    GOROOT=external/go_sdk \
    GOROOT_FINAL=GOROOT \
    PATH=external/local_config_cc:/bin:/usr/bin \
    XCODE_VERSION_OVERRIDE=9.4.1 \
  bazel-out/host/bin/external/io_bazel_rules_go/go/tools/builders/darwin_amd64_stripped/compile -sdk external/go_sdk -installsuffix darwin_amd64 -src gocode/src/corp/it/apps/zendesk/client.go -src gocode/src/corp/it/apps/zendesk/models.go -src gocode/src/corp/it/apps/zendesk/blackbox_test.go -src gocode/src/corp/it/apps/zendesk/client_test.go -src gocode/src/corp/it/apps/zendesk/models_test.go -arc 'corp/it/worker/config=corp/it/worker/config=bazel-out/darwin-fastbuild/bin/gocode/src/corp/it/worker/config/darwin_amd64_stripped/go_default_library%/corp/it/worker/config.a' -arc 'github.com/stretchr/testify/assert=github.com/stretchr/testify/assert=bazel-out/darwin-fastbuild/bin/external/com_github_stretchr_testify/assert/darwin_amd64_stripped/go_default_library%/github.com/stretchr/testify/assert.a' -arc 'corp/it/apps=corp/it/apps=bazel-out/darwin-fastbuild/bin/gocode/src/corp/it/apps/darwin_amd64_stripped/go_default_library%/corp/it/apps.a' -arc 'github.com/corp/glog=github.com/corp/glog=bazel-out/darwin-fastbuild/bin/external/com_github_corp_glog/darwin_amd64_stripped/go_default_library%/github.com/corp/glog.a' -arc 'corp/it/apps/zendesk=corp/it/apps/zendesk=bazel-out/darwin-fastbuild/bin/gocode/src/corp/it/apps/zendesk/darwin_amd64_stripped/go_default_test%/corp/it/apps/zendesk.a' -o bazel-out/darwin-fastbuild/bin/gocode/src/corp/it/apps/zendesk/darwin_amd64_stripped/go_default_test%/corp/it/apps/zendesk_test.a -package_list bazel-out/host/bin/external/go_sdk/packages.txt -testfilter only -- -trimpath . -p corp/it/apps/zendesk_test)

Use --sandbox_debug to see verbose messages from the sandbox: compile failed: error executing command
  ... [DUPE ERROR MESSAGE AS ABOVE] ...

Use --sandbox_debug to see verbose messages from the sandbox
GoCompile: error running compiler: exit status 2
/private/var/tmp/_bazel_robfig/1db08896ecff7e5e96a745981c3b77e6/sandbox/darwin-sandbox/399/execroot/corp/gocode/src/corp/it/apps/zendesk/blackbox_test.go:11:2: internal compiler error: conflicting package heights 18 and 16 for path "corp/it/apps/zendesk"

Please file a bug report including a short program that triggers the error.
https://golang.org/issue/new

WORKSPACE

http_archive(
    name = "io_bazel_rules_go",
    url = "https://github.com/bazelbuild/rules_go/releases/download/0.16.5/rules_go-0.16.5.tar.gz",
    sha256 = "7be7dc01f1e0afdba6c8eb2b43d2fa01c743be1b9273ab1eaf6c233df078d705",
)

BUILD file

# //gocode/src/corp/it/apps/okta/BUILD.bazel
load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")

go_library(
    name = "go_default_library",
    srcs = [
        "client.go",
        "common.go",
        "const.go",
        "models.go",
    ],
    importpath = "corp/it/apps/okta",
    visibility = ["//visibility:public"],
    deps = [
        "//gocode/src/corp/it/apps:go_default_library",
        "@com_github_corp_errgo//:go_default_library",
        "@com_github_corp_glog//:go_default_library",
    ],
)

go_test(
    name = "go_default_test",
    srcs = [
        "blackbox_test.go",
        "client_test.go",
        "models_test.go",
    ],
    embed = [":go_default_library"],
    deps = [
        "//gocode/src/corp/it/apps:go_default_library",
        "//gocode/src/corp/it/worker/config:go_default_library",
        "@com_github_stretchr_testify//assert:go_default_library",
        "@com_github_corp_errgo//:go_default_library",
    ],
)

Go file (package + imports)

// blackbox_test.go
package okta_test

import (
    "os"
    "testing"
    "corp/it/apps"
    "corp/it/apps/okta"
    "corp/it/worker/config"
)

// tests

Splitting up the BUILD file in the typical way fixes the problem:

go_test(
    name = "go_default_test",
    srcs = [
        "client_test.go",
        "models_test.go",
    ],
    embed = [":go_default_library"],
    deps = [
        "//gocode/src/corp/it/apps:go_default_library",
        "//gocode/src/corp/it/worker/config:go_default_library",
        "@com_github_stretchr_testify//assert:go_default_library",
        "@com_github_corp_errgo//:go_default_library",
    ],
)

go_test(
    name = "go_default_xtest",
    srcs = ["blackbox_test.go"],
    deps = [
        ":go_default_library",
        "//gocode/src/corp/it/apps:go_default_library",
        "//gocode/src/corp/it/worker/config:go_default_library",
        "@com_github_stretchr_testify//assert:go_default_library",
        "@com_github_corp_errgo//:go_default_library",
    ],
)

but results in this warning on subsequent gazelle runs

gazelle: /Users/robfig/alpha/gocode/src/corp/it/apps/okta/BUILD.bazel: go_default_xtest is no longer necessary. Run 'gazelle fix' to squash with go_default_test.

[jayconrod]

Could you post a minimal test case that reproduces this? The smaller the better.

"Conflicting package height" errors generally mean that different versions of the same package are used in a build. Those errors are difficult to analyze.

[robfig]

Here's a minimal test case:
https://github.com/robfig/packageheights-example

Run this to see the error:

$ bazel test b:go_default_test

The source of the issue appears to be due to these two factors:

1. A combination of "external" and non-external tests in the same target

2. An external test that depends upon a package which would create a cycle if it
   were part of the same package as the code under test.

In this case, the dependencies look like:

a/a.go -> b/b.go
b/b_test.go -> b/b.go
b/b_external_test.go -> a/a.go, b/b.go

package "b" is not allowed to depend upon package "a", but package "b_test" should be allowed to.

[jayconrod]

bazel test @org_golang_x_net//bpf:go_default_test also reproduces this issue.

It seems like the external test package bpf_test imports packages that transitively import the library under test. go build dictates that these libraries should be recompiled to depend on the internal test package instead.

[AustinSchuh]

What about automatically creating 2 go_test rules, one for each package?

[jayconrod]

We used to do that (go_default_xtest). We squashed them because it was a bit messy and it was hard for external tests to depend on definitions in internal tests.

It doesn't really solve this problem though. Try running go list -test -json golang.org/x/net/bpf. golang.org/x/net/bpf_test (external test package) imports golang.org/x/net/bpf [golang.org/x/net/bpf.test] (internal test package). It also imports golang.org/x/net/ipv4 [golang.org/x/net/bpf.test]. That's where the problem is. We get the error because @org_golang_x_net//ipv4:go_default_library is compiled with @org_golang_x_net//bpf:go_default_library, not the internal test package produced by @org_golang_x_net//bpf:go_default_test.

In order to fix this, we'd need to recompile every transitive dependency of @org_golang_x_net//bpf:go_default_test that transitively depends on @org_golang_x_net//bpf:go_default_library. I think we could do that with an aspect, but I'd like to get rid of our current cross-compiling aspect before we think about introducing another one, even in narrow cases like this.

[asf-stripe]

Chiming in that we're getting bitten by this too, using rules_go 0.18.2, and gazelle 0.17.0.

The repository I'm trying to test under bazel is not going to get BUILD files checked in anytime soon (so we have to rely on gazelle-generated build files), and this bug is preventing us from making progress pulling its tests into bazel. Is there a workaround we could use in the meantime?

The best I can come up with is by manually excluding the external test files. /:

[jayconrod]

@asf-stripe The build files will require some adjustment to work around this. If you're okay with excluding some test sources, that may just be a matter of deleting something from srcs after Gazelle runs or adding # gazelle:exclude some_test.go before Gazelle runs. If you're using go_repository to pull in this repo, it lets you apply patches after Gazelle runs.

[asf-stripe]

Right, I use

    patch_cmds = [
      # Exclude package-external tests from the test run, due to the
      # "conflicting package heights" bug in rules_go at
      # https://github.com/bazelbuild/rules_go/issues/1877:
      """for external_test_file in $(grep -l '^package.*_test$' $(find . -name '*_test.go')) ; do
            bn=$(basename $external_test_file)
            dir=$(dirname $external_test_file)
            sed -i.bak "s#\\"$bn\\",#\\# removed $bn due to https://github.com/bazelbuild/rules_go/issues/1877 #" $dir/BUILD.bazel
         done
      """
    ],

at the moment, and I feel very very disgusted; but it does work... I guess I'll ask around if we're comfortable running like this.

[jayconrod]

This should be addressed before Go 1.15 ships. 1.15 will trigger link errors in additional tests, since the new linker verifies that hashes of export data match hashes that were seen at compile time (#2535).

I've been thinking about how to implement this. It's at the edge of not being feasible, so I'm leaning toward explicitly not supporting it and providing a clear error message.

To recap, suppose we have a package A with internal and external tests. The external test imports package B, which imports A. Normally, when B is built, it would be built with the library A. However, when building A's tests, we don't pass the library A to the linker; we pass A's internal test package (which embeds the library's sources). So B needs to be recompiled against A's internal test package. We also need to recompile anything that transitively imports B. And we need to do this for every test with this pattern. This scales very poorly: O(n*t) compilations for n packages, with t tests in the worst case.

I don't think there's a practical way to recompile a graph of packages in go_test itself. GoArchive doesn't carry enough information. We could recompile direct dependencies but not transitive dependencies, since we only have a depset of GoArchiveData to pass to the linker.

So we'd be stuck with an aspect. That would need to be gated by a flag on a go_test wrapper to avoid the expense of triggering it for the majority of tests, which don't need it. The aspect would carry the list of embedded targets from the root go_test, and it would replace dependencies on those targets with dependencies on the go_test internal test.

I think this would work, assuming it wouldn't introduce circular dependencies I haven't thought about. Again though, it's complicated, and it enables a pattern which is very costly. It's probably better to just provide a clear error message and encourage developers to avoid this pattern.

[robfig]

This is probably a dumb question: what does the Go CLI do, and what is the barrier from Bazel doing the same?

[jayconrod]

The go command recompiles packages that import the library under test to import the internal test package instead. It also recompiles everything that imports recompiled packages, and so on. That's the O(n*t) behavior.

It's harder to do this in rules_go, since individual rules don't have access to the full configured target graph. We'd have to either carry additional information in providers (whether we use it or not), or we'd have to use an aspect (same, but more automatic). Either way, it's pretty expensive.

[robfig]

Ah, I assumed that the go command managed to avoid the O(n*t) behavior somehow. Makes sense, thanks for the explanation.

[robfig]

To clarify - without support, a package will be able to have internal tests or external tests, but not both?

That limitation seems ok, with a clear message. We don't have many packages with external tests to start with, so the number of affected packages would certainly be small, and there's always a workaround. Go compiles so quickly that the processing time is not a significant factor in my mind, but keeping the rules simpler presumably has maintenance benefits and avoids the opportunity cost.

[jayconrod]

You can have both internal tests and external tests, and when the external test imports the library under test, it will get the internal test package (which embeds the library under test and can inject test doubles)... but you can't have an external test that imports another package that imports the library under test.

When you import another go_library, it's just built as a regular go_library instead of being built in an alternative configuration for the test that needs it.

[fishy]

What's the longer term plan of this issue? Currently the warning message says "This is not supported" and "This will be an error in the future". If this really becomes an error in the future, that means a perfect fine situation to Go language itself, and supported by go toolchains, will be broken in Bazel.

If rules_go itself doesn't fix this issue, and we solely rely on the code to walkaround this issue, then the walkarounds I can think of are:

1. Don't have a_test -> b -> a dependency chain. This isn't always possible, for example we could have a similar situation that a is net/http and b is net/http/httptest, so that of course b has to depend on a and a's external tests are likely to depend on b for the mocks.
2. Don't use external tests. This is doable, but external tests are really valuable to help test API boundaries.

[jayconrod]

For now, the plan is to make this a prominent warning in v0.24.0 (same as what went into v0.23.5), then a hard error in v0.25.0.

My assumption here is that this case is relatively rare. If you find this pattern is common in your code base, please comment. If this turns out to be common for a lot of people, we'll need a better solution. However, my preference is not to support this for two reasons:

1. Any proper solution for this requires recompiling a potentially large number of packages for each test. This is true for go build as well. This scales poorly, and this pattern should be avoided.
2. The engineering cost to fix this is substantial. The problem can be fixed with an aspect (inefficient; adds technical debt), or adding recompilation logic to the rules (efficient, but may require a redesign of providers and toolchain API).

In terms for workarounds, it's not quite so restrictive. An a_test -> b -> a dependency chain is fine if external and internal tests aren't mixed together. If they are, and the external test doesn't actually depend on definitions from the internal tests, then the easiest workaround is to split the external tests into another package so they can safely import the library being tested instead of the internal tests.

[croachrose]

This is very common in our codebase. I think the big issue we face is that even with _purely_ external tests, gazelle produces a go_test rule that _embeds_ the package and triggers this warning. Perhaps gazelle can be updated to separate the internal and external tests into different targets?

[asf-stripe]

(Also chiming in as a person who has foo_test packages living in a directory with foo-packaged files)

Perhaps gazelle can be updated to separate the internal and external tests into different targets?

I think this is the real solution here: foo_test-packaged files should not be included in the test target for the foo package, and instead go into a separate test target.

[jayconrod]

This is very common in our codebase. I think the big issue we face is that even with purely external tests, gazelle produces a go_test rule that embeds the package and triggers this warning.

Sounds like a bug with the warning then. If there aren't any internal tests, then there ought to be no problem. I'll reopen this issue and post a fix soon.

Perhaps gazelle can be updated to separate the internal and external tests into different targets?

Sorry, no. We did that for a long time and people hated it. Not going to reverse that migration.

[croachrose]

Sorry, no. We did that for a long time and people hated it. Not going to reverse that migration.

True, but that was before this issue. Having separate targets when a single target will link correctly _is_ annoying. Having a single target that eventually won't compile...is worse?

[fishy]

We have 3 out of 23 tests having this issue currently in https://github.com/reddit/baseplate.go codebase, so I'd say this is not rare.

the easiest workaround is to split the external tests into another package so they can safely import the library being tested instead of the internal tests.

This is basically what Gazelle did before by auto separate internal and external tests, and you said we are not going to reverse that? No?

I do think separating internal and external tests are the best mid-term solution, but it would be best done via gazelle (e.g. reverse that migration). Maybe make it a flag to gazelle so that for people know that if they don't do it they will get this issue, they can turn it on?

[jayconrod]

We have 3 out of 23 tests having this issue currently in https://github.com/reddit/baseplate.go codebase, so I'd say this is not rare.

Thanks for adding this. Just to confirm, these are packages with both internal and external tests, right? Fixing the buggy warning won't help these?

This is basically what Gazelle did before by auto separate internal and external tests, and you said we are not going to reverse that? No?

I do think separating internal and external tests are the best mid-term solution, but it would be best done via gazelle (e.g. reverse that migration). Maybe make it a flag to gazelle so that for people know that if they don't do it they will get this issue, they can turn it on?

Previously, Gazelle generated go_default_test and go_default_xtest targets. It was annoying for folks since Bazel treats them as separate targets: it links multiple binaries and has separate caching, coverage, and filtering. I'm not going to support that again. This would also break support for external tests that depend on definitions in internal tests (which works as long as it's a direct import).

If there's a solution for this, it should be in go_test. As I've said, it could be done with either an aspect or with significant new analysis logic.

[fishy]

Thanks for adding this. Just to confirm, these are packages with both internal and external tests, right? Fixing the buggy warning won't help these?

Yes, with mixed internal and external tests.

This would also break support for external tests that depend on definitions in internal tests

Hmm I didn't know you could do that. TIL. I would feel that is rarer than this case but that's likely just because I'm not aware of it :)

[fishy]

Just to clarify, my suggestion is:

1. Gazelle still default to combine internal and external tests together (the same as what it does today)
2. Add a flag to gazelle to separate internal and external tests (what it did before)

This way, for people not having this problem they don't need to do anything. For people have this problem, and they don't have external tests that depend on definitions in internal tests, they could turn on that flag. It's only for people that have both they have to write their build files manually to walk around (or use other walk arounds).

Also this is just a short term stop gap before the proper fix in go_test is landed. Does that make sense to you? But maybe just ignore the warning would be good enough if we can land the proper fix in go_test before v0.25.0 :)

[jayconrod]

Sorry, no. Gazelle has a huge number of flags and settings, and it's difficult to make changes without breaking (seemingly unrelated tests). It's also confusing for users to get just the right set of flags and directives when something goes wrong.

[jayconrod]

This is very common in our codebase. I think the big issue we face is that even with purely external tests, gazelle produces a go_test rule that embeds the package and triggers this warning. Perhaps gazelle can be updated to separate the internal and external tests into different targets?

@asf-stripe Could you confirm this? I couldn't reproduce this with a small example. Here's what I tried:

-- BUILD.bazel --

load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")


go_library(

    name = "go_default_library",

    srcs = ["a.go"],

    importpath = "example.com/m/a",

    visibility = ["//visibility:public"],

)

go_test(

    name = "go_default_test",

    srcs = [

        "a_i_test.go",

        "a_x_test.go",

    ],

    embed = [":go_default_library"],

)

-- a.go --

package a

func A() {}

-- a_i_test.go --

package a

import "testing"

func TestAInternal(t *testing.T) {

    A()

}

-- a_x_test.go --

package a_test

import (

    "testing"

"example.com/m/a"


)

func TestAExternal(t *testing.T) {

    a.A()

}

This should work without an error message. You'd get an error if you wrote deps = [":go_default_library], in addition to the embed, but Gazelle won't generate that.

To clarify what's happening, go_test generates three archives: an internal test archive that includes sources and dependencies from embedded packages, an external test archive that implicitly depends on the internal test archive, and a main package that imports both and drives the test. Neither archive may directly depend on the library under test. Both archives are generated unconditionally, since go_test doesn't know at analysis time whether source files belong to the internal or external test archive. The files are filtered during execution, and the packages may end up empty.

[AustinSchuh]

I've switched jobs since I last looked at this so I can't reference it quickly, but I remember running into this a bunch when trying to get the golang standard library tests running. Things like x/net/html have tests which triggered this warning. I ended up giving up and saying that we weren't going to run tests for any external libraries, and preventing Gazelle from generating the targets.

[jayconrod]

I spent most of today working on a permanent fix for this. I think I have something promising; will polish it and submit it tomorrow if it passes all the tests. It doesn't require an aspect, but it adds a lot of complicated analysis code to go_test. It requires storing a lot more information in GoArchiveData, but I'm going to keep those fields private so this change can be backported.

[jayconrod]

Hopefully #2579 should fix this issue. Please try it out and comment if you find that anything is still broken.

If it works, I'll backport it and tag a point release this week.

[fishy]

@jayconrod : I tried 464eb9c71c682d1a8367944ec768f25f989a9916 on the current master version of Baseplate.go and it no longer show the warnings when we run bazel test //...:all. When on v0.23.5 it shows 3 warnings.

[jayconrod]

Glad to hear that! I'm also no longer seeing the warning on @bazel_gazelle//repo:go_default_test. I added some golang.org/x/... tests to rules_go CI that had been broken by this, and they seem to work now.

[jayconrod]

Point releases with this fix are out now. Please give them a try: v0.22.9, v0.23.6.