Repository gets polluted with _virtual_imports

[ignasl]

I have encountered an interesting issue that reproduces in Bazel 0.27.0 and later versions when dependency tree is a bit more complex.

Consider such set-up:

• There are two repositories: repo_one and repo_two -- links point to actual repositories where this issue reproduces.
• two:proto in repo_two depends on one:proto in repo_one
• one:proto in repo_one depends on three:proto in repo_two
• Apparently, it is important that one:proto must be at certain level within repo_two. In this test it is under module/submodule/one/src/main/proto/

In repo_two when running bazel build --sandbox_debug //... two problems occur:

1. //three/_virtual_imports/proto/three.proto gets copied (all Bazel versions after and including 0.27.0) -- repository gets polluted.
2. Build fails because several copies of three.proto gets passed to the protoc at the same time. Reproduced mostly on 0.28.1, but it seems to be not 100% consistent. Last arguments for protoc look like this:

bazel-out/darwin-fastbuild/bin/three/_virtual_imports/proto/_virtual_imports/proto/_virtual_imports/proto/three.proto
bazel-out/darwin-fastbuild/bin/three/_virtual_imports/proto/_virtual_imports/proto/three.proto
bazel-out/darwin-fastbuild/bin/three/_virtual_imports/proto/three.proto

I suspect that ScalaPBGenerator.setupIncludedProto is not completely correct way to go. Why is it necessary to copy proto files someplace?.. It seems like the working directory has changed in 0.27.0, which caused the copies to go into the working copy of the repository.

[ignasl]

@lberki, @ittaiz suggested you might have some insights.
@ianoc-stripe, any insights on the copying business?
Issue #779 seems closely related.

[ianoc-stripe]

Imports have been a bit of a pain in bazel. I believe
bazelbuild/bazel@f65cc72
is part of an effort to try fix this all properly. Each set of rules has been attempting afaik trying to set things up for protoc.

Something did change in 0.27, but i believe 0.29 or the 1.0 rc probably are the spots with the right new sets of code to maybe fix this all in a nice manner. I haven't had a chance to look at it more though. if you remove the copying tests in the repo here should fail

[ignasl]

The main difference seems to be the working directory. In 0.26.1 it is under sandbox and in 0.29.0 it was changed to the execroot:

• 0.26.1 -- /private/var/tmp/_bazel_<username>/a70e513916154e4e47d9c1e37dc1e85f/sandbox/darwin-sandbox/925/execroot/<repo>
• 0.29.0 -- /private/var/tmp/_bazel_<username>/a70e513916154e4e47d9c1e37dc1e85f/execroot/<repo>

Since ScalaPBGenerator.setupIncludedProto is copying files relative to the working directory, in 0.29.0 they end-up in the repository that is actually being built.

Smoke test show that removing (from our fork) this copy operation seems to help.

[ignasl]

Another interesting thing I noticed is that the list of identical proto increases together with number of attempts to run bazel build //.... The first build after clean is often successful, failures start with incremental builds.

[ittaiz]

@ignasl if you remove the copying and run this repo's build with bazel 29.1 do the tests pass? do the e2e tests pass (you need to run ./test_rules_scala.sh)?

[ittaiz]

@lberki if you can shed some light on this we'd be really grateful

[ignasl]

When I run the ./test_rules_scala.sh on master, without any changes, test_expect_failure/proto_source_root/dependency/_virtual_imports/ gets created. Which is good, because problem seems to be reproducing on rules_scala repo as well.
@ittaiz Removing copy operation (remove call to setupIncludedProto) fails to build.

[ittaiz]

ok. So right now we are blocked? (assuming we want new bazel version and to use the aspect based proto)

[ignasl]

Indeed we are -- either someone helps us with this one or we fix it ourselves. I am looking for solutions as well.

[Yannic]

This could be caused by bazelbuild/bazel#9215. I'll look into it.

[ignasl]

Why is it not correct to convert all paths to absolute and thus avoid copying?

[ianoc-stripe]

@ignasl the problem is the paths need to get presented to protoc in a means it'll be happy with/can handle the imports for other protos. You can do a -I arg and try build up the mapping of absolute file to expected relative location too i think, its also kinda messy.

[ignasl]

Thank you. I will try it out.