Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/go: flag modcacherw does not take effect in the target package #64282

Closed
marjus45 opened this issue Nov 20, 2023 · 17 comments
Closed

cmd/go: flag modcacherw does not take effect in the target package #64282

marjus45 opened this issue Nov 20, 2023 · 17 comments
Assignees
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. GoCommand cmd/go modules NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@marjus45
Copy link

What version of Go are you using (go version)?

$ go version
go version go1.21.4 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

I tested this on a clean Debian container.

go env Output
$ go env
GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/root/.cache/go-build'
GOENV='/root/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/root/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/root/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21.4'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='0'
GOMOD='/dev/null'
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 -fno-caret-diagnostics -Qunused-arguments -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build392749006=/tmp/go-build -gno-record-gcc-switches'

What did you do?

  1. Install a package using the modcacherw flag with go install:
    $ GOPATH=$(pwd)/_go go install -modcacherw github.com/google/go-containerregistry/cmd/crane@v0.16.1
    go: downloading github.com/google/go-containerregistry v0.16.1
    go: downloading github.com/docker/cli v24.0.0+incompatible
    go: downloading github.com/opencontainers/image-spec v1.1.0-rc3
    go: downloading github.com/spf13/cobra v1.7.0
    go: downloading github.com/opencontainers/go-digest v1.0.0
    go: downloading golang.org/x/sync v0.2.0
    go: downloading github.com/docker/distribution v2.8.2+incompatible
    go: downloading github.com/containerd/stargz-snapshotter/estargz v0.14.3
    go: downloading github.com/google/go-cmp v0.5.9
    go: downloading github.com/mitchellh/go-homedir v1.1.0
    go: downloading github.com/klauspost/compress v1.16.5
    go: downloading github.com/spf13/pflag v1.0.5
    go: downloading github.com/vbatts/tar-split v0.11.3
    go: downloading github.com/pkg/errors v0.9.1
    go: downloading golang.org/x/sys v0.8.0
    go: downloading github.com/docker/docker-credential-helpers v0.7.0
    go: downloading github.com/docker/docker v24.0.0+incompatible
    go: downloading github.com/sirupsen/logrus v1.9.1
    
  2. List the directories:
    $ ls _go/pkg/mod/github.com/google/ -la
    total 16
    drwxr-xr-x  4 root root 4096 Nov 20 13:49 .
    drwxr-xr-x 12 root root 4096 Nov 20 13:49 ..
    drwxr-xr-x  4 root root 4096 Nov 20 13:49 go-cmp@v0.5.9
    dr-xr-xr-x 10 root root 4096 Nov 20 13:49 go-containerregistry@v0.16.1
    
  3. Notice that go-containerregistry@v0.16.1 has read only permissions while their dependencies (go-cmp) has read-write permissions, as expected.

What did you expect to see?

I expected to see the directory go-containerregistry@v0.16.1 to have read-write permissions, since I have set the flag modcacherw.

	-modcacherw
		leave newly-created directories in the module cache read-write
		instead of making them read-only.

What did you see instead?

Instead go-containerregistry@v0.16.1 has read only permissions.

I thinks this is a regression, since up to v1.20 I confirmed that it worked as expected.

@mknyszek mknyszek added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. GoCommand cmd/go labels Nov 20, 2023
@mknyszek mknyszek added this to the Backlog milestone Nov 20, 2023
@mknyszek
Copy link
Contributor

@seankhliao seankhliao changed the title cmd/go: Flag modcacherw does not take effect in the target package cmd/go: flag modcacherw does not take effect in the target package Nov 21, 2023
@bcmills
Copy link
Contributor

bcmills commented Dec 1, 2023

@marjus45, I'm not able to reproduce the behavior you report:

$ go1.21.4 install -modcacherw github.com/google/go-containerregistry/cmd/crane@v0.16.1
go: downloading github.com/google/go-containerregistry v0.16.1
go: downloading github.com/opencontainers/image-spec v1.1.0-rc3
go: downloading github.com/docker/cli v24.0.0+incompatible
go: downloading golang.org/x/sync v0.2.0
go: downloading github.com/spf13/cobra v1.7.0
go: downloading github.com/docker/distribution v2.8.2+incompatible
go: downloading github.com/containerd/stargz-snapshotter/estargz v0.14.3
go: downloading github.com/google/go-cmp v0.5.9
go: downloading github.com/opencontainers/go-digest v1.0.0
go: downloading github.com/mitchellh/go-homedir v1.1.0
go: downloading github.com/klauspost/compress v1.16.5
go: downloading github.com/vbatts/tar-split v0.11.3
go: downloading github.com/spf13/pflag v1.0.5
go: downloading github.com/pkg/errors v0.9.1
go: downloading github.com/docker/docker v24.0.0+incompatible
go: downloading github.com/sirupsen/logrus v1.9.1
go: downloading golang.org/x/sys v0.8.0
go: downloading github.com/docker/docker-credential-helpers v0.7.0

$ ls -la $(go1.21.4 env GOMODCACHE)/github.com/google
total 16
drwxr-x---  4 bcmills primarygroup 4096 Dec  1 09:27 .
drwxr-x--- 12 bcmills primarygroup 4096 Dec  1 09:27 ..
drwxr-x---  4 bcmills primarygroup 4096 Dec  1 09:27 go-cmp@v0.5.9
drwxr-x--- 10 bcmills primarygroup 4096 Dec  1 09:27 go-containerregistry@v0.16.1

Did you run any other commands before or after the go install command?

@bcmills
Copy link
Contributor

bcmills commented Dec 1, 2023

Huh. Apparently it has something to do with running as root:

$ sudo $(which go1.21.4) install -modcacherw github.com/google/go-containerregistry/cmd/crane@v0.16.1
go: downloading github.com/google/go-containerregistry v0.16.1
go: downloading github.com/docker/cli v24.0.0+incompatible
go: downloading github.com/opencontainers/image-spec v1.1.0-rc3
go: downloading github.com/spf13/cobra v1.7.0
go: downloading golang.org/x/sync v0.2.0
go: downloading github.com/opencontainers/go-digest v1.0.0
go: downloading github.com/mitchellh/go-homedir v1.1.0
go: downloading github.com/google/go-cmp v0.5.9
go: downloading github.com/docker/distribution v2.8.2+incompatible
go: downloading github.com/containerd/stargz-snapshotter/estargz v0.14.3
go: downloading github.com/klauspost/compress v1.16.5
go: downloading github.com/vbatts/tar-split v0.11.3
go: downloading github.com/spf13/pflag v1.0.5
go: downloading github.com/pkg/errors v0.9.1
go: downloading github.com/sirupsen/logrus v1.9.1
go: downloading github.com/docker/docker v24.0.0+incompatible
go: downloading golang.org/x/sys v0.8.0
go: downloading github.com/docker/docker-credential-helpers v0.7.0

$ sudo ls -la $(sudo $(which go1.21.4) env GOMODCACHE)/github.com/google
total 16
drwxr-xr-x  4 root root 4096 Dec  1 09:33 .
drwxr-xr-x 12 root root 4096 Dec  1 09:33 ..
drwxr-xr-x  4 root root 4096 Dec  1 09:33 go-cmp@v0.5.9
dr-xr-xr-x 10 root root 4096 Dec  1 09:33 go-containerregistry@v0.16.1

@bcmills
Copy link
Contributor

bcmills commented Dec 1, 2023

Bisecting now.

@bcmills
Copy link
Contributor

bcmills commented Dec 1, 2023

aa99c4d2925e3628460482db8657765880e6836c is the first bad commit
commit aa99c4d2925e3628460482db8657765880e6836c
Author: Russ Cox <rsc@golang.org>
Date:   Wed May 24 07:10:17 2023 -0400

    cmd/go: handle queries properly in go install m@v

    The original code only handled go install m@v1.0.0
    but not queries like go install m@v1 or m@master.
    Handle those by invoking more of the module machinery.

    For #57001.

    Change-Id: I7d54fc3dd65072e5906a17ff95b407b0f7feac69
    Reviewed-on: https://go-review.googlesource.com/c/go/+/497879
    Run-TryBot: Russ Cox <rsc@golang.org>
    Reviewed-by: Bryan Mills <bcmills@google.com>
    Auto-Submit: Russ Cox <rsc@golang.org>
    TryBot-Result: Gopher Robot <gobot@golang.org>

 src/cmd/go/gotoolchain.go                         | 62 +++++++----------------
 src/cmd/go/internal/modload/init.go               | 14 +++++
 src/cmd/go/testdata/mod/rsc.io_fortune_v0.0.1.txt |  4 ++
 src/cmd/go/testdata/script/gotoolchain.txt        | 13 +++++
 4 files changed, 50 insertions(+), 43 deletions(-)

@bcmills bcmills modified the milestones: Backlog, Go1.22 Dec 1, 2023
@bcmills bcmills added NeedsFix The path to resolution is known, but the work has not been done. modules labels Dec 1, 2023
@bcmills bcmills self-assigned this Dec 1, 2023
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 1, 2023
@bcmills
Copy link
Contributor

bcmills commented Dec 1, 2023

@gopherbot, please backport to Go 1.21. This was a regression in Go 1.21 and has no clear workaround.

@gopherbot
Copy link
Contributor

Backport issue(s) opened: #64497 (for 1.21).

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

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/546635 mentions this issue: cmd/go/internal/toolchain: make a best effort to parse 'go run' and 'go install' flags

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/550055 mentions this issue: [release-branch.go1.21] cmd/go/internal/toolchain: make a best effort to parse 'go run' and 'go install' flags

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/550237 mentions this issue: cmd/go/internal/toolchain: simplify flag parsing and stop at the first non-flag

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/551215 mentions this issue: cmd/go/internal/toolchain: revert "make a best effort to parse 'go run' and 'go install' flags"

gopherbot pushed a commit that referenced this issue Dec 19, 2023
…n' and 'go install' flags"

This caused other problems, and for the purposes of the Go 1.22
release, we can just roll back to the Go 1.21 behavior and then
decide in January what the correct path forward is.

Revert of CL 546635.

Unfixes #64282.
Fixes #64738.
For #57001.

This reverts commit e44b8b1.

Change-Id: I78753c76dcd0bc6dbc90caa17f73248c42e5f64a
Reviewed-on: https://go-review.googlesource.com/c/go/+/551215
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Than McIntosh <thanm@google.com>
@rsc
Copy link
Contributor

rsc commented Dec 19, 2023

Rolled back due to causing #64738.

@rsc rsc reopened this Dec 19, 2023
@rsc rsc added the okay-after-rc1 Used by release team to mark a release-blocker issue as okay to resolve either before or after rc1 label Dec 19, 2023
@dmitshur dmitshur moved this to Todo in Release Blockers Dec 19, 2023
@gopherbot gopherbot removed the okay-after-rc1 Used by release team to mark a release-blocker issue as okay to resolve either before or after rc1 label Dec 19, 2023
@dmitshur dmitshur moved this from Todo to In Progress in Release Blockers Jan 10, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/555436 mentions this issue: cmd/go/internal/toolchain: apply the -modcacherw flag when downloading a module to determine what toolchain it needs

@bcmills bcmills added the FixPending Issues that have a fix which has not yet been reviewed or submitted. label Jan 11, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in Release Blockers Jan 29, 2024
@bcmills bcmills reopened this Jan 29, 2024
@bcmills
Copy link
Contributor

bcmills commented Jan 29, 2024

This is merged to the main branch, but due to the timing needs to be backported to the release branch.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/559218 mentions this issue: [release-branch.go1.22] cmd/go/internal/toolchain: apply the -modcacherw flag when downloading a module to determine what toolchain it needs

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/559198 mentions this issue: [release-branch.go1.21] cmd/go/internal/toolchain: apply the -modcacherw flag when downloading a module to determine what toolchain it needs

gopherbot pushed a commit that referenced this issue Jan 31, 2024
…erw flag when downloading a module to determine what toolchain it needs

Fixes #64282.

Change-Id: I3f211c599ee70cb58254d0bc07eeb3c135124e58
Reviewed-on: https://go-review.googlesource.com/c/go/+/555436
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
(cherry picked from commit cc38c68)
Reviewed-on: https://go-review.googlesource.com/c/go/+/559218
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@eliben
Copy link
Member

eliben commented Jan 31, 2024

https://go.dev/cl/559218 was merged, so I'm removing the release-blocker label

@bcmills bcmills closed this as completed Jan 31, 2024
gopherbot pushed a commit that referenced this issue Jan 31, 2024
…erw flag when downloading a module to determine what toolchain it needs

Fixes #64497.
Updates #64282.

Change-Id: I3f211c599ee70cb58254d0bc07eeb3c135124e58
Reviewed-on: https://go-review.googlesource.com/c/go/+/555436
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
(cherry picked from commit cc38c68)
Reviewed-on: https://go-review.googlesource.com/c/go/+/559198
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
…go install' flags

When the argument to 'go install' or 'go run' looks like a versioned
package, we make a best effort to switch to a toolchain compatible
with the module containing that package, by fetching its go.mod file
and checking the go version it specifies.

At this point in the code, we have not yet parsed the arguments given
on the command line: instead, we just make a best effort to find one
we can use to select a toolchain version. Since that toolchain may be
newer, the command to install it may also include flags that are only
supported by that Go version — and we don't want to fail due to an
error that would be resolved by switching to a more appropriate
toolchain.

So at this point in the code we can't parse the flags in a way that
will surface errors, but we want to make a best effort to parse the
ones that we know about. It turns out that “parse the flags we know
about” is already a familiar problem: that's also what we do in
'go test', so we can reuse the cmdflag library from that to do the
best-effort pass of parsing.

If it turns out that we don't need to switch toolchains after all,
cmd/go's main function will parse the flags again, and will report any
errors at that point.

This fixes a regression, introduced in CL 497879, which caused
'go install -modcacherw pkg@version' to unset the write bit for
directories created while selecting the toolchain to use.

Fixes golang#64282.
Updates golang#57001.

Change-Id: Icc409c57858aa15c7d58a97a61964b4bc2560547
Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-longtest,gotip-windows-amd64-longtest
Reviewed-on: https://go-review.googlesource.com/c/go/+/546635
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
…n' and 'go install' flags"

This caused other problems, and for the purposes of the Go 1.22
release, we can just roll back to the Go 1.21 behavior and then
decide in January what the correct path forward is.

Revert of CL 546635.

Unfixes golang#64282.
Fixes golang#64738.
For golang#57001.

This reverts commit e44b8b1.

Change-Id: I78753c76dcd0bc6dbc90caa17f73248c42e5f64a
Reviewed-on: https://go-review.googlesource.com/c/go/+/551215
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Than McIntosh <thanm@google.com>
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
…g a module to determine what toolchain it needs

Fixes golang#64282.

Change-Id: I3f211c599ee70cb58254d0bc07eeb3c135124e58
Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-longtest,gotip-windows-amd64-longtest
Reviewed-on: https://go-review.googlesource.com/c/go/+/555436
Auto-Submit: Bryan Mills <bcmills@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Russ Cox <rsc@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. GoCommand cmd/go modules NeedsFix The path to resolution is known, but the work has not been done.
Projects
Status: Done
Development

No branches or pull requests

7 participants