Create empty `.go` file in a random location #3566

fmeum · 2023-05-19T16:35:00Z

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

Isolating the empty .go files generated for targets without sources
and removing them after a build turned out not to solve issues with
concurrent unsandboxed builds causing races. Instead, just generate the
file in a temporary location, which for a truly empty file does not
result in (non-hermetic) source file paths being included in the
archive.

Along the way fix a potential source of non-hermeticity in
go_bazel_test.

Which issues(s) does this PR fix?

Fixes #3558

Other notes for review

Isolating the empty `.go` files generated for targets without sources and removing them after a build turned out not to solve issues with concurrent unsandboxed builds causing races. Instead, just generate the file in a temporary location, which for a truly empty file does not result in (non-hermetic) source file paths being included in the archive. Along the way fix a potential source of non-hermeticity in `go_bazel_test`.

fmeum · 2023-05-20T18:44:59Z

cc @sluongng

linzhp

Tests passed in Uber

sluongng

Thanks a ton for fixing this. I did not know that using a random path would still ensure the hermeticity for the action the last time I touch this code path.

I have some minor nits but love the overall solution 👍

sluongng · 2023-05-21T16:45:09Z

go/tools/builders/compilepkg.go

+		defer os.Remove(emptyGoFile.Name())
+		defer emptyGoFile.Close()


Nit: it would be a bit easier to read if we combine these 2 defer into 1 single defer func() {}.
It's a bit hard to read here whether we are intentionally rm-ing an opened file handle or not.

Then we could also add err check for the os.Remove() call.

defer func() {} would definitely be more readable. But what would we do if os.Remove() or Close() return an error? This is part of best-effort cleanup, so I don't think we would want to fail compilation over it.

sluongng · 2023-05-21T16:49:01Z

go/tools/builders/compilepkg.go

+		if _, err := emptyGoFile.WriteString("package empty\n"); err != nil {
+			return err
+		}
+		if err := emptyGoFile.Close(); err != nil {


So we are closing the same file... twice. And we ignore the error happening when we close the second time inside the defer.

sluongng · 2023-05-21T16:49:56Z

tests/core/go_library/no_srcs_test.go

+func Test(t *testing.T) {
+	commonArgs := []string{
+		"--spawn_strategy=local",
+		"--compilation_mode=dbg",


any particular reason why dbg was needed here?

It's not needed, but I wanted to ensure that the output does include all debug symbols, even if we move to not generate them in other compilation modes in the future.

For more information: https://github.com/bazelbuild/rules_go/releases/tag/v0.40.1 https://github.com/bazelbuild/rules_go/releases/tag/v0.40.0 This should help with buildbuddy-io/buildbuddy-internal#2314 thanks to bazel-contrib/rules_go#3566

Isolating the empty `.go` files generated for targets without sources and removing them after a build turned out not to solve issues with concurrent unsandboxed builds causing races. Instead, just generate the file in a temporary location, which for a truly empty file does not result in (non-hermetic) source file paths being included in the archive. Along the way fix a potential source of non-hermeticity in `go_bazel_test`.

fmeum force-pushed the 3558-speculative-fix branch 2 times, most recently from 9e919ce to 1da4247 Compare May 19, 2023 22:29

fmeum force-pushed the 3558-speculative-fix branch from 1da4247 to 3e9f43c Compare May 20, 2023 18:42

fmeum changed the title ~~Leave empty .go file in the output tree~~ Create empty .go file in a random location May 20, 2023

fmeum marked this pull request as ready for review May 20, 2023 18:44

fmeum requested review from linzhp and achew22 May 20, 2023 18:44

linzhp approved these changes May 21, 2023

View reviewed changes

linzhp merged commit c3e33c3 into bazel-contrib:master May 21, 2023

sluongng reviewed May 21, 2023

View reviewed changes

fmeum deleted the 3558-speculative-fix branch May 21, 2023 17:00

sluongng mentioned this pull request Jul 5, 2023

rules_go: upgrade to v0.40.1 buildbuddy-io/buildbuddy#4250

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create empty `.go` file in a random location #3566

Create empty `.go` file in a random location #3566

fmeum commented May 19, 2023 •

edited

Loading

fmeum commented May 20, 2023

linzhp left a comment

sluongng left a comment

sluongng May 21, 2023

fmeum May 21, 2023

sluongng May 21, 2023

sluongng May 21, 2023

fmeum May 21, 2023

		defer os.Remove(emptyGoFile.Name())
		defer emptyGoFile.Close()

Create empty .go file in a random location #3566

Create empty .go file in a random location #3566

Conversation

fmeum commented May 19, 2023 • edited Loading

fmeum commented May 20, 2023

linzhp left a comment

Choose a reason for hiding this comment

sluongng left a comment

Choose a reason for hiding this comment

sluongng May 21, 2023

Choose a reason for hiding this comment

fmeum May 21, 2023

Choose a reason for hiding this comment

sluongng May 21, 2023

Choose a reason for hiding this comment

sluongng May 21, 2023

Choose a reason for hiding this comment

fmeum May 21, 2023

Choose a reason for hiding this comment

Create empty `.go` file in a random location #3566

Create empty `.go` file in a random location #3566

fmeum commented May 19, 2023 •

edited

Loading