compilepkg: fix race when run without sandbox

[sluongng]

compilepkg: fix race when run without sandbox

On platforms without sandbox execution support such as Windows, multiple
go_test targets declared in the same package would cause a race condition.

[
    go_library(
        name = "same_package_{}".format(i),
        srcs = ["same_package.go"],
        importpath = "github.com/bazelbuild/rules_go/tests/core/go_test/same_package_{}".format(i),
        x_defs = {
            "name": "{}".format(i),
        },
    )
    for i in range(1, 80)
]

[
    go_test(
        name = "same_package_{}_test".format(i),
        srcs = ["same_package_test.go"],
        embed = [":same_package_{}".format(i)],
    )
    for i in range(1, 80)
]

For each of go_test targets, there would be internal archive and
external archive that needs to compile from source files. However, due
to testfilter, external archive would end up without any Go source and
thus compilepkg needs to generate a dummy source file _empty.go to
trick 'go tool compile'.

This _empty.go file is generated in the output path of the Bazel
package thus multiple go_test targets in the same package would generate
this file using the same path. On a sandbox supported platform,
this is not a problem as the full path is prefixed with the sandbox root
dir. However, without sandbox, the full path is the same for each
go_test's external archive compilation, leading to a race condition
where multiple compilations running in parallel would compete to
create/delete the same file.

Fix this by using the unique importPath to create a directory to store
the _empty.go file. This would effectively give each go_test compilation
it's own _empty.go file in non-sandbox environment.

Worth to note that in current solution, we still have yet to solve
problem when 'importPath' is not provided. For example, in a setup like
this where go_library does not exist, importPath will be blank and
thus causes similar issues on non-sandbox platforms.

[
    go_test(
        name = "same_package_{}_test".format(i),
        srcs = ["same_package_test.go"],
    )
    for i in range(1, 80)
]

Although this is a valid use case, we recommend avoid setting up tests
this way. Instead, you could setup a single go_test target with multiple
shards to achieve similar result:

go_test(
    name = "same_package_{}_test".format(i),
    srcs = ["same_package_test.go"],
    shard_count = 80,
)

For more information, please review Bazel Test Sharding documentation(1)

(1): https://docs.bazel.build/versions/main/test-encyclopedia.html#test-sharding

Fix #3144

[sluongng]

This PR is ready for review.

The detail contexts is shared in #3144

[sluongng]

Merged latest master into this PR to resolve the upstream merge-conflict

[sluongng]

@achew22 @linzhp could you guys please take a look at this PR?

The fix is relatively small and there are tests included.

[linzhp]

Thanks for the fix. It also make the Windows CI on rules_go more stable. Minor suggestions

[sluongng]

Looks like all CI have passed @linzhp PTAL 🙏

[fmeum]

@linzhp I don't have write access yet, could you do the merge if you are satisfied with the PR?

[sluongng]

FWIW, I think using target's name in the action input might be an anti pattern.
It will become part of the key calculated in action cache, so similar actions would not re-use cached action if they have different names:

[
    go_test(
        name = "test_{}".format(i),
        srcs = ["my_test.go"],
    )
    for i in range(1, 80)
]

In such (rare?) case, the 80 test binary compilation/link might run not once, but 80 times.

I am convinced that this is not an issue worth solving right now 🤔
The use case is very rare that it might not matter at all.

But thinking about the (unrelated) use case of constructing a synthetic tests for RBE, where I want the build graph to be widen to benefit from parallelism, made me think back about this PR and how it might have unintentionally enabled such use case.

[fmeum]

Avoiding this would be harder than expected though: The command line is part of the action key computation and necessarily contains the path of the output of the compile/link actions, which is already derived from the test name. If I'm not missing anything, the recent change thus should make a difference here.

[sluongng]

I quickly created a test like this:

bazel aquery '//tests/core/go_test:same_package_1_test' > /tmp/same_package_1.txt
bazel aquery '//tests/core/go_test:same_package_2_test' > /tmp/same_package_2.txt

diff -u /tmp/same_package_*

# less noisy
diff -u /tmp/same_package_* | grep -vE '(1|2)_test'

Then I ran the test before and after reverting this commit.

# check out my commit
> git checkout 1e9931b0c2ec4c89f6f651dc9cfccda75d95e1d5

# run test

# revert //go dir
> git checkout 1e9931b0c2ec4c89f6f651dc9cfccda75d95e1d5^ -- go
> git status
HEAD detached at 1e9931b0
Changes to be committed:
  (use "git restore --staged <file>..." to unstage)
        modified:   go/private/actions/compilepkg.bzl
        modified:   go/tools/builders/compilepkg.go

And the result seems to be that ActionKey values were always updated as target's name has always been part of the Command Line input.

I event tried doing this on top of my patch

master ~/work/misc/rules_go> git diff
diff --git a/tests/core/go_test/BUILD.bazel b/tests/core/go_test/BUILD.bazel
index 829ca8d5..3e58db06 100644
--- a/tests/core/go_test/BUILD.bazel
+++ b/tests/core/go_test/BUILD.bazel
@@ -199,6 +199,7 @@ go_library(
     go_test(
         name = "same_package_{}_test".format(i),
         srcs = ["same_package_test.go"],
+        importpath = "blah/blah",
     )
     for i in range(1, 80)
 ]

And then ran the same test: same result, ActionKey hash always changed.

---

So it's safe to conclude that this PR did not introduce a regression. 😇

I was being paranoid for no reason. Sorry for the noise 🙇