Closed robfig closed 4 years ago
jayconrod
commented
5 years ago 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
commented
5 years ago Here's a minimal test case: https://github.com/robfig/packageheights-example
Run this to see the error:
$ bazel test b:go_default_testThe source of the issue appears to be due to these two factors:
A combination of "external" and non-external tests in the same target
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
commented
5 years ago 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
commented
5 years ago What about automatically creating 2 go_test rules, one for each package?
jayconrod
commented
5 years ago 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
commented
5 years ago 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
commented
5 years ago @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
commented
5 years ago 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
commented
4 years ago 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
commented
4 years ago This is probably a dumb question: what does the Go CLI do, and what is the barrier from Bazel doing the same?
jayconrod
commented
4 years ago 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
commented
4 years ago Ah, I assumed that the go command managed to avoid the O(n*t) behavior somehow. Makes sense, thanks for the explanation.
robfig
commented
4 years ago 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
commented
4 years ago 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
commented
4 years ago 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:
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.jayconrod
commented
4 years ago 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:
go build as well. This scales poorly, and this pattern should be avoided.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.
patrickmscott
commented
4 years ago 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
commented
4 years ago (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
commented
4 years ago 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.
patrickmscott
commented
4 years ago 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
commented
4 years ago 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
commented
4 years ago 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
commented
4 years ago 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
commented
4 years ago Just to clarify, my suggestion is:
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
commented
4 years ago 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
commented
4 years ago 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
commented
4 years ago 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
commented
4 years ago 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
commented
4 years ago 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
commented
4 years ago @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
commented
4 years ago 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
commented
4 years ago 
ajwerner
commented
2 years ago This sometimes happens when an external test (package ending with _test) imports a package that imports the library being tested. This is not supported.
This is a totally valid thing to do in go. Should we have a separate tracking issue to support it?
linzhp
commented
2 years ago @ajwerner I think this is fixed. Have you tried?
miracvbasaran
commented
11 months ago I am still having an issue with this with go version 1.20, bazel version 6.4.0, io_bazel_rules_go version 0.42.0 and bazel_gazelle version 0.33.0:
link: package conflict error: cloud.google.com/go/iam/apiv1/iampb: package imports google.golang.org/genproto/googleapis/api/annotations
was compiled with: @org_golang_google_genproto_googleapis_api//annotations:annotations
but was linked with: @org_golang_google_genproto//googleapis/api/annotations:annotations
See https://github.com/bazelbuild/rules_go/issues/1877.Are there any fixes I can make on my side?
jmhodges
commented
9 months ago @miracvbasaran ran into this while debugging some other problem and happen to know one possible answer: That Go code you're building might have a dependency on google.golang.org/genproto/googleapis/api but the target is defined as depending on @org_golang_google_genproto//googleapis/api/annotations:annotations (which maps to the parent module google.golang.org/genproto) but should instead depend on its submodule org_golang_google_genproto_googleapis_api//annotations:annotations (which maps to google.golang.org/genproto/googleapis/api). There's two different go.mods (parent module, subpackage module) which I found from looking at the pkg.go.dev pages for these packages.
This happens when you both the parent and subpackage external forms of the target gets added (even transitively) as dependencies!
The fetch of the parent package includes the code for the subpackage, so it's easy to depend on the parent's external and not notice. I think this happens just by explicit accidental inclusion of both externals in the deps but also can happen when the parent and subpackage's maintainers split them out after the parent package has been used by folks for a while and you can upgrade into that without noticing for a while.
Tangentially, it might be nice for rules_go to help detect when folks are accidentally depending on a subpackage's code from the parent package's external. (Maybe it does already! I'm currently upgrading from a 6 month old version)
remiphilippe
commented
9 months ago @jmhodges we're running into the exact same issue, do you happen to have a workaround?
link: package conflict error: google.golang.org/genproto/googleapis/cloud/location: package imports google.golang.org/genproto/googleapis/api/annotations
was compiled with: @org_golang_google_genproto//googleapis/api/annotations:annotations
but was linked with: @org_golang_google_genproto_googleapis_api//annotations:annotations
See https://github.com/bazelbuild/rules_go/issues/1877.Thanks!
edit: forgot to mention we also seen linker warnings:
DEBUG: /private/var/tmp/_bazel_/6108bdb662b1210897dd1ebdb34d9b4a/external/io_bazel_rules_go/go/private/actions/link.bzl:59:18: Multiple copies of google.golang.org/genproto/googleapis/api passed to the linker. Ignoring bazel-out/darwin-fastbuild/bin/external/org_golang_google_genproto/googleapis/api/api.a in favor of bazel-out/darwin-fastbuild/bin/external/org_golang_google_genproto_googleapis_api/api.a
DEBUG: /private/var/tmp/_bazel_/6108bdb662b1210897dd1ebdb34d9b4a/external/io_bazel_rules_go/go/private/actions/link.bzl:59:18: Multiple copies of google.golang.org/genproto/googleapis/api/annotations passed to the linker. Ignoring bazel-out/darwin-fastbuild/bin/external/org_golang_google_genproto/googleapis/api/annotations/annotations.a in favor of bazel-out/darwin-fastbuild/bin/external/org_golang_google_genproto_googleapis_api/annotations/annotations.a@remiphilippe I'd have to see more of the set up, but I believe that happens when the go_repository for the parent module and the go_repository for the submodule are very different versions and both are (transitively) depended on in a target. Something about there being a missing package in the submodule that is in the parent or vice versa. Often this is because some other dependency requires the older of the two or whatever (and doesn't show up in the errors because that's all working correctly)!
Oh, not sure if you know: you also have to account for the rules_go v0.41 change to how googleapis works now, so be sure to grab a version of it that matches your genproto submodule and your targets that need any of the genproto/... deps.
It's a real mess of upgrading golang.org/x/oauth2, cloud.google.com/go/... repos, google.golang.org/genproto/... repos and, of course, the rules_go 0.34 change to googleapis (iirc) while figuring out what externals are registering older versions of those you need to override. (e.g. golang.org/x/oauth2/google depending on cloud.google.com/go/compute/metadata while cloud.google.com/go/ stuff depends on golang.org/x/oauth2/ is surprisingly annoying and takes a bit to figure out!)
jmhodges
commented
9 months ago @remiphilippe Ah, though, the release that came out yesterday might be relevant to you: https://github.com/bazelbuild/rules_go/releases/tag/v0.44.1
remiphilippe
commented
9 months ago thanks @jmhodges just gave it a try, still the same. Will try to create a bare bone repro tomorrow, I'm 99% sure it's due to the deps that are pulled in by vault sdk (https://github.com/hashicorp/vault/tree/main/sdk) we use that for our tests, and this issue only happens in tests.
remiphilippe
commented
9 months ago so couldn't repro in an isolated env, but figured out a fix. Seems to be indeed linked to vault/sdk but not only. What I ended up doing is adding this in my deps:
build_directives = [
"gazelle:resolve go google.golang.org/genproto/googleapis/api/annotations @org_golang_google_genproto//googleapis/api/annotations", # keep
"gazelle:resolve go google.golang.org/genproto/googleapis/api @org_golang_google_genproto//googleapis/api", # keep
],for the following packages (I got a list from go mod graph, not all the packages require it, couldn't find a specific version common to all of these):
name = "com_google_cloud_go_compute",
name = "com_google_cloud_go_iam",
name = "com_google_cloud_go_kms",
name = "com_google_cloud_go_monitoring",
name = "io_etcd_go_etcd_api_v3",
name = "io_etcd_go_etcd_client_pkg_v3",
name = "io_opentelemetry_go_otel_exporters_otlp_otlpmetric_otlpmetricgrpc"
name = "io_opentelemetry_go_otel_exporters_otlp_otlptrace_otlptracegrpc",
name = "org_golang_google_api",
name = "org_golang_google_genproto_googleapis_api",everything builds fine now, I'll see if I can dig into more later. Thanks for the help!
fmeum
commented
9 months ago @remiphilippe Glad you found a fix.
Anyone interested in submitting a PR against https://github.com/bazelbuild/bazel-gazelle/blob/8fbf08c4da0a015a2489512aa8238d52f8e32ef4/internal/bzlmod/default_gazelle_overrides.bzl#L24 so that gazelle users get this by default?
miracvbasaran
commented
9 months ago Thanks @jmhodges & @remiphilippe! @remiphilippe's fix (https://github.com/bazelbuild/rules_go/issues/1877#issuecomment-1874616324) did the trick for me.
However, I also had the add the build_directives to:
name = "com_google_cloud_go_storage"
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:
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:
WORKSPACE
BUILD file
Go file (package + imports)
Splitting up the BUILD file in the typical way fixes the problem:
but results in this warning on subsequent gazelle runs