From 8c249f921e31482054b0b45adc975474a0dc170d Mon Sep 17 00:00:00 2001 From: Max Neverov Date: Sat, 16 Nov 2024 14:36:46 +0100 Subject: [PATCH 1/8] md/go: fail go clean command when failed to find go cache directory Currently, if computing of the go cache directory fails it does not expose the error. Commands like go clean, exec, modindex that use go cache directory continue execution producing incorrect or no result. This patch adds an error to the return values such that it can be validated on call sites. It also introduces such validation in go clean -cache command to fail execution in case when error occurred. Fixes #69997 --- src/cmd/go/alldocs.go | 2 +- src/cmd/go/go_test.go | 2 +- src/cmd/go/internal/cache/default.go | 6 +++--- src/cmd/go/internal/clean/clean.go | 10 ++++++++-- src/cmd/go/internal/envcmd/env.go | 2 +- src/cmd/go/internal/help/helpdoc.go | 2 +- src/cmd/go/internal/modindex/read.go | 2 +- src/cmd/go/internal/test/test.go | 2 +- src/cmd/go/internal/work/shell.go | 2 +- 9 files changed, 18 insertions(+), 12 deletions(-) diff --git a/src/cmd/go/alldocs.go b/src/cmd/go/alldocs.go index 830bac2b2f8965..cdd53eb8de0b31 100644 --- a/src/cmd/go/alldocs.go +++ b/src/cmd/go/alldocs.go @@ -2331,7 +2331,7 @@ // GOBIN // The directory where 'go install' will install a command. // GOCACHE -// The directory where the go command will store cached +// The absolute path to the directory where the go command will store cached // information for reuse in future builds. // GOCACHEPROG // A command (with optional space-separated flags) that implements an diff --git a/src/cmd/go/go_test.go b/src/cmd/go/go_test.go index 1df7cf8faa8b01..4cf4294a84c135 100644 --- a/src/cmd/go/go_test.go +++ b/src/cmd/go/go_test.go @@ -197,7 +197,7 @@ func TestMain(m *testing.M) { defer removeAll(testTmpDir) } - testGOCACHE, _ = cache.DefaultDir() + testGOCACHE, _, _ = cache.DefaultDir() if testenv.HasGoBuild() { testBin = filepath.Join(testTmpDir, "testbin") if err := os.Mkdir(testBin, 0777); err != nil { diff --git a/src/cmd/go/internal/cache/default.go b/src/cmd/go/internal/cache/default.go index f8e5696cbd1e84..bc1e3e85b9113f 100644 --- a/src/cmd/go/internal/cache/default.go +++ b/src/cmd/go/internal/cache/default.go @@ -34,7 +34,7 @@ See golang.org to learn more about Go. // initDefaultCache does the work of finding the default cache // the first time Default is called. func initDefaultCache() Cache { - dir, _ := DefaultDir() + dir, _, _ := DefaultDir() if dir == "off" { if defaultDirErr != nil { base.Fatalf("build cache is required, but could not be located: %v", defaultDirErr) @@ -71,7 +71,7 @@ var ( // DefaultDir returns the effective GOCACHE setting. // It returns "off" if the cache is disabled, // and reports whether the effective value differs from GOCACHE. -func DefaultDir() (string, bool) { +func DefaultDir() (string, bool, error) { // Save the result of the first call to DefaultDir for later use in // initDefaultCache. cmd/go/main.go explicitly sets GOCACHE so that // subprocesses will inherit it, but that means initDefaultCache can't @@ -100,5 +100,5 @@ func DefaultDir() (string, bool) { defaultDir = filepath.Join(dir, "go-build") }) - return defaultDir, defaultDirChanged + return defaultDir, defaultDirChanged, defaultDirErr } diff --git a/src/cmd/go/internal/clean/clean.go b/src/cmd/go/internal/clean/clean.go index 37566025cebd8a..71c9a8e69235cb 100644 --- a/src/cmd/go/internal/clean/clean.go +++ b/src/cmd/go/internal/clean/clean.go @@ -153,7 +153,10 @@ func runClean(ctx context.Context, cmd *base.Command, args []string) { sh := work.NewShell("", &load.TextPrinter{Writer: os.Stdout}) if cleanCache { - dir, _ := cache.DefaultDir() + dir, _, err := cache.DefaultDir() + if err != nil { + base.Fatal(err) + } if dir != "off" { // Remove the cache subdirectories but not the top cache directory. // The top cache directory may have been created with special permissions @@ -180,7 +183,10 @@ func runClean(ctx context.Context, cmd *base.Command, args []string) { // Instead of walking through the entire cache looking for test results, // we write a file to the cache indicating that all test results from before // right now are to be ignored. - dir, _ := cache.DefaultDir() + dir, _, err := cache.DefaultDir() + if err != nil { + base.Fatal(err) + } if dir != "off" { f, err := lockedfile.Edit(filepath.Join(dir, "testexpire.txt")) if err == nil { diff --git a/src/cmd/go/internal/envcmd/env.go b/src/cmd/go/internal/envcmd/env.go index 7c370d427f9c2f..16c35a37371e46 100644 --- a/src/cmd/go/internal/envcmd/env.go +++ b/src/cmd/go/internal/envcmd/env.go @@ -131,7 +131,7 @@ func MkEnv() []cfg.EnvVar { env[i].Changed = true } case "GOCACHE": - env[i].Value, env[i].Changed = cache.DefaultDir() + env[i].Value, env[i].Changed, _ = cache.DefaultDir() case "GOTOOLCHAIN": env[i].Value, env[i].Changed = cfg.EnvOrAndChanged("GOTOOLCHAIN", "") case "GODEBUG": diff --git a/src/cmd/go/internal/help/helpdoc.go b/src/cmd/go/internal/help/helpdoc.go index 23459ef15413e3..07ed918d6e8201 100644 --- a/src/cmd/go/internal/help/helpdoc.go +++ b/src/cmd/go/internal/help/helpdoc.go @@ -504,7 +504,7 @@ General-purpose environment variables: GOBIN The directory where 'go install' will install a command. GOCACHE - The directory where the go command will store cached + The absolute path to the directory where the go command will store cached information for reuse in future builds. GOCACHEPROG A command (with optional space-separated flags) that implements an diff --git a/src/cmd/go/internal/modindex/read.go b/src/cmd/go/internal/modindex/read.go index 4c1fbd835961f6..d11bf128e9af5c 100644 --- a/src/cmd/go/internal/modindex/read.go +++ b/src/cmd/go/internal/modindex/read.go @@ -151,7 +151,7 @@ func GetPackage(modroot, pkgdir string) (*IndexPackage, error) { // using the index, for instance because the index is disabled, or the package // is not in a module. func GetModule(modroot string) (*Module, error) { - dir, _ := cache.DefaultDir() + dir, _, _ := cache.DefaultDir() if !enabled || dir == "off" { return nil, errDisabled } diff --git a/src/cmd/go/internal/test/test.go b/src/cmd/go/internal/test/test.go index 90f2d88d6b8a45..af2632a4a79a9b 100644 --- a/src/cmd/go/internal/test/test.go +++ b/src/cmd/go/internal/test/test.go @@ -839,7 +839,7 @@ func runTest(ctx context.Context, cmd *base.Command, args []string) { // Read testcache expiration time, if present. // (We implement go clean -testcache by writing an expiration date // instead of searching out and deleting test result cache entries.) - if dir, _ := cache.DefaultDir(); dir != "off" { + if dir, _, _ := cache.DefaultDir(); dir != "off" { if data, _ := lockedfile.Read(filepath.Join(dir, "testexpire.txt")); len(data) > 0 && data[len(data)-1] == '\n' { if t, err := strconv.ParseInt(string(data[:len(data)-1]), 10, 64); err == nil { testCacheExpire = time.Unix(0, t) diff --git a/src/cmd/go/internal/work/shell.go b/src/cmd/go/internal/work/shell.go index dd5a31c60668d8..2604b074da9f73 100644 --- a/src/cmd/go/internal/work/shell.go +++ b/src/cmd/go/internal/work/shell.go @@ -127,7 +127,7 @@ func (sh *Shell) moveOrCopyFile(dst, src string, perm fs.FileMode, force bool) e // Otherwise fall back to standard copy. // If the source is in the build cache, we need to copy it. - dir, _ := cache.DefaultDir() + dir, _, _ := cache.DefaultDir() if strings.HasPrefix(src, dir) { return sh.CopyFile(dst, src, perm, force) } From b410a69f8d6004c7aa015e7c5793a01d8f894f5a Mon Sep 17 00:00:00 2001 From: Max Neverov Date: Mon, 18 Nov 2024 07:50:48 +0100 Subject: [PATCH 2/8] Add test. --- src/cmd/go/internal/clean/clean_test.go | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 src/cmd/go/internal/clean/clean_test.go diff --git a/src/cmd/go/internal/clean/clean_test.go b/src/cmd/go/internal/clean/clean_test.go new file mode 100644 index 00000000000000..dc0af06b706f38 --- /dev/null +++ b/src/cmd/go/internal/clean/clean_test.go @@ -0,0 +1,24 @@ +package clean + +import ( + "os/exec" + "strings" + "testing" +) + +func TestCleanCache(t *testing.T) { + cmd := exec.Command("go", "clean", "-cache") + // See issue 69997. GOCACHE must be an absolute path. + cmd.Env = append(cmd.Environ(), "GOCACHE='.cache'") + _, err := cmd.Output() + + if err != nil { + ee, ok := err.(*exec.ExitError) + if !ok || ee.ExitCode() != 1 || !strings.Contains(string(ee.Stderr), "GOCACHE is not an absolute path") { + t.Errorf("\"go clean -cache\" failed. expected status 1 != %d; error: %s", ee.ExitCode(), ee.Stderr) + } + } else { + t.Errorf("expected go clean -cache to fail") + } + +} From d7959525b0910b807c3f24dc346a9af4f5c7a1e1 Mon Sep 17 00:00:00 2001 From: Max Neverov Date: Mon, 18 Nov 2024 18:56:43 +0100 Subject: [PATCH 3/8] Add test. --- src/cmd/go/internal/clean/clean_test.go | 24 -------------------- src/cmd/go/testdata/script/clean_cache_n.txt | 5 ++++ 2 files changed, 5 insertions(+), 24 deletions(-) delete mode 100644 src/cmd/go/internal/clean/clean_test.go diff --git a/src/cmd/go/internal/clean/clean_test.go b/src/cmd/go/internal/clean/clean_test.go deleted file mode 100644 index dc0af06b706f38..00000000000000 --- a/src/cmd/go/internal/clean/clean_test.go +++ /dev/null @@ -1,24 +0,0 @@ -package clean - -import ( - "os/exec" - "strings" - "testing" -) - -func TestCleanCache(t *testing.T) { - cmd := exec.Command("go", "clean", "-cache") - // See issue 69997. GOCACHE must be an absolute path. - cmd.Env = append(cmd.Environ(), "GOCACHE='.cache'") - _, err := cmd.Output() - - if err != nil { - ee, ok := err.(*exec.ExitError) - if !ok || ee.ExitCode() != 1 || !strings.Contains(string(ee.Stderr), "GOCACHE is not an absolute path") { - t.Errorf("\"go clean -cache\" failed. expected status 1 != %d; error: %s", ee.ExitCode(), ee.Stderr) - } - } else { - t.Errorf("expected go clean -cache to fail") - } - -} diff --git a/src/cmd/go/testdata/script/clean_cache_n.txt b/src/cmd/go/testdata/script/clean_cache_n.txt index 72f9abf9ae1c82..52e9029847eb93 100644 --- a/src/cmd/go/testdata/script/clean_cache_n.txt +++ b/src/cmd/go/testdata/script/clean_cache_n.txt @@ -18,6 +18,11 @@ go clean -cache ! go clean -cache . stderr 'go: clean -cache cannot be used with package arguments' +# GOCACHE must be an absolute path. +env GOCACHE=. +! go clean -cache +stderr 'go: GOCACHE is not an absolute path' + -- main.go -- package main From 8c856bbeb4a82ed31447eb946c6e96809890b4e8 Mon Sep 17 00:00:00 2001 From: Max Neverov Date: Thu, 21 Nov 2024 07:34:59 +0100 Subject: [PATCH 4/8] Add errcheck when initializing default cache. --- src/cmd/go/internal/cache/default.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/cmd/go/internal/cache/default.go b/src/cmd/go/internal/cache/default.go index bc1e3e85b9113f..f3f5414333d6e1 100644 --- a/src/cmd/go/internal/cache/default.go +++ b/src/cmd/go/internal/cache/default.go @@ -34,7 +34,10 @@ See golang.org to learn more about Go. // initDefaultCache does the work of finding the default cache // the first time Default is called. func initDefaultCache() Cache { - dir, _, _ := DefaultDir() + dir, _, err := DefaultDir() + if err != nil { + base.Fatalf("build cache is required, but could not be located: %v", err) + } if dir == "off" { if defaultDirErr != nil { base.Fatalf("build cache is required, but could not be located: %v", defaultDirErr) From 2e6e351743b3e241b9b4e3c10a9c7c29b9aceb01 Mon Sep 17 00:00:00 2001 From: Max Neverov Date: Thu, 21 Nov 2024 17:45:21 +0100 Subject: [PATCH 5/8] Remove obsolete errcheck. --- src/cmd/go/internal/cache/default.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/cmd/go/internal/cache/default.go b/src/cmd/go/internal/cache/default.go index f3f5414333d6e1..eec2be9fa0a5f6 100644 --- a/src/cmd/go/internal/cache/default.go +++ b/src/cmd/go/internal/cache/default.go @@ -39,9 +39,6 @@ func initDefaultCache() Cache { base.Fatalf("build cache is required, but could not be located: %v", err) } if dir == "off" { - if defaultDirErr != nil { - base.Fatalf("build cache is required, but could not be located: %v", defaultDirErr) - } base.Fatalf("build cache is disabled by GOCACHE=off, but required as of Go 1.12") } if err := os.MkdirAll(dir, 0o777); err != nil { From 8084eba8e20a1264f5ac8d87127b93ffdf46cec3 Mon Sep 17 00:00:00 2001 From: Max Neverov Date: Tue, 10 Dec 2024 08:02:32 +0100 Subject: [PATCH 6/8] Fix GOCACHE doc comment. --- src/cmd/go/alldocs.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cmd/go/alldocs.go b/src/cmd/go/alldocs.go index cdd53eb8de0b31..c6f91454e68211 100644 --- a/src/cmd/go/alldocs.go +++ b/src/cmd/go/alldocs.go @@ -2332,7 +2332,7 @@ // The directory where 'go install' will install a command. // GOCACHE // The absolute path to the directory where the go command will store cached -// information for reuse in future builds. +// information for reuse in future builds. Must be an absolute path. // GOCACHEPROG // A command (with optional space-separated flags) that implements an // external go command build cache. From 957fd30cd0f806eeaf4aaf153d0de5c3316935bc Mon Sep 17 00:00:00 2001 From: Max Neverov Date: Thu, 12 Dec 2024 07:12:06 +0100 Subject: [PATCH 7/8] Fix GOCACHE doc comment again. --- src/cmd/go/alldocs.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cmd/go/alldocs.go b/src/cmd/go/alldocs.go index c6f91454e68211..2c78e806a7f6d9 100644 --- a/src/cmd/go/alldocs.go +++ b/src/cmd/go/alldocs.go @@ -2331,7 +2331,7 @@ // GOBIN // The directory where 'go install' will install a command. // GOCACHE -// The absolute path to the directory where the go command will store cached +// The directory where the go command will store cached // information for reuse in future builds. Must be an absolute path. // GOCACHEPROG // A command (with optional space-separated flags) that implements an From e2063d10db7bb969bcbc8993761e3b38bb420938 Mon Sep 17 00:00:00 2001 From: Max Neverov Date: Tue, 18 Mar 2025 20:55:46 +0100 Subject: [PATCH 8/8] Update helpdoc. --- src/cmd/go/internal/help/helpdoc.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cmd/go/internal/help/helpdoc.go b/src/cmd/go/internal/help/helpdoc.go index 07ed918d6e8201..377329a0f34833 100644 --- a/src/cmd/go/internal/help/helpdoc.go +++ b/src/cmd/go/internal/help/helpdoc.go @@ -504,8 +504,8 @@ General-purpose environment variables: GOBIN The directory where 'go install' will install a command. GOCACHE - The absolute path to the directory where the go command will store cached - information for reuse in future builds. + The directory where the go command will store cached + information for reuse in future builds. Must be an absolute path. GOCACHEPROG A command (with optional space-separated flags) that implements an external go command build cache.