go_test: linker conflict when external test indirectly imports library under test #1877
Comments
|
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. |
|
Here's a minimal test case: Run this to see the error: The source of the issue appears to be due to these two factors:
In this case, the dependencies look like: a/a.go -> b/b.go package "b" is not allowed to depend upon package "a", but package "b_test" should be allowed to. |
|
It seems like the external test package |
|
What about automatically creating 2 go_test rules, one for each package? |
|
We used to do that ( It doesn't really solve this problem though. Try running In order to fix this, we'd need to recompile every transitive dependency of |
|
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. /: |
|
@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 |
|
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. |
Summary:
My changes to github.com/grailbio/bio/encoding/bam in D42945 apparently caused
a Go compiler import-related error:
/home/grail-builder/.cache/bazel/_bazel_grail-builder/b20b5481828eeefc6fcc4ab4c0b87731/sandbox/processwrapper-sandbox/724/execroot/grail/go/src/github.com/grailbio/bio/encoding/bam/shard_test.go:9:2: internal compiler error: conflicting package heights 21 and 19 for path "github.com/grailbio/bio/encoding/bam"
Please file a bug report including a short program that triggers the error.
https://golang.org/issue/new
My best guess is this is a bug in rules_go, maybe [1877](bazelbuild/rules_go#1877) or something similar.
I wasn't able to make a small repro case in [my own branch](https://github.com/josh-newman/bio/tree/gazelle), unfortunately.
However, I guessed this multipath dependency was the problem:
.--------------.
/ \
bam_test -> bamprovider -> bam
And with this revision, I no longer encounter the issue with D42945's `vcontext`
fix, so maybe this is really the issue.
Test Plan: Existing builds and tests.
Reviewers: sbagaria
Reviewed By: sbagaria
Maniphest Tasks: T30452
Differential Revision: https://phabricator.grailbio.com/D42946
fbshipit-source-id: 27a256b
jayconrod
changed the title
|
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 So we'd be stuck with an aspect. That would need to be gated by a flag on a 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. |
|
This is probably a dumb question: what does the Go CLI do, and what is the barrier from Bazel doing the same? |
|
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. |
|
Ah, I assumed that the go command managed to avoid the O(n*t) behavior somehow. Makes sense, thanks for the explanation. |
|
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. |
|
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 |
In general, packages should be compiled with the same dependencies they're linked against. We haven't been too strict about this in the past, and there are two common situations where this might not happen: 1) go_proto_library and go_library targets are linked for the same package, 2) an external test imports a library that imports the package being tested. Analysis will now detect both problems and will pass a descriptive message to the GoLink action. Making this a hard error would be an incompatible change, so for now, it will be reported if the incompatible_package_conflict_is_error setting is enabled (bazelbuild#1374) or if the linker fails. Fixes bazelbuild#1877 Updates bazelbuild#1374 Updates bazelbuild#2535
…2553) In general, packages should be compiled with the same dependencies they're linked against. We haven't been too strict about this in the past, and there are two common situations where this might not happen: 1) go_proto_library and go_library targets are linked for the same package, 2) an external test imports a library that imports the package being tested. Analysis will now detect both problems and will pass a descriptive message to the GoLink action. Making this a hard error would be an incompatible change, so for now, it will be reported if the incompatible_package_conflict_is_error setting is enabled (#1374) or if the linker fails. Fixes #1877 Updates #1374 Updates #2535
All of these tests fail to link because the external test package indirectly imports the internal test package. See bazelbuild#1877 for the original issue. This pattern is difficult to implement in Bazel, since it requires an aspect to recompile test dependencies. It's generally possible to work around it, so now this fails with a descriptive error message. Fixes bazelbuild#2535
…#2554) All of these tests fail to link because the external test package indirectly imports the internal test package. See #1877 for the original issue. This pattern is difficult to implement in Bazel, since it requires an aspect to recompile test dependencies. It's generally possible to work around it, so now this fails with a descriptive error message. Fixes #2535
…2553) In general, packages should be compiled with the same dependencies they're linked against. We haven't been too strict about this in the past, and there are two common situations where this might not happen: 1) go_proto_library and go_library targets are linked for the same package, 2) an external test imports a library that imports the package being tested. Analysis will now detect both problems and will pass a descriptive message to the GoLink action. Making this a hard error would be an incompatible change, so for now, it will be reported if the incompatible_package_conflict_is_error setting is enabled (#1374) or if the linker fails. Fixes #1877 Updates #1374 Updates #2535
…2553) In general, packages should be compiled with the same dependencies they're linked against. We haven't been too strict about this in the past, and there are two common situations where this might not happen: 1) go_proto_library and go_library targets are linked for the same package, 2) an external test imports a library that imports the package being tested. Analysis will now detect both problems and will pass a descriptive message to the GoLink action. Making this a hard error would be an incompatible change, so for now, it will be reported if the incompatible_package_conflict_is_error setting is enabled (#1374) or if the linker fails. Fixes #1877 Updates #1374 Updates #2535
…2579) go_test, like 'go test', splits tests into two separate archives: an internal archive ('package foo') and an external archive ('package foo_test'). The library under test is embedded into the internal archive. The external archive may import it and may depend on symbols defined in the internal test files. To avoid conflicts, the library under test must not be linked into the test binary, since the internal test archive embeds the same sources. Libraries imported by the external test that transitively import the library under test must be recompiled too, or the linker will complain that export data they were compiled with doesn't match the export data they are linked with. This PR adds a function, used in go_test, that identifies which archives may need to be recompiled, then declares new output files and actions to recompile them. This is an unfortunately an expensive process requiring O(V+E) time and space in the size of the test's dependency graph for each test. Fixes #1877
|
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. |
|
@jayconrod : I tried 464eb9c on the current master version of Baseplate.go and it no longer show the warnings when we run |
|
Glad to hear that! I'm also no longer seeing the warning on |
Should fix bazelbuild/rules_go/issues/1877 Signed-off-by: Stephen Augustus <saugustus@vmware.com>
Should address bazelbuild/rules_go/issues/1877 Signed-off-by: Stephen Augustus <saugustus@vmware.com>
…2579) go_test, like 'go test', splits tests into two separate archives: an internal archive ('package foo') and an external archive ('package foo_test'). The library under test is embedded into the internal archive. The external archive may import it and may depend on symbols defined in the internal test files. To avoid conflicts, the library under test must not be linked into the test binary, since the internal test archive embeds the same sources. Libraries imported by the external test that transitively import the library under test must be recompiled too, or the linker will complain that export data they were compiled with doesn't match the export data they are linked with. This PR adds a function, used in go_test, that identifies which archives may need to be recompiled, then declares new output files and actions to recompile them. This is an unfortunately an expensive process requiring O(V+E) time and space in the size of the test's dependency graph for each test. Fixes #1877
…2579) go_test, like 'go test', splits tests into two separate archives: an internal archive ('package foo') and an external archive ('package foo_test'). The library under test is embedded into the internal archive. The external archive may import it and may depend on symbols defined in the internal test files. To avoid conflicts, the library under test must not be linked into the test binary, since the internal test archive embeds the same sources. Libraries imported by the external test that transitively import the library under test must be recompiled too, or the linker will complain that export data they were compiled with doesn't match the export data they are linked with. This PR adds a function, used in go_test, that identifies which archives may need to be recompiled, then declares new output files and actions to recompile them. This is an unfortunately an expensive process requiring O(V+E) time and space in the size of the test's dependency graph for each test. Fixes #1877
For compatibility with Bazel 1.2.0 Updates bazelbuild#1877
For compatibility with Bazel 1.2.0 Updates #1877
For compatibility with Bazel 1.2.0 Updates #1877
For compatibility with Bazel 1.2.0 Updates #1877
Upgrade rules_go to v0.23.6, which fixed bazelbuild/rules_go#1877 that's affecting us. Also add 'size = "small"' to breakerbp's test target.
Upgrade rules_go to v0.23.6, which fixed bazelbuild/rules_go#1877 that's affecting us. Also add 'size = "small"' to breakerbp's test target.
This is a totally valid thing to do in go. Should we have a separate tracking issue to support it? |
|
@ajwerner I think this is fixed. Have you tried? |
|
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: Are there any fixes I can make on my side? |
|
@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 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 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) |
|
@jmhodges we're running into the exact same issue, do you happen to have a workaround? Thanks! edit: |
|
@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 It's a real mess of upgrading |
|
@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 |
|
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. |
|
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: for the following packages (I got a list from everything builds fine now, I'll see if I can dig into more later. |
|
@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? |
|
Thanks @jmhodges & @remiphilippe! @remiphilippe's fix (#1877 (comment)) did the trick for me. However, I also had the add the |
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 (ea0abdd). 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
The text was updated successfully, but these errors were encountered: