Pants incorrectly identifies cyclic dependencies between a module and its tests for Go

[fiffeek]

Describe the bug

Currently, I am using Bazel to manage a Go monorepo, and I wanted to migrate to pants for more effortless operability and maintenance
(have been successfully using Pants with python for side projects for a while now). However, as simple as it might have been,
the project does not build due to pants incorrectly identifying circular dependencies between packages.

In a simple setup where we have one file, one test file in the same folder, and a shared utils package for the test, pants does not build
due to identifying these as circular imports (while they are not). In short, in the following setup:

/src
---- /example  
-------- /testutils  
------------ BUILD  
------------ testutils.go  
-------- BUILD
-------- example.go
-------- example_test.go

Where example_test.go imports something from example.go and from testutils.go, and testutils.go imports something from example.go,
while keeping the standardized package naming conventions (for example.go -> example, and for example_test.go -> example_test)
Pants identifies the structure as cyclic (which should not be the case).

The dependencies in play here are:

• example.go (example package) - no deps
• example_test.go (example_test package) - depends on: example and testutils package
• testutils.go (testutils package) - depends on: example package

Thus, clearly, there is no cycle. While I understand possibly why it's a tricky situation, keeping go test files next to the actual code
gives the benefit of getting the coverage right when running it locally (as I tried to fix the above situation by moving test code from src to tests, but then the coverage information is lost). Moreover, bazel correctly identifies this situation and resolves the imports without the error, while pants results in:

@rule(pants.backend.go.util_rules.build_pkg_target.setup_build_go_package_target_request(src/example:example)) <-
@rule(pants.backend.go.util_rules.build_pkg_target.setup_build_go_package_target_request(src/example/testutils:testutils))
@rule(pants.backend.go.util_rules.build_pkg_target.setup_build_go_package_target_request(src/example:example)) <-

Which is a hint that test files are possibly bundled together with the actual code? (There is no way of defining go_tests goal as in python).

A complete example is available in my repository.

Splitting the rule in two will not work as mentioned:

go_package(
    sources = ["*.go", "!*_test.go"]
)

go_package(
    name = "example_test",
    sources = ["*_test.go"]
)

Will lead to ambiguous imports:

13:43:25.02 [WARN] Ambiguous mapping for import path ...

Pants version
2.13.0, 2.14.x*rc

OS
Linux/Macos

Additional info
One workaround that I tried was to manually split the package into two (with tests and without), but then the import is ambiguous and so it does not work correctly either.

My guess is that the desired behaviour here is to resolve the dependencies per test/non_test module in the same package, or split the go_package goal as is done with python. I am open to pick this up and work on it, just need a couple of pointers and set the right direction.

[Eric-Arellano]

Thanks for opening this! I agree that this is broken, and we aim for Pants to support workflows that you have before using Pants.

Indeed, Pants includes test files in our abstractions for a first party Go package. But we track what is a test file vs xtest file vs normal file, and we don't compile the test sources unless we're running tests:

pants/src/python/pants/backend/go/util_rules/build_pkg.py

Line 62 in 520aee6

for_tests: bool = False,

I think it'd likely be ideal if we can stick to having a single go_package target per directory, rather than needing to add a go_tests target. That would mean refactoring our code to avoid the cycle, which I'm not certain is possible. Barring that, we could indeed add a go_tests target.

cc @tdyas

[tdyas]

Some initial thoughts:

BuildGoPackageRequest only has the notion of one type of dependency currently (in the direct_dependencies) field.

There might be a need to split out xtest-only dependencies from the direct_dependencies in BuildGoPackageRequest. The information about regular versus xtest imports is already known at package analysis time. And later on when running xtests, the BuildGoPackageRequest for the xtest is restricted to only dependencies for xtest imports.

The "middle" of that build process doesn't make a distinction though because the Go rules just infer a dependency between go_package targets without regard to which part of the package (regular or xtest) needs the dependency. So the dependency lookup logic just puts everything in direct_dependencies on BuildGoPackageRequest.

Some unorganized thoughts on what to explore next:

• We already model xtest code as a distinct synthetic package when running tests. I wonder if we should have a generated target to represent xtest code, which would help break the cycle. This would allow the user to continue to just specify go_package, but would allow Pants to properly separate the dependencies by automatically generating a target to represent them.
• Another solution that modifies BuildGoPackageRequest to track xtest dependencies and prevent a cycle (but still allow a cycle at the go_package level) might also work. The risk here is whether the mere existence of a cycle among go_package targets is the issue. It also means that the test code would need to do some amount of resolving xtest dependencies from targets instead of just relying on the already-resolved BuildGoPackageRequest tree.

[fiffeek]

Ideally use cases like this are supported out of the box otherwise after adopting and tailoring the code it simply does not build.

That said, I extend this thread a little bit by explaining a simple workaround that uncovered another issue with generating coverage for Go. As I said in the original post, it is easy to refactor the dependency cycle by extracting all the test code somewhere else (let's say all code lives under /src in the project root, and all the tests live under /test). That is somewhat feasible and what I have encountered with older build systems (mvn/gradle). This solves the dependency cycle by removing the link from example_test to example (which now does not point to the same package [in pants' terminology, in go's they are/were separate]).

The issue with the above is that coverage now cannot be generated for any of the packages. As when I tried (pants 2.14.0rc3) the generated coverage file is empty. Go generates coverage only in the local context by default, that can be extended, though, by coverpkg which is currently unsupported by pants.

I reckon all the code under test will be included in the executable that executes the tests, either way, so adding the support for the above should be a reasonable solution (a simple flag to passthrough an argument over coverpkg, or a boolean flag to output coverage on all the dependencies of the tests in the executable). Would you have anything against me doing the work on this? (Do correct me if the assumptions are wrong in any of the statements here)

Thanks for the response and the help, appreciate it!

(One other request: Could you assign the go backend label to the issue?)

---

EDIT:
I guess I found a place that needs to be extended as well to support coverage outside of the main package

[tdyas]

Ideally use cases like this are supported out of the box otherwise after adopting and tailoring the code it simply does not build.

I agree. I have been hesitant to remove the "experimental" label on the Go backend due to issues like this one. The Go backend definitely is missing feature equivalence to go. (This issue is also a reminder that I need to write up a "caveats" section for the documentation about how Pants currently diverges from go and Bazel rules_go.)

it is easy to refactor the dependency cycle by extracting all the test code somewhere else

The original intent for the Pants Go backend was that it should work with Go packages as much as possible as go would work with that same code. There should be a way to avoid requiring the user to move code around:

A "target generation" solution would probably be a good solution here. As context, a target type in Pants can be configured (in rules) as a "target generator" which programmatically generates "generated targets" that conceptually live under the generator target. For example, the go_mod target type is a target generator; it generates go_third_party_package targets automatically for any configured dependencies in the go.mod file for the module. (SeeTargetGenerator and its subclasses including GoModTarget.)

The go_package target type should become a target generator which generates a "xtest" target type. Any xtest imports from package analysis should be placed on the generated xtest target type and not the parent go_package target during dependency inference.

The test goal would then be updated to use a package's xtest target when it needs to generate the synthetic BuildGoPackageRequest for the xtest package.

Would you have anything against me doing the work on this?

Not at all. I definitely want others to help out with the Go backend. It has been mostly a side project for me given higher priority items at work.

As for coverage of packages other than the one under test, this is a known limitation. I left out that support in order to ship coverage functionality for the "package under test" sooner. #13201 covers the coverage support generally. Could you open an issue focused on the -coverpkg support? I can then add additional context there about how I had planned to solve it and what the currently blockers for that support are.

[Eric-Arellano]

The go_package target type should become a target generator which generates a "xtest" target type. Any xtest imports from package analysis should be placed on the generated xtest target type and not the parent go_package target during dependency inference.

Ah, that generally sounds like a good design. It conceptually breaks Pants's view of target generators though: we assume target generators solely exist for boilerplate reduction, not that they are also meaningful "atoms" in themselves. We do a lot of things like replacing a target generator in dependencies with all the generated targets, so it acts like an alias; those would behave improperly because we need to preserve the original target generator, in this case.

I suspect changing this target generator paradigm would be an uphill battle, but could be worth considering.

Alternatively, I wonder if we can refactor the code to get things working as is. Or, manually add a go_xtest_package target. Or, allow you to have two go_package targets w/o getting the ambiguity error by recognizing one is an xtest.

[tdyas]

An explicit go_xtest_package would be a good start with tailor generating it as needed. We can then defer the work for generating it via target generation until after we get the code pattern in this issue working.

"Fun" historical fact about the Go backend: The early iterations of the Go backend used tailor to generate explicit targets for all third party dependencies. Target generation was written in order to reduce that boilerplate.

[tdyas]

I suspect changing this target generator paradigm would be an uphill battle, but could be worth considering.
Alternatively, I wonder if we can refactor the code to get things working as is. Or, manually add a go_xtest_package target. Or, allow you to have two go_package targets w/o getting the ambiguity error by recognizing one is an xtest.

Or go_package could just be a generator and generate a "pkg" target for the regular package and a "xtest" target for the xtests.

[Eric-Arellano]

Or go_package could just be a generator and generate a "pkg" target for the regular package and a "xtest" target for the xtests.

It could, but I suspect this will be more complex because of target addresses. The generated targets have to have a #generated_name appended. What would we name the normal package? (Whereas xtest we could probably do something like #test)

I'd bias towards either an explicit go_xtest_package target, or refactoring our code to make this Just Work. I like the explictness of go_xtest_package more than allowing two go_package targets in the same directory; otherwise I think it's unclear why there are two of the same target types.

[tdyas]

A narrower alternative would be to refactor thisMultiGet in build_pkg_target.py to filter out xtest dependencies. (I am assuming for the moment that the rule cycle is caused by that MultiGet recursing eventually back into the same package. That assumption should be verified though.) Modifications would also be needed in goals/test.py to ensure xtest-only dependencies are constructed correctly for the synthetic xtest package.

[Eric-Arellano]

A narrower alternative would be to refactor this MultiGet in build_pkg_target.py to filter out xtest dependencies. (I am assuming for the moment that the rule cycle is caused by that MultiGet recursing eventually back into the same package. That assumption should be verified though.) Modifications would also be needed in goals/test.py to ensure xtest-only dependencies are constructed correctly for the synthetic xtest package.

If possible, this seems like the best approach to start with. It keeps things simple for users and has the behavior expected by @fiffeek.

@fiffeek if you have any interest, we'd definitely welcome a patch making this change -- definitely happy to give more instructions if you're not sure what we mean, including talking about it on Slack if easier: https://www.pantsbuild.org/docs/getting-help

[fiffeek]

Hey, thanks for the extensive comments and support on this :)

I can definitely take a look and would be keen on helping. I looked at the code and have a vague idea of how to proceed, will drop a message on Slack when stuck and I should start work on this soonish. That said, I cannot promise any tight deadline so if this issue is somewhat essential and you'd rather have it fixed asap then feel free to start the work.