From 6c1220894dd198a6b32b854ea1383601f9681a54 Mon Sep 17 00:00:00 2001 From: Suhas Karanth Date: Tue, 3 Oct 2017 10:02:46 +0530 Subject: [PATCH 1/4] allow cachedir override using env var source - main.go: Read and use env var `DEPCACHEDIR` for instantiating dep context. - context.go: - Add field `Cachedir` to struct `Ctx`. This holds the value of env var `DEPCACHEDIR`. - Use `Ctx.Cachedir` while instantiating `gps.SourceMgr` if present, fallback to `$GOPATH/pkg/dep` otherwise. - source_manager.go: Add a getter func `Cachedir` to facilitate testing in `context_test.go`. test - context_test.go Add test to check `gps.SourceMgr` is instantiated with appropriate `cachedir`. - integration_test.go: Add test to check environment variable `DEPCACHEDIR` is loaded and used if present. misc - update changelog --- CHANGELOG.md | 2 ++ cmd/dep/integration_test.go | 40 ++++++++++++++++++++++++++++ cmd/dep/main.go | 5 ++++ cmd/dep/testdata/cachedir/Gopkg.lock | 15 +++++++++++ cmd/dep/testdata/cachedir/Gopkg.toml | 4 +++ cmd/dep/testdata/cachedir/main.go | 12 +++++++++ context.go | 9 ++++++- context_test.go | 39 +++++++++++++++++++++++++++ gps/source_manager.go | 5 ++++ 9 files changed, 130 insertions(+), 1 deletion(-) create mode 100644 cmd/dep/testdata/cachedir/Gopkg.lock create mode 100644 cmd/dep/testdata/cachedir/Gopkg.toml create mode 100644 cmd/dep/testdata/cachedir/main.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 10f54c4569..6194f2b6fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ NEW FEATURES: * Add support for importing from [glock](https://github.com/robfig/glock) based projects (#1422). * Add support for importing from [govendor](https://github.com/kardianos/govendor) based projects (#815). +* Allow override of cache directory location using environment variable +`DEPCACHEDIR`. ([#1234](https://github.com/golang/dep/pull/1234)) BUG FIXES: diff --git a/cmd/dep/integration_test.go b/cmd/dep/integration_test.go index cc9f959cc9..672f1aa071 100644 --- a/cmd/dep/integration_test.go +++ b/cmd/dep/integration_test.go @@ -52,6 +52,46 @@ func TestIntegration(t *testing.T) { }) } +func TestDepCachedir(t *testing.T) { + t.Parallel() + + test.NeedsExternalNetwork(t) + test.NeedsGit(t) + + wd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + + initPath := filepath.Join("testdata", "cachedir") + + testProj := integration.NewTestProject(t, initPath, wd, runMain) + defer testProj.Cleanup() + + testProj.TempDir("cachedir") + cachedir := testProj.Path("cachedir") + testProj.Setenv("DEPCACHEDIR", cachedir) + + // Running `dep ensure` will pull in the dependency into cachedir. + err = testProj.DoRun([]string{"ensure"}) + if err != nil { + // Log the error output from running `dep ensure`, could be useful. + t.Log(testProj.GetStderr()) + t.Fatalf("got an unexpected error: %s", err.Error()) + } + + // Check that the cache was created in the cachedir. Our fixture has the dependency + // `github.com/sdboyer/deptest` + _, err = os.Stat(testProj.Path("cachedir", "sources", "https---github.com-sdboyer-deptest")) + if err != nil { + if os.IsNotExist(err) { + t.Fatal("Expected cachedir to have been populated but none was found") + } else { + t.Fatalf("Got unexpected error: %s", err) + } + } +} + // execCmd is a test.RunFunc which runs the program in another process. func execCmd(prog string, args []string, stdout, stderr io.Writer, dir string, env []string) error { cmd := exec.Command(prog, args...) diff --git a/cmd/dep/main.go b/cmd/dep/main.go index aa29e0a589..11b0c5e7b9 100644 --- a/cmd/dep/main.go +++ b/cmd/dep/main.go @@ -180,12 +180,17 @@ func (c *Config) Run() int { return errorExitCode } + // Cachedir is loaded from env if present. `$GOPATH/pkg/dep` is used as the + // fallback cache location. + cachedir := getEnv(c.Env, "DEPCACHEDIR") + // Set up dep context. ctx := &dep.Ctx{ Out: outLogger, Err: errLogger, Verbose: *verbose, DisableLocking: getEnv(c.Env, "DEPNOLOCK") != "", + Cachedir: cachedir, } GOPATHS := filepath.SplitList(getEnv(c.Env, "GOPATH")) diff --git a/cmd/dep/testdata/cachedir/Gopkg.lock b/cmd/dep/testdata/cachedir/Gopkg.lock new file mode 100644 index 0000000000..c7f497e7a1 --- /dev/null +++ b/cmd/dep/testdata/cachedir/Gopkg.lock @@ -0,0 +1,15 @@ +# This file is autogenerated, do not edit; changes may be undone by the next 'dep ensure'. + + +[[projects]] + name = "github.com/sdboyer/deptest" + packages = ["."] + revision = "ff2948a2ac8f538c4ecd55962e919d1e13e74baf" + version = "v1.0.0" + +[solve-meta] + analyzer-name = "dep" + analyzer-version = 1 + inputs-digest = "14b07b05e0f01051b03887ab2bf80b516bc5510ea92f75f76c894b1745d8850c" + solver-name = "gps-cdcl" + solver-version = 1 diff --git a/cmd/dep/testdata/cachedir/Gopkg.toml b/cmd/dep/testdata/cachedir/Gopkg.toml new file mode 100644 index 0000000000..e242e02114 --- /dev/null +++ b/cmd/dep/testdata/cachedir/Gopkg.toml @@ -0,0 +1,4 @@ + +[[constraint]] + name = "github.com/sdboyer/deptest" + version = "1.0.0" diff --git a/cmd/dep/testdata/cachedir/main.go b/cmd/dep/testdata/cachedir/main.go new file mode 100644 index 0000000000..1fe0d19d6a --- /dev/null +++ b/cmd/dep/testdata/cachedir/main.go @@ -0,0 +1,12 @@ +// Copyright 2016 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +import ( + _ "github.com/sdboyer/deptest" +) + +func main() { +} diff --git a/context.go b/context.go index bf2502ddb2..8aad0338ba 100644 --- a/context.go +++ b/context.go @@ -40,6 +40,7 @@ type Ctx struct { Out, Err *log.Logger // Required loggers. Verbose bool // Enables more verbose logging. DisableLocking bool // When set, no lock file will be created to protect against simultaneous dep processes. + Cachedir string // Cache directory loaded from environment. } // SetPaths sets the WorkingDir and GOPATHs fields. If GOPATHs is empty, then @@ -87,8 +88,14 @@ func defaultGOPATH() string { // SourceManager produces an instance of gps's built-in SourceManager // initialized to log to the receiver's logger. 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`. + cachedir = filepath.Join(c.GOPATH, "pkg", "dep") + } + return gps.NewSourceManager(gps.SourceManagerConfig{ - Cachedir: filepath.Join(c.GOPATH, "pkg", "dep"), + Cachedir: cachedir, Logger: c.Out, DisableLocking: c.DisableLocking, }) diff --git a/context_test.go b/context_test.go index 5ee5ba15fb..e5864990b3 100644 --- a/context_test.go +++ b/context_test.go @@ -488,3 +488,42 @@ func TestDetectGOPATH(t *testing.T) { } } } + +func TestDepCachedir(t *testing.T) { + h := test.NewHelper(t) + defer h.Cleanup() + + h.TempDir("cache") + // Create the directory for fallback cachedir location. + h.TempDir(filepath.Join("go", "pkg", "dep")) + + testCachedir := h.Path("cache") + gopath := h.Path("go") + discardLgr := discardLogger() + + cases := []struct { + cachedir string + wantCachedir string + }{ + // If `Cachedir` is not set in the context, it should use `$GOPATH/pkg/dep`. + {cachedir: "", wantCachedir: h.Path(filepath.Join("go", "pkg", "dep"))}, + // If `Cachedir` is set in the context, it should use that. + {cachedir: testCachedir, wantCachedir: testCachedir}, + } + + for _, c := range cases { + ctx := &Ctx{ + GOPATH: gopath, + Cachedir: c.cachedir, + Out: discardLgr, + Err: discardLgr, + } + sm, err := ctx.SourceManager() + h.Must(err) + defer sm.Release() + + if sm.Cachedir() != c.wantCachedir { + t.Errorf("expected cachedir to be %s, got %s", c.wantCachedir, sm.Cachedir()) + } + } +} diff --git a/gps/source_manager.go b/gps/source_manager.go index bb12d492ad..de5d8a6b96 100644 --- a/gps/source_manager.go +++ b/gps/source_manager.go @@ -291,6 +291,11 @@ func NewSourceManager(c SourceManagerConfig) (*SourceMgr, error) { return sm, nil } +// Cachedir returns the location of the cache directory. +func (sm *SourceMgr) Cachedir() string { + return sm.cachedir +} + // UseDefaultSignalHandling sets up typical os.Interrupt signal handling for a // SourceMgr. func (sm *SourceMgr) UseDefaultSignalHandling() { From c8f88484f39d1646ce37675bd3728927c591bf46 Mon Sep 17 00:00:00 2001 From: Suhas Karanth Date: Mon, 16 Oct 2017 11:28:12 +0530 Subject: [PATCH 2/4] validate env var DEPCACHEDIR is a valid path if set - fs.go - Add method `IsValidPath` to check if given file path string is valid. Add tests as well. - main.go - After loading cachedir from env, if it has been set, check validity, exit with status 1 if not. Update integration tests for this scenario. --- cmd/dep/integration_test.go | 68 +++++++++++++++++++++++++------------ cmd/dep/main.go | 23 ++++++++----- internal/fs/fs.go | 18 ++++++++++ internal/fs/fs_test.go | 41 ++++++++++++++++++++++ 4 files changed, 121 insertions(+), 29 deletions(-) diff --git a/cmd/dep/integration_test.go b/cmd/dep/integration_test.go index 672f1aa071..8f1acb9f96 100644 --- a/cmd/dep/integration_test.go +++ b/cmd/dep/integration_test.go @@ -65,31 +65,57 @@ func TestDepCachedir(t *testing.T) { initPath := filepath.Join("testdata", "cachedir") - testProj := integration.NewTestProject(t, initPath, wd, runMain) - defer testProj.Cleanup() + t.Run("env-cachedir", func(t *testing.T) { + t.Parallel() + testProj := integration.NewTestProject(t, initPath, wd, runMain) + defer testProj.Cleanup() - testProj.TempDir("cachedir") - cachedir := testProj.Path("cachedir") - testProj.Setenv("DEPCACHEDIR", cachedir) + testProj.TempDir("cachedir") + cachedir := testProj.Path("cachedir") + testProj.Setenv("DEPCACHEDIR", cachedir) - // Running `dep ensure` will pull in the dependency into cachedir. - err = testProj.DoRun([]string{"ensure"}) - if err != nil { - // Log the error output from running `dep ensure`, could be useful. - t.Log(testProj.GetStderr()) - t.Fatalf("got an unexpected error: %s", err.Error()) - } + // Running `dep ensure` will pull in the dependency into cachedir. + err = testProj.DoRun([]string{"ensure"}) + if err != nil { + // Log the error output from running `dep ensure`, could be useful. + t.Logf("`dep ensure` error output: \n%s", testProj.GetStderr()) + t.Errorf("got an unexpected error: %s", err) + } - // Check that the cache was created in the cachedir. Our fixture has the dependency - // `github.com/sdboyer/deptest` - _, err = os.Stat(testProj.Path("cachedir", "sources", "https---github.com-sdboyer-deptest")) - if err != nil { - if os.IsNotExist(err) { - t.Fatal("Expected cachedir to have been populated but none was found") - } else { - t.Fatalf("Got unexpected error: %s", err) + // Check that the cache was created in the cachedir. Our fixture has the dependency + // `github.com/sdboyer/deptest` + _, err = os.Stat(testProj.Path("cachedir", "sources", "https---github.com-sdboyer-deptest")) + if err != nil { + if os.IsNotExist(err) { + t.Error("expected cachedir to have been populated but none was found") + } else { + t.Errorf("got an unexpected error: %s", err) + } } - } + }) + t.Run("env-invalid-cachedir", func(t *testing.T) { + t.Parallel() + 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"}) + + 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) + } + }) + } // execCmd is a test.RunFunc which runs the program in another process. diff --git a/cmd/dep/main.go b/cmd/dep/main.go index 11b0c5e7b9..ec86155900 100644 --- a/cmd/dep/main.go +++ b/cmd/dep/main.go @@ -18,6 +18,7 @@ import ( "text/tabwriter" "github.com/golang/dep" + "github.com/golang/dep/internal/fs" ) var ( @@ -158,31 +159,37 @@ func (c *Config) Run() int { for _, cmd := range commands { if cmd.Name() == cmdName { // Build flag set with global flags in there. - fs := flag.NewFlagSet(cmdName, flag.ContinueOnError) - fs.SetOutput(c.Stderr) - verbose := fs.Bool("v", false, "enable verbose logging") + flags := flag.NewFlagSet(cmdName, flag.ContinueOnError) + flags.SetOutput(c.Stderr) + verbose := flags.Bool("v", false, "enable verbose logging") // Register the subcommand flags in there, too. - cmd.Register(fs) + cmd.Register(flags) // Override the usage text to something nicer. - resetUsage(errLogger, fs, cmdName, cmd.Args(), cmd.LongHelp()) + resetUsage(errLogger, flags, cmdName, cmd.Args(), cmd.LongHelp()) if printCommandHelp { - fs.Usage() + flags.Usage() return errorExitCode } // Parse the flags the user gave us. // flag package automatically prints usage and error message in err != nil // or if '-h' flag provided - if err := fs.Parse(c.Args[2:]); err != nil { + if err := flags.Parse(c.Args[2:]); err != nil { return errorExitCode } // Cachedir is loaded from env if present. `$GOPATH/pkg/dep` is used as the // fallback 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 + } // Set up dep context. ctx := &dep.Ctx{ @@ -197,7 +204,7 @@ func (c *Config) Run() int { ctx.SetPaths(c.WorkingDir, GOPATHS...) // Run the command with the post-flag-processing args. - if err := cmd.Run(ctx, fs.Args()); err != nil { + if err := cmd.Run(ctx, flags.Args()); err != nil { errLogger.Printf("%v\n", err) return errorExitCode } diff --git a/internal/fs/fs.go b/internal/fs/fs.go index 1ed5d31eb5..dec9221e9b 100644 --- a/internal/fs/fs.go +++ b/internal/fs/fs.go @@ -480,6 +480,24 @@ 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 + } + + // Attempt to create it + var d []byte + if err := ioutil.WriteFile(fp, d, 0644); err == nil { + os.Remove(fp) // And delete it + return true + } + + return false +} + // IsDir determines is the path given is a directory or not. func IsDir(name string) (bool, error) { fi, err := os.Stat(name) diff --git a/internal/fs/fs_test.go b/internal/fs/fs_test.go index 4b9422dd88..63a86cb7cd 100644 --- a/internal/fs/fs_test.go +++ b/internal/fs/fs_test.go @@ -837,6 +837,47 @@ 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) + } + + var dn string + + cleanup := setupInaccessibleDir(t, func(dir string) error { + dn = filepath.Join(dir, "dir") + return os.Mkdir(dn, 0777) + }) + 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, + } + + if runtime.GOOS == "windows" { + // This test doesn't work on Microsoft Windows because + // of the differences in how file permissions are + // implemented. For this to work, the directory where + // the directory exists should be inaccessible. + 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) + } + } +} + func TestIsRegular(t *testing.T) { wd, err := os.Getwd() if err != nil { From 37d6c560cdf407be7b6cd035b23dba89df9275cf Mon Sep 17 00:00:00 2001 From: Suhas Karanth Date: Sat, 2 Dec 2017 11:30:06 +0530 Subject: [PATCH 3/4] 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 e5864990b3..d673cc845a 100644 --- a/context_test.go +++ b/context_test.go @@ -494,7 +494,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) } } } From e88d062abc7573462bd3cca99102244226e7a016 Mon Sep 17 00:00:00 2001 From: Suhas Karanth Date: Fri, 8 Dec 2017 09:01:27 +0530 Subject: [PATCH 4/4] skip test for cachedir on Windows --- cmd/dep/integration_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/cmd/dep/integration_test.go b/cmd/dep/integration_test.go index 0ecebaff54..b332b0cf50 100644 --- a/cmd/dep/integration_test.go +++ b/cmd/dep/integration_test.go @@ -11,6 +11,7 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "strings" "testing" @@ -54,6 +55,13 @@ func TestIntegration(t *testing.T) { } func TestDepCachedir(t *testing.T) { + if runtime.GOOS == "windows" { + // This test is unreliable on Windows and fails at random which makes it very + // difficult to debug. It might have something to do with parallel execution. + // Since the test doesn't test any specific behavior of Windows, it should be okay + // to skip. + t.Skip("skipping on windows") + } t.Parallel() test.NeedsExternalNetwork(t)