chrisnovakovic commented 3 weeks ago

Go version

go version go1.23.0 linux/amd64

Output of `go env` in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/csn/please-go-rules/plz-out/tmp/test/root_test/root_test._test/run_1/.cache/go-build'
GOENV='/home/csn/please-go-rules/plz-out/tmp/test/root_test/root_test._test/run_1/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/csn/please-go-rules/plz-out/tmp/test/root_test/root_test._test/run_1/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/csn/please-go-rules/plz-out/tmp/test/root_test/root_test._test/run_1/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/csn/please-go-rules/plz-out/bin/third_party/go/toolchain'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/csn/please-go-rules/plz-out/bin/third_party/go/toolchain/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.23.0'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/home/csn/please-go-rules/plz-out/tmp/test/root_test/root_test._test/run_1/.config/go/telemetry'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/csn/please-go-rules/plz-out/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/home/csn/please-go-rules/plz-out/tmp/test/root_test/root_test._test/run_1/go-build1923171216=/tmp/go-build -gno-record-gcc-switches'

What did you do?

The Please build system performs build actions in an environment that is isolated from the host system as much as possible. In particular, the inputs for a build action are linked into a temporary directory, and the build action's environment is sanitised - you'll notice the impact this has on the Go environment in the output of go env above. The go-rules plugin contains build definitions for compiling Go code with a Go toolchain, similarly to Bazel's rules_go.

After bumping go-rules' Go toolchain to 1.23.0, we've run into a situation where Please can't clean up the temporary directory it creates after a target has been built:

28 test targets and 78 tests run; 75 passed, 1 errored, 2 skipped.
Total time: 2m18.18s real, 5m33.79s compute.
Messages:
19:36:15.239 WARNING: Failed to remove temporary directory for //tools/please_go/install/exec:exec: failed to remove plz-out/tmp/tools/please_go/install/exec/exec._build: exit status 1
Output: rm: cannot remove 'plz-out/tmp/tools/please_go/install/exec/exec._build/pkg/mod/cache/download/golang.org/x/telemetry/config/@v': Directory not empty
19:36:19.065 WARNING: Failed to remove test directory for //tools/please_go/install:install_test: failed to remove plz-out/tmp/tools/please_go/install/install_test._test/run_1: exit status 1
Output: rm: cannot remove 'plz-out/tmp/tools/please_go/install/install_test._test/run_1/go/pkg/mod/golang.org/x/telemetry/config@v0.29.0/doc.go': Permission denied
rm: cannot remove 'plz-out/tmp/tools/please_go/install/install_test._test/run_1/go/pkg/mod/golang.org/x/telemetry/config@v0.29.0/LICENSE': Permission denied
rm: cannot remove 'plz-out/tmp/tools/please_go/install/install_test._test/run_1/go/pkg/mod/golang.org/x/telemetry/config@v0.29.0/config.json':Permission denied
rm: cannot remove 'plz-out/tmp/tools/please_go/install/install_test._test/run_1/go/pkg/mod/golang.org/x/telemetry/config@v0.29.0/go.mod': Permission denied

The root cause of this is that, by default, any invocation of go causes x/telemetry/internal/configstore to pull the latest telemetry configuration from x/telemetry/config via go mod download without using the -modcacherw flag:

https://github.com/golang/telemetry/blob/0693e6240b9b888df93a2e280a64431c10d47a63/internal/configstore/download.go#L43

Because this writes the configuration beneath GOROOT with 0444 permissions (and with 0555 intermediate directories), Please is prevented from unlinking the entire temporary directory tree.

I'm aware that telemetry can be disabled by writing off to $HOME/.config/go/telemetry before running go, which prevents this module from being pulled in the first place. However, the value of HOME is also set to the path to the temporary directory so this would have to be done before every invocation of go inside the build sandbox - while we could guarantee that this happens for the build definitions in the go-rules plugin, it's impossible to guarantee in the general case, where users write custom build rules that run whatever commands they want.

What did you see happen?

Please builds the target, but isn't able to clean up its own temporary directory afterwards owing to the Go toolchain writing a read-only file tree beneath GOROOT.

What did you expect to see?

Please builds the target and is able to clean up its own temporary directory afterwards without having to resort to hacks such as running go clean -modcache or chmodding GOROOT to make every file within it writeable after every invocation of go.

gabyhelp commented 3 weeks ago

Related Issues and Documentation

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

chrisnovakovic commented 3 weeks ago

67463 seems relevant here, although some of the comments imply that this only happens when telemetry is uploaded, which I don't believe is the case - here, the default telemetry mode (`local`) is in use, and the config file is still being pulled.

seankhliao commented 3 weeks ago

perhaps please can set GOFLAGS=-modcacherw to have it apply to all go commands instead of passing it individually to each go invocation?

and/or clean up with go clean -modcache?

cherrymui commented 3 weeks ago

cc @golang/telemetry @golang/tools-team

findleyr commented 3 weeks ago

although some of the comments imply that this only happens when telemetry is uploaded,

I think we may download the config too eagerly during the check for work to upload. In any case, if this is breaking users it doesn't really matter if it is infrequent.

I think we had discussed having the upload use a temp (or even dedicated) GOMODCACHE and clean up after itself. However, in #67463 we were only thinking about cmd/dist, not general usage where -modcacherw is desirable.

We should try to fix this for 1.23.1, particularly if the fix is low risk. I will investigate.

hyangah commented 3 weeks ago

I think we need to fix x/telemetry/internal/upload to avoid downloading the config unless the telemetry mode is on. (we still want to run other uploader logic such as compacting the counter files in local mode and producing json files even when the telemetry mode is local).

I still wish we could avoid polluting the module cache even when telemetry is on. Previously in https://github.com/golang/go/issues/67463#issuecomment-2119623176 the idea of using a temporary module cache was rejected because we thought checksum db was still stored under GOPATH. But I don't think that's true (except the sum.golang.org/latest data).

$ GOMODCACHE=/tmp/gomodcache GOPATH=/tmp/gopath go mod download golang.org/x/telemetry/config@latest
$ tree /tmp/gomodcache
/tmp/gomodcache
├── cache
│   └── download
│       ├── golang.org
│       │   └── x
│       │       └── telemetry
│       │           └── config
│       │               └── @v
│       │                   ├── list
│       │                   ├── v0.29.0.info
│       │                   ├── v0.29.0.lock
│       │                   ├── v0.29.0.mod
│       │                   ├── v0.29.0.zip
│       │                   └── v0.29.0.ziphash
│       └── sumdb
│           └── sum.golang.org
│               ├── lookup
│               │   └── golang.org
│               │       └── x
│               │           └── telemetry
│               │               └── config@v0.29.0
│               └── tile
│                   └── 8
│                       ├── 0
│                       │   └── x113
│                       │       ├── 563
│                       │       └── 749.p
│                       │           └── 112
│                       ├── 1
│                       │   ├── 443
│                       │   └── 444.p
│                       │       └── 85
│                       ├── 2
│                       │   └── 001.p
│                       │       └── 188
│                       └── 3
│                           └── 000.p
│                               └── 1
└── golang.org
    └── x
        └── telemetry
            └── config@v0.29.0
                ├── LICENSE
                ├── config.json
                ├── doc.go
                └── go.mod

28 directories, 17 files
$ tree /tmp/gopath
/tmp/gopath
└── pkg
    └── sumdb
        └── sum.golang.org
            └── latest

3 directories, 1 file

peterebden commented 3 weeks ago

perhaps please can set GOFLAGS=-modcacherw to have it apply to all go commands instead of passing it individually to each go invocation?

We could indeed do that, although it's still suboptimal if it'll attempt to download the module for each invocation.

and/or clean up with go clean -modcache?

We would ideally not want to have to add that to the end of every command; it also wouldn't get run if something else fails first so we'd still get the same issue (and for a language-agnostic build system it isn't clear on which actions it should/shouldn't run this if we try to special-case it).

The ideal from my perspective would be some way of turning all telemetry off, preferably via an env var which is easy to set, which also prevents it from downloading any telemetry libraries.

chrisnovakovic commented 3 weeks ago

The ideal from my perspective would be some way of turning all telemetry off, preferably via an env var which is easy to set, which also prevents it from downloading any telemetry libraries.

+1. The idea of a GOTELEMETRY environment variable whose value would only be honoured if it were set to off (i.e. telemetry could be opted out of, but not into, using this mechanism) was floated in https://github.com/golang/go/issues/65503#issuecomment-1925446871. We'd certainly make use of that in Please, although I think it's also worth addressing the over-eager downloading of the config by x/telemetry/internal/upload as a separate issue.

findleyr commented 3 weeks ago

@chrisnovakovic I filed #68960 last night for making GOTELEMETRY=off settable.

thediveo commented 3 weeks ago

isn't telemetry off by default and an opt-in? if not, this would violate European Union law and the corresponding EC member state laws.

ianlancetaylor commented 3 weeks ago

@thediveo Yes, telemetry is off by default.

findleyr commented 3 weeks ago

@thediveo the fix for this particular issue will be to not download the telemetry configuration if telemetry uploading is off (the default).

dmitshur commented 3 weeks ago

Moved to Go1.24 milestone since this need to be fixed on the main branch first (for Go 1.24), before being considered for backporting. Please use the usual process (https://go.dev/wiki/MinorReleases) to create a separate backport tracking issue in the Go1.23.1 milestone.

findleyr commented 3 weeks ago

Thanks @dmitshur.

@gopherbot please backport this issue to 1.23: it is a misbehavior of the telemetry integration with 1.23 that breaks existing CI workflows.

gopherbot commented 3 weeks ago

Backport issue(s) opened: #68994 (for 1.23).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

fwcd commented 3 weeks ago

Ran into this while building yay from the Arch User Repository: https://github.com/archsink/x86_64/actions/runs/10484074392/job/29037778152

gopherbot commented 3 weeks ago

Change https://go.dev/cl/607796 mentions this issue: internal/upload: only download the upload config if necessary