bazel: tests that depend on buildutil.VerifyNoImports() fail under remote execution

[rickystewart]

The dependency on the build package is breaking stuff here. I don't think these tests are doing anything that Bazel's visibility rules couldn't do.

    build.go:56: go/build: go list github.com/cockroachdb/cockroach/pkg/server/diagnostics: exec: "go": executable file not found in $PATH

Epic CRDB-8308

Jira issue: CRDB-11962

[petermattis]

I don't think these tests are doing anything that Bazel's visibility rules couldn't do.

I'm not sure this is true, and even if it is true specifying Bazel visibility rules that correspond to what tests using buildutil.VerifyNoImports() are doing might be prohibitively onerous. Perhaps I'm missing something. For example, sql/dep_test.go verifies that //pkg/sql does not include //pkg/storage or //c-deps. How would this be specified using Bazel visibility rules? I think you'd need to specify in pkg/storage/BUILD.bazel the packages that the rules there are visible too. There are currently 61 packages which depend on //pkg/storage, so that list is already long. Then you have to make sure that nothing that //pkg/sql depends on transitively imports //pkg/storage. I'm not seeing how visibility rules can readily accomplish this.

Using bazel query seems like a more fruitful approach:

~ ~/go/src/github.com/cockroachdb/cockroach pmattis/fix-publish-artifacts-test-main bazel query 'somepath("//pkg/sql:sql", "//pkg/storage")'
//pkg/sql:sql
//pkg/kv/kvclient:kvclient
//pkg/storage:storage

But how to use bazel query from a test?

Also worthy of note is that sql/dep_test.go is relatively simple. The logic in cli/flags_test.go:TestNoLinkForbidden() is more complicated.

[rickystewart]

I'm not sure this is true, and even if it is true specifying Bazel visibility rules that correspond to what tests using buildutil.VerifyNoImports() are doing might be prohibitively onerous. Perhaps I'm missing something. For example, sql/dep_test.go verifies that //pkg/sql does not include //pkg/storage or //c-deps. How would this be specified using Bazel visibility rules? I think you'd need to specify in pkg/storage/BUILD.bazel the packages that the rules there are visible too. There are currently 61 packages which depend on //pkg/storage, so that list is already long. Then you have to make sure that nothing that //pkg/sql depends on transitively imports //pkg/storage. I'm not seeing how visibility rules can readily accomplish this.

In my opinion, nothing you've described is prohibitively onerous. We can run a one-time query to find which libraries depend on //pkg/storage, substitute the ["//visibility:public"] in pkg/storage/BUILD.bazel with that list of libraries, and then no library will be able to take a dependency on //pkg/storage unless we explicitly add it to the allow-list. This requires no additional upkeep outside of having to add a new library to the list each time you need it -- and your build will trivially fail until you fix it, so there's not really a risk of something slipping through the cracks or of having to wait for a long CI cycle to realize your code is broken.

Of course I'm biased and there is disagreement about how much friction this actually is, so we haven't forced anything like this on anyone, but I do think that as the monorepo grows we'll have to make changes in this direction either way.

I agree that bazel query is another way to accomplish the same thing. Running bazel querys from Go tests is a nonstarter but we already do bazel querys from CI and I'm sure that we will continue adding bazel querys to validate extra stuff like this. The trade-off is that implementing it this way means that you don't get the nice behavior of having the build immediately fail if something is wrong as would be the case for visibility restrictions, so this means more CI iterations.

RE: cli/flags_test.go, bazel query or tightening visibility restrictions would allow you to implement the same logic as well. An exception to this is that bazel doesn't understand how the stdlib works to the same degree that go/build does, so I don't think you can write a bazel query to verify that cockroach does not depend on the stdlib testing or go/build packages. I think you can write an Analyzer that does the same thing and we can hook that into Bazel easily enough but I haven't looked closely at this.

[petermattis]

In my opinion, nothing you've described is prohibitively onerous. We can run a one-time query to find which libraries depend on //pkg/storage, substitute the ["//visibility:public"] in pkg/storage/BUILD.bazel with that list of libraries, and then no library will be able to take a dependency on //pkg/storage unless we explicitly add it to the allow-list. This requires no additional upkeep outside of having to add a new library to the list each time you need it -- and your build will trivially fail until you fix it, so there's not really a risk of something slipping through the cracks or of having to wait for a long CI cycle to realize your code is broken.

//kv/kvclient recently received a new dependency on //pkg/storage. When that was done, someone would have had to extend the visibility rule for //pkg/storage to include //kv/kvclient. Sounds innocuous enough, yet doing so would have broken the forbidden import restriction that we're trying to mandate with VerifyNoImports as //pkg/sql now transitively depends on //pkg/storage. In fact, I'm not actually clear on why pkg/sql:deps_test.go:TestNoLinkForbidden isn't currently failing.

I think you can write an Analyzer that does the same thing and we can hook that into Bazel easily enough but I haven't looked closely at this.

If you're referring to a vet Analyzer, then I agree that sounds like a promising direction.

[rickystewart]

//kv/kvclient recently received a new dependency on //pkg/storage. When that was done, someone would have had to extend the visibility rule for //pkg/storage to include //kv/kvclient. Sounds innocuous enough, yet doing so would have broken the forbidden import restriction that we're trying to mandate with VerifyNoImports as //pkg/sql now transitively depends on //pkg/storage. In fact, I'm not actually clear on why pkg/sql:deps_test.go:TestNoLinkForbidden isn't currently failing.

I'm not sure I'm following the argument here. In the scenario you're outlining it sounds like this SHOULD break the build because we're violating the invariant that we don't want pkg/sql to depend on pkg/storage. Am I misunderstanding something?

[petermattis]

I'm not sure I'm following the argument here. In the scenario you're outlining it sounds like this SHOULD break the build because we're violating the invariant that we don't want pkg/sql to depend on pkg/storage. Am I misunderstanding something?

In the scenario I outlined we should have broken the build, but I don't think it would have. //pkg/storage would not have been a direct dependency of //pkg/sql. As far as I understand, bazel visibility rules only restrict what rules can directly depend on a rule, not the transitive dependencies which is what we need to restrict. Perhaps I'm not understanding how this would work. How are you imagining visibility rules would prohibit //pkg/sql from transitively depending on //pkg/storage?

[ajwerner]

TestNoLinkForbidden has been broken for ages. #74119 tracks a recent rediscovery of this problem. I think I took a stab at in now going on a year ago one friday #64450. From some quick digging, I think it broke some time around June 2020.

[ajwerner]

I feel like I'd address this not with visibility rules, but with genquery and some simple test that looks at its output. Namely, I think I'd create a bazel macro which internally leveraged genquery and something to assert that that the query result was empty. Imagine for pkg/sql/BUILD.bazel

verify_no_imports(
   library = [":sql"]
   does_not_import = [
       "//pkg/storage",
   ]
)

Today this would fail, as bazel query "somepath(//pkg/sql, //pkg/storage)" gives us:

//pkg/sql:sql
//pkg/kv/kvclient:kvclient
//pkg/storage:storage

This should be a simple macro to write.

[rickystewart]

Ah yes, sorry I was misunderstanding the point about transitive dependencies. Yes, we can build on top of genquery for this.

[petermattis]

genquery looks like exactly what we want to use. I suspect something like that existed, but was completely failing to find it while Googling yesterday. Thanks, @ajwerner.

I think the output from genquery could be fed into a Go test, in case we want to do processing of that output that is more complex than would be reasonable to do in a macro.

[rickystewart]

This function no longer exists as of #103059, and we have a suitable replacement in disallowed_imports_test.