window: race condition when declare multiple go_library in the same directory

[sluongng]

What version of rules_go are you using?

Latest

What version of gazelle are you using?

Irrelevant

What version of Bazel are you using?

Latest

Does this issue reproduce with the latest releases of all the above?

yes

What operating system and processor architecture are you using?

rules_go Window CI

Any other potentially useful information about your toolchain?

Via our CI setup

What did you do?

Setup is described in #3138 (comment)

where we have something like this in the same BUILD file

[
    go_library(
        name = analyzer,
        srcs = ["analyzer.go"],
        importpath = "github.com/bazelbuild/rules_go/go/analyzer/staticcheck/" + analyzer,
        visibility = ["//visibility:public"],
        x_defs = {"name": analyzer},
        deps = ["//go/analyzer/staticcheck/util"],
    )
    for analyzer in ANALYZERS
]

[
    go_test(
        name = analyzer + "_test",
        srcs = ["analyzer_test.go"],
        embed = [analyzer],
    )
    for analyzer in ANALYZERS
]

What did you expect to see?

Window CI to successfully passed

What did you see instead?

Window CI reporting this error

open bazel-out\x64_windows-fastbuild\bin\go\analyzer\staticcheck\_empty.go: The system cannot find the file specified.

Most likely caused by this block https://github.com/bazelbuild/rules_go/blob/a2677c2c0af38a851bf381e013c45a27642892ac/go/tools/builders/compilepkg.go#L187-L204

Here the outpath value of each go_library target is the same bazel-out\x64_windows-fastbuild\bin\go\analyzer\staticcheck thus a compilation of one go_library was deleting the same _empty.go file of another compilation. This is most likely caused by Window lack of sandboxing support for these action to run in parallel bazelbuild/bazel#5136

[sluongng]

Some findings from #3145:

The issue is indeed happening exclusively to Window due to lack of sandboxing support.

This is caused exclusively due to having multiple go_test targets defined in the same directory/folder:

[
    go_test(
        name = analyzer + "_test",
        srcs = ["analyzer_test.go"],
        embed = [analyzer],
    )
    for analyzer in ANALYZERS
]

According to https://github.com/bazelbuild/rules_go/blob/fee2095e0a8219c93206be0b5adecfce6f32eac0/go/private/rules/test.bzl#L454-L486, go_test would compile twice, once for internal archive and once for external archive. During external package compilation, all the go source were filtered out of the compilation thus rules_go needed to create an _empty.go file in the folder/directory to compile with it.

However, because we have multiple go_test targets in the same directory and their compilation are being executed in parallel by Bazel, each of the compilation would race to create and delete _empty.go file the same path thus break the build.

The current workaround is to mark these go_test targets to be only compatible with platforms which support sandboxing

[
    go_test(
        name = analyzer + "_test",
        srcs = ["analyzer_test.go"],
        embed = [analyzer],
        target_compatible_with = select({
            "@platforms//os:osx": [],
            "@platforms//os:linux": [],
            "//conditions:default": ["@platforms//:incompatible"],
        }),
    )
    for analyzer in ANALYZERS
]

Not sure what the correct fix for this situation would be 🤔

[fmeum]

Is it necessary to delete the _empty.go? If not, then creating it in a temporary location and moving it to the output directory in an atomic operation should ensure that concurrent build actions can consistently read the file.

[sluongng]

Is it necessary to delete the _empty.go?

I tried commenting out the defer deletion and the file stayed, however each compilation kept on writing into the file which then create an invalid go file with multiple package empty lines.

If not, then creating it in a temporary location and moving it to the output directory in an atomic operation should ensure that concurrent build actions can consistently read the file.

Let me try this temp file approach

@@ -190,17 +190,20 @@ func compileArchive(
                // objects.
                // _empty.go needs to be in a deterministic location (not tmpdir) in order
                // to ensure deterministic output
-               emptyPath := filepath.Join(filepath.Dir(outPath), "_empty.go")
-               if err := ioutil.WriteFile(emptyPath, []byte("package empty\n"), 0666); err != nil {
+               f, err := os.CreateTemp("", "_empty.go")
+               if err != nil {
+                       return err
+               }
+               defer os.Remove(f.Name())
+               if err := ioutil.WriteFile(f.Name(), []byte("package empty\n"), 0666); err != nil {
                        return err
                }
                srcs.goSrcs = append(srcs.goSrcs, fileInfo{
-                       filename: emptyPath,
+                       filename: f.Name(),
                        ext:      goExt,
                        matched:  true,
                        pkg:      "empty",
                })
-               defer os.Remove(emptyPath)
        }
        packageName := srcs.goSrcs[0].pkg
        var goSrcs, cgoSrcs []string

[sluongng]

hmm it seems like using os.CreateTemp would work but it would fail the //tests/integration/reproducibility test 🤔
which makes sense because we are using an undeterministic file path for the compilation. The good news is that multiple go_test targets in same dir can run on Windows now without issues.

I will try this next

@@ -190,7 +190,7 @@ func compileArchive(
                // objects.
                // _empty.go needs to be in a deterministic location (not tmpdir) in order
                // to ensure deterministic output
-               emptyPath := filepath.Join(filepath.Dir(outPath), "_empty.go")
+               emptyPath := filepath.Join(filepath.Dir(outPath), sanitizePathForIdentifier(importPath), "_empty.go")
                if err := ioutil.WriteFile(emptyPath, []byte("package empty\n"), 0666); err != nil {
                        return err
                }

[sluongng]

This seems to work

@@ -188,19 +188,26 @@ func compileArchive(
                // We need to run the compiler to create a valid archive, even if there's
                // nothing in it. GoPack will complain if we try to add assembly or cgo
                // objects.
+               //
                // _empty.go needs to be in a deterministic location (not tmpdir) in order
                // to ensure deterministic output
-               emptyPath := filepath.Join(filepath.Dir(outPath), "_empty.go")
+               emptyDir := filepath.Join(filepath.Dir(outPath), sanitizePathForIdentifier(importPath))
+               err := os.Mkdir(emptyDir, 0700)
+               if err != nil {
+                       return err
+               }
+               defer os.RemoveAll(emptyDir)
+               emptyPath := filepath.Join(emptyDir, "_empty.go")
                if err := ioutil.WriteFile(emptyPath, []byte("package empty\n"), 0666); err != nil {
                        return err
                }
+               defer os.Remove(emptyPath)
                srcs.goSrcs = append(srcs.goSrcs, fileInfo{
                        filename: emptyPath,
                        ext:      goExt,
                        matched:  true,
                        pkg:      "empty",
                })
-               defer os.Remove(emptyPath)
        }
        packageName := srcs.goSrcs[0].pkg
        var goSrcs, cgoSrcs []string

[sluongng]

Turns out we can just define go_test without go_library and thus having empty importPath 🙃

[sluongng]

I figured for the case where importPath being empty for a go_test, the test itself could be rewritten as a single test target with multiple test shards. So I will only implement the solution with the assumption that importPath is not empty and document that we recommend folks to use sharded testing for other use case.