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: go clean should not accept flags like -modcache with packages #53725

Closed
mvdan opened this issue Jul 6, 2022 · 13 comments
Closed

cmd/go: go clean should not accept flags like -modcache with packages #53725

mvdan opened this issue Jul 6, 2022 · 13 comments

Comments

@mvdan
Copy link
Member

mvdan commented Jul 6, 2022

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

$ go version
go version devel go1.19-818ffd4247 Tue Jul 5 15:19:44 2022 +0100 linux/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/mvdan/.cache/go-build"
GOENV="/home/mvdan/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/mvdan/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/mvdan/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/mvdan/tip"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/mvdan/tip/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="devel go1.19-818ffd4247 Tue Jul 5 15:19:44 2022 +0100"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/mvdan/src/garble/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1584385538=/tmp/go-build -gno-record-gcc-switches"
GOROOT/bin/go version: go version devel go1.19-818ffd4247 Tue Jul 5 15:19:44 2022 +0100 linux/amd64
GOROOT/bin/go tool compile -V: compile version devel go1.19-818ffd4247 Tue Jul 5 15:19:44 2022 +0100
uname -sr: Linux 5.18.9-arch1-1
/usr/lib/libc.so.6: GNU C Library (GNU libc) stable release version 2.35.
gdb --version: GNU gdb (GDB) 12.1

What did you do?

I wanted to simulate fetching a module from proxy.golang.org, and since it is already in my GOMODCACHE, I wanted to delete it from there. So I naively ran:

go clean -modcache some/module/path

What did you expect to see?

I expected only a few files/directories to be deleted from GOMODCACHE.

What did you see instead?

It deleted my entire GOMODCACHE; presumably, the command is equivalent to:

go clean -modcache
go clean some/module/path

Given that flags like -cache, -modcache, -testcache, and -fuzzcache clear the entire cache directories in question, they should reject package arguments, because otherwise they are very easy to be misused or misunderstood.

It is possible that this might break a few users, but I generally doubt it will be many. I haven't seen any uses of go clean pkg in the wild in years. If anyone wants the current behavior of go clean -modcache pkg, they can always do go clean -modcache; go clean pkg as shown above, which is also more explicit.

@mvdan mvdan added the GoCommand cmd/go label Jul 6, 2022
@hyangah
Copy link
Contributor

hyangah commented Jul 6, 2022

I agree that it's better to report an error since this is confusing and the cost of the mistake can be large,
even though this behavior is documented.

Related #32976, which is marked as duplicate of #28835 even though the motivation of the proposal and the proposed command line structure is different.

@mvdan
Copy link
Member Author

mvdan commented Jul 7, 2022

Thanks @seankhliao; the same search also works on github: https://cs.github.com/?scopeName=All+repos&scope=&q=%2Fgo+clean%28+-%5Ba-z%5D%2B%29*%28+-%28cache%7Cmodcache%7Ctestcache%7Cfuzzcache%29%29%2B%28+-%5Ba-z%5D%2B%29*+%5Ba-z%5C.%5D%2B%2F

I'm willing to bet that the majority of those lines are buggy. The way they are written, they look like the intent is to e.g. remove the cached test results for a few packages.

Related #32976, which is marked as duplicate of #28835 even though the motivation of the proposal and the proposed command line structure is different.

Good call out, I did not notice that proposal - cc @bcmills. I think this change could be a stepping stone towards that proposal; any people who currently use go clean -flag pkg and who want #28835's behavior will then come show their support for the proposal. Any people who just wanted go clean -flag, as per the current behavior, can simply adjust their commands.

@heschi heschi added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jul 7, 2022
@heschi heschi added this to the Backlog milestone Jul 7, 2022
@mvdan
Copy link
Member Author

mvdan commented Aug 3, 2022

@bcmills suggests that I should turn this into a proposal, because it's a CLI change that could potentially break some users. See my reasoning above as to why I think the breakage will be minimal, though.

@mvdan mvdan changed the title cmd/go: go clean should not accept flags like -modcache with packages proposal: cmd/go: go clean should not accept flags like -modcache with packages Aug 3, 2022
@mvdan mvdan removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 3, 2022
@mvdan mvdan modified the milestones: Backlog, Proposal Aug 3, 2022
@rsc
Copy link
Contributor

rsc commented Aug 3, 2022

Reporting an error sounds fine to me.

@rsc
Copy link
Contributor

rsc commented Aug 3, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this to Active in Proposals Aug 10, 2022
@rsc rsc added this to Proposals Aug 10, 2022
@rsc rsc moved this from Active to Likely Accept in Proposals Aug 12, 2022
@rsc
Copy link
Contributor

rsc commented Aug 12, 2022

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Aug 17, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc moved this from Likely Accept to Accepted in Proposals Aug 17, 2022
@rsc rsc changed the title proposal: cmd/go: go clean should not accept flags like -modcache with packages cmd/go: go clean should not accept flags like -modcache with packages Aug 17, 2022
@rsc rsc modified the milestones: Proposal, Backlog Aug 17, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/425874 mentions this issue: cmd/go: go clean should not accept flags like -modcache with packages

gopherbot pushed a commit that referenced this issue Aug 30, 2022
For #53725

Change-Id: I99a85b437d5f918dba74c4eccefcf8087193646a
Reviewed-on: https://go-review.googlesource.com/c/go/+/425874
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
evanw added a commit to evanw/esbuild that referenced this issue Feb 3, 2023
After upgrading to Go 1.20, I now see this error:

    go: clean -testcache cannot be used with package arguments

More info: golang/go#53725
@penggy
Copy link

penggy commented Jun 25, 2023

I don't think that's a good suggestion.
I just want to clean my cgo package, go clean -cache my/cgo/package It's what I want.
It worked well in the past.
When I upgraded to 1.20, It say "go: clean -cache cannot be used with package arguments".
Now I have to run go clean -cache to clean all packages, and then rebuild cost more time.
This commit cause the trouble.

@mvdan
Copy link
Member Author

mvdan commented Jun 25, 2023

@penggy please read the original post carefully - go clean -cache some/pkg used to clean the entire build cache, not just that one package's build cache entries. It never did what you thought it did, which is why I raised this issue.

@penggy
Copy link

penggy commented Jun 25, 2023

@mvdan 😄
Thank you for reply.
I have misunderstanding on go clean -cache.

AlexanderYastrebov added a commit to zalando/skipper that referenced this issue Jul 15, 2023
* remove unnecessary bin target
* remove bin directory during clean
* fix typo
* do not clean entire go build cache (see golang/go#53725)

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
AlexanderYastrebov added a commit to zalando/skipper that referenced this issue Jul 15, 2023
* remove unnecessary bin target
* remove bin directory during clean
* fix typo
* do not clean entire go build cache (see golang/go#53725)

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
AlexanderYastrebov added a commit to zalando/skipper that referenced this issue Jul 17, 2023
* remove unnecessary bin target
* remove bin directory during clean
* fix typo
* do not clean entire go build cache (see golang/go#53725)

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
@bcmills
Copy link
Contributor

bcmills commented Oct 12, 2023

https://go.dev/cl/425874 was merged in 2022. I think this proposal was implemented in Go 1.20?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants