From bff0adc9a69eef0d1e8e859dd335dfa219513771 Mon Sep 17 00:00:00 2001 From: Suhas Karanth Date: Sat, 2 Dec 2017 11:30:06 +0530 Subject: [PATCH] do not create cachedir recursively source - main.go: Try to ensure directory for given `cachedir` path. - context.go: Create the default cache directory, `$GOPATH/pkg/dep`, if the user did not override it. - source_manager.go: Use `fs.EnsureDir` instead of `os.MkdirAll` for creating sources folder in cache directory. - fs.go: - Add func `EnsureDir` to create a directory if it does not exist. - Remove func `IsValidPath`. test - integration_test.go: Improve tests for invalid cache directory. - fs_test.go: Add test for `EnsureDir`, remove test for `IsValidPath`. - manager_test.go: fix TestSourceManagerInit - Re-create cache directory before trying to call `NewSourceManager` the 2nd time and defer it's removal. - If `NewSourceManager` fails the 2nd time, exit the error using `t.Fatal` to avoid panic in `sm.Release` misc - language - {fallback => default} for cachedir --- cmd/dep/integration_test.go | 44 ++++++++++++++++++++++++----------- cmd/dep/main.go | 15 +++++++----- context.go | 6 ++++- context_test.go | 2 +- gps/manager_test.go | 16 +++++++++---- gps/source_manager.go | 3 ++- internal/fs/fs.go | 25 ++++++++++---------- internal/fs/fs_test.go | 46 ++++++++++++++++++++++--------------- 8 files changed, 98 insertions(+), 59 deletions(-) diff --git a/cmd/dep/integration_test.go b/cmd/dep/integration_test.go index 8f1acb9f96..0ecebaff54 100644 --- a/cmd/dep/integration_test.go +++ b/cmd/dep/integration_test.go @@ -7,6 +7,7 @@ package main import ( "fmt" "io" + "io/ioutil" "os" "os/exec" "path/filepath" @@ -98,21 +99,36 @@ func TestDepCachedir(t *testing.T) { testProj := integration.NewTestProject(t, initPath, wd, runMain) defer testProj.Cleanup() - cachedir := "/invalid/path" - testProj.Setenv("DEPCACHEDIR", cachedir) - wantErr := fmt.Sprintf( - "dep: $DEPCACHEDIR set to an invalid or inaccessible path: %q", cachedir, - ) - - // Running `dep ensure` will pull in the dependency into cachedir. - err = testProj.DoRun([]string{"ensure"}) + var d []byte + tmpFp := testProj.Path("tmp-file") + ioutil.WriteFile(tmpFp, d, 0644) + cases := []string{ + // invalid path + "\000", + // parent directory does not exist + testProj.Path("non-existent-fldr", "cachedir"), + // path is a regular file + tmpFp, + // invalid path, tmp-file is a regular file + testProj.Path("tmp-file", "cachedir"), + } - if err == nil { - // Log the output from running `dep ensure`, could be useful. - t.Logf("test run output: \n%s\n%s", testProj.GetStdout(), testProj.GetStderr()) - t.Error("unexpected result: \n\t(GOT) nil\n\t(WNT) exit status 1") - } else if gotErr := strings.TrimSpace(testProj.GetStderr()); gotErr != wantErr { - t.Errorf("unexpected error output: \n\t(GOT) %s\n\t(WNT) %s", gotErr, wantErr) + wantErr := "dep: $DEPCACHEDIR set to an invalid or inaccessible path" + for _, c := range cases { + testProj.Setenv("DEPCACHEDIR", c) + + err = testProj.DoRun([]string{"ensure"}) + + if err == nil { + // Log the output from running `dep ensure`, could be useful. + t.Logf("test run output: \n%s\n%s", testProj.GetStdout(), testProj.GetStderr()) + t.Error("unexpected result: \n\t(GOT) nil\n\t(WNT) exit status 1") + } else if stderr := testProj.GetStderr(); !strings.Contains(stderr, wantErr) { + t.Errorf( + "unexpected error output: \n\t(GOT) %s\n\t(WNT) %s", + strings.TrimSpace(stderr), wantErr, + ) + } } }) diff --git a/cmd/dep/main.go b/cmd/dep/main.go index ec86155900..760a378034 100644 --- a/cmd/dep/main.go +++ b/cmd/dep/main.go @@ -182,13 +182,16 @@ func (c *Config) Run() int { } // Cachedir is loaded from env if present. `$GOPATH/pkg/dep` is used as the - // fallback cache location. + // default cache location. cachedir := getEnv(c.Env, "DEPCACHEDIR") - if cachedir != "" && !fs.IsValidPath(cachedir) { - errLogger.Printf( - "dep: $DEPCACHEDIR set to an invalid or inaccessible path: %q\n", cachedir, - ) - return errorExitCode + if cachedir != "" { + if err := fs.EnsureDir(cachedir, 0777); err != nil { + errLogger.Printf( + "dep: $DEPCACHEDIR set to an invalid or inaccessible path: %q\n", cachedir, + ) + errLogger.Printf("dep: failed to ensure cache directory: %v\n", err) + return errorExitCode + } } // Set up dep context. diff --git a/context.go b/context.go index 8aad0338ba..475efb3776 100644 --- a/context.go +++ b/context.go @@ -90,8 +90,12 @@ func defaultGOPATH() string { func (c *Ctx) SourceManager() (*gps.SourceMgr, error) { cachedir := c.Cachedir if cachedir == "" { - // When `DEPCACHEDIR` isn't set in the env, fallback to `$GOPATH/pkg/dep`. + // When `DEPCACHEDIR` isn't set in the env, use the default - `$GOPATH/pkg/dep`. cachedir = filepath.Join(c.GOPATH, "pkg", "dep") + // Create the default cachedir if it does not exist. + if err := os.MkdirAll(cachedir, 0777); err != nil { + return nil, errors.Wrap(err, "failed to create default cache directory") + } } return gps.NewSourceManager(gps.SourceManagerConfig{ diff --git a/context_test.go b/context_test.go index 4628d59691..7843b63fce 100644 --- a/context_test.go +++ b/context_test.go @@ -491,7 +491,7 @@ func TestDepCachedir(t *testing.T) { defer h.Cleanup() h.TempDir("cache") - // Create the directory for fallback cachedir location. + // Create the directory for default cachedir location. h.TempDir(filepath.Join("go", "pkg", "dep")) testCachedir := h.Path("cache") diff --git a/gps/manager_test.go b/gps/manager_test.go index 7023b2a530..4551e60816 100644 --- a/gps/manager_test.go +++ b/gps/manager_test.go @@ -121,17 +121,23 @@ func TestSourceManagerInit(t *testing.T) { t.Fatalf("Global cache lock file not cleared correctly on Release()") } + err = os.MkdirAll(cpath, 0777) + if err != nil { + t.Errorf("Failed to re-create temp dir: %s", err) + } + defer func() { + err = os.RemoveAll(cpath) + if err != nil { + t.Errorf("removeAll failed: %s", err) + } + }() // Set another one up at the same spot now, just to be sure sm, err = NewSourceManager(cfg) if err != nil { - t.Errorf("Creating a second SourceManager should have succeeded when the first was released, but failed with err %s", err) + t.Fatalf("Creating a second SourceManager should have succeeded when the first was released, but failed with err %s", err) } sm.Release() - err = os.RemoveAll(cpath) - if err != nil { - t.Errorf("removeAll failed: %s", err) - } } func TestSourceInit(t *testing.T) { diff --git a/gps/source_manager.go b/gps/source_manager.go index de5d8a6b96..489084d7ef 100644 --- a/gps/source_manager.go +++ b/gps/source_manager.go @@ -20,6 +20,7 @@ import ( "time" "github.com/golang/dep/gps/pkgtree" + "github.com/golang/dep/internal/fs" "github.com/nightlyone/lockfile" "github.com/pkg/errors" "github.com/sdboyer/constext" @@ -197,7 +198,7 @@ func NewSourceManager(c SourceManagerConfig) (*SourceMgr, error) { c.Logger = log.New(ioutil.Discard, "", 0) } - err := os.MkdirAll(filepath.Join(c.Cachedir, "sources"), 0777) + err := fs.EnsureDir(filepath.Join(c.Cachedir, "sources"), 0777) if err != nil { return nil, err } diff --git a/internal/fs/fs.go b/internal/fs/fs.go index dec9221e9b..4be512aad8 100644 --- a/internal/fs/fs.go +++ b/internal/fs/fs.go @@ -480,22 +480,21 @@ func cloneSymlink(sl, dst string) error { return os.Symlink(resolved, dst) } -// IsValidPath checks if the given string is a valid path. -func IsValidPath(fp string) bool { - // See https://stackoverflow.com/questions/35231846/golang-check-if-string-is-valid-path - // Check if file/dir already exists - if _, err := os.Stat(fp); err == nil { - return true - } +// EnsureDir tries to ensure that a directory is present at the given path. It first +// checks if the directory already exists at the given path. If there isn't one, it tries +// to create it with the given permissions. However, it does not try to create the +// directory recursively. +func EnsureDir(path string, perm os.FileMode) error { + _, err := IsDir(path) - // Attempt to create it - var d []byte - if err := ioutil.WriteFile(fp, d, 0644); err == nil { - os.Remove(fp) // And delete it - return true + if os.IsNotExist(err) { + err = os.Mkdir(path, perm) + if err != nil { + return errors.Wrapf(err, "failed to ensure directory at %q", path) + } } - return false + return err } // IsDir determines is the path given is a directory or not. diff --git a/internal/fs/fs_test.go b/internal/fs/fs_test.go index 63a86cb7cd..5ca8bf1649 100644 --- a/internal/fs/fs_test.go +++ b/internal/fs/fs_test.go @@ -837,14 +837,15 @@ func setupInaccessibleDir(t *testing.T, op func(dir string) error) func() { return cleanup } -func TestIsValidPath(t *testing.T) { - wd, err := os.Getwd() - if err != nil { - t.Fatal(err) - } +func TestEnsureDir(t *testing.T) { + h := test.NewHelper(t) + defer h.Cleanup() + h.TempDir(".") + h.TempFile("file", "") - var dn string + tmpPath := h.Path(".") + var dn string cleanup := setupInaccessibleDir(t, func(dir string) error { dn = filepath.Join(dir, "dir") return os.Mkdir(dn, 0777) @@ -852,13 +853,16 @@ func TestIsValidPath(t *testing.T) { defer cleanup() tests := map[string]bool{ - wd: true, - filepath.Join(wd, "testdata"): true, - filepath.Join(wd, "main.go"): true, - filepath.Join(wd, "this_file_does_not_exist.thing"): true, - dn: false, - "": false, - "/invalid/path": false, + // [success] A dir already exists for the given path. + tmpPath: true, + // [success] Dir does not exist but parent dir exists, so should get created. + filepath.Join(tmpPath, "testdir"): true, + // [failure] Dir and parent dir do not exist, should return an error. + filepath.Join(tmpPath, "notexist", "testdir"): false, + // [failure] Regular file present at given path. + h.Path("file"): false, + // [failure] Path inaccessible. + dn: false, } if runtime.GOOS == "windows" { @@ -869,11 +873,17 @@ func TestIsValidPath(t *testing.T) { delete(tests, dn) } - for fp, want := range tests { - got := IsValidPath(fp) - - if got != want { - t.Fatalf("expected %t for %s, got %t", want, fp, got) + for path, shouldEnsure := range tests { + err := EnsureDir(path, 0777) + if shouldEnsure { + if err != nil { + t.Fatalf("unexpected error %q for %q", err, path) + } else if ok, err := IsDir(path); !ok { + t.Fatalf("expected directory to be preset at %q", path) + t.Fatal(err) + } + } else if err == nil { + t.Fatalf("expected error for path %q, got none", path) } } }