allow cachedir override using env var #1234

sudo-suhas · 2017-10-03T04:50:00Z

What does this do / why do we need it?

Allows to override the cache directory location using an environment variable(DEPCACHEDIR). Having the cache location depend on the gopath can negatively affect performance for anyone using multiple Go workspaces.

What should your reviewer look out for in this PR?

The source code change was simple enough but it was a bit challenging to write the tests. Please check if they look alright.

Do you need help or clarification on anything?

Tests.

Which issue(s) does this PR fix?

fixes #1066

Changelog

main - Read and use env var DEPCACHEDIR for instantiating dep context.
context
- 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 - Add a getter func Cachedir to facilitate testing in
context_test.go.
context_test - Add test to check gps.SourceMgr is instantiated with
appropriate cachedir.
integration_test - Add test to check environment variable DEPCACHEDIR is
loaded and used if present.

jmank88 · 2017-10-03T12:17:06Z

internal/gps/source_manager.go

@@ -292,6 +292,11 @@ func NewSourceManager(c SourceManagerConfig) (*SourceMgr, error) {
 	return sm, nil
 }

+// Cachedir is a getter for the cachedir location.


What about Cachedir returns the location of the cache directory.?

Sure @jmank88. I'll wait for some more feedback and do it along with the other requested changes(if any).

yep, i agree with @jmank88's suggestion.

sudo-suhas · 2017-10-03T12:28:41Z

I am on a windows machine and the newly added tests were passing. However, the appveyor build failed while trying to git clone https://github.com/sdboyer/deptest. Does this happen often? Is there anything I would need to do to fix this?

sdboyer · 2017-10-07T00:50:07Z

appveyor tests can be pretty flaky, and network failures like that aren't uncommon. i kicked the test to rerun, but it revealed a new incompatibility that's popped up as a result of a push from a couple days ago, so you'll need to rebase and accommodate that change anyway.

sudo-suhas · 2017-10-07T05:36:58Z

@sdboyer I have rebased. However, I am not able to replicate the error on appveyor:

λ go test -run ^TestDepCachedir$ -v
=== RUN   TestDepCachedir
--- PASS: TestDepCachedir (0.40s)
PASS
ok      github.com/golang/dep   0.603s

sdboyer · 2017-10-12T16:48:30Z

ok, i'm gonna choose to think that whatever we saw was transient.

please update the CHANGELOG.md in the root of the repo - just a single line, consistent with the level of information recorded in other items there. then we can merge!

sudo-suhas · 2017-10-13T10:29:48Z

@sdboyer Do you think we should validate the value of env var DEPCACHEDIR? I could use the method described here - https://stackoverflow.com/questions/35231846/golang-check-if-string-is-valid-path

sdboyer · 2017-10-15T20:31:41Z

hmmm - fair point, that is definitely user input. yes, we should validate. the empirical approach there is fine - it'll also be a requirement to make sure that the directory exists in order for gps to work correctly with it, I think.

sdboyer

moving back to "request changes" until we've got the validation done.

sudo-suhas · 2017-10-16T03:06:32Z

@sdboyer Wouldn't creating the directory, if it does not exist, make sense? I think it is a valid scenario where the user gives the dir as $HOME/.cache/dep but the directory does not exist.

Edit: This should take care of creating the dir -

dep/internal/gps/source_manager.go

Lines 196 to 204 in 1d73a4a

    
           func NewSourceManager(c SourceManagerConfig) (*SourceMgr, error) { 
        
           	if c.Logger == nil { 
        
           		c.Logger = log.New(ioutil.Discard, "", 0) 
        
           	} 
        
           	err := os.MkdirAll(filepath.Join(c.Cachedir, "sources"), 0777) 
        
           	if err != nil { 
        
           		return nil, err 
        
           	}

darkowlzz · 2017-10-20T13:35:55Z

CHANGELOG.md

@@ -7,6 +7,8 @@ NEW FEATURES:
 (#1206)
 * `dep ensure -no-vendor -dry-run` now exits with an error when changes would
 have to be made to `Gopkg.lock`. This is useful for CI. (#1256)
+* Allow override of cache directory location using environment variable
+`DEPCACHEDIR`. (#1234)


We just released v0.3.2, so please move this under v0.3.3. We have started using release milestones to avoid this in the future.
Thanks!

Done with rebase.

sudo-suhas · 2017-10-21T02:27:05Z

@darkowlzz Small note, in the release notes for v0.3.2, the WIP section is also included, which, IMHO, shouldn't be present in the release notes.

Edit: I might have misunderstood what WIP means here. I see that the PRs have been merged but the targeted feature itself is a work-in-progress.

sudo-suhas · 2017-11-04T03:12:01Z

Rebased and resolved conflicts. Can we please merge this in? I can do the changes if any are required.

sudo-suhas · 2017-11-11T06:43:44Z

@sdboyer Could we move forward with this? The failure in CI is unrelated to changes in this PR as far as I can tell.

sdboyer · 2017-11-11T08:13:01Z

I'm still not back from vacation yet.

…

On November 11, 2017 7:43:45 AM GMT+01:00, Suhas Karanth ***@***.***> wrote: @sdboyer Could we move forward with this? The failure in CI is unrelated to changes in this PR as far as I can tell. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #1234 (comment)

sudo-suhas · 2017-11-11T08:14:29Z

My apologies. Hope you enjoy your vacation 😄 .

sdboyer · 2017-11-14T03:32:33Z

ok, i'm back. thanks for your patience.

so, the only question i have at this point is re: the use of os.MkdirAll() instead of os.Mkdir() for creating the directory. my feeling - which is to say, not backed by clear research - is that most tools avoid recursively creating directories for situations like this. problem is, i can't put my finger on exactly what criteria constitute "like this," though i think it's probably related to the use of an env var or param for setting the topmost-level directory in which a tool then dumps a bunch of additional files.

i think i'd prefer we be more conservative initially with this, and use os.Mkdir(). we can always switch it to os.MkdirAll() later.

sudo-suhas · 2017-11-14T03:53:39Z

@sdboyer To be clear, I did not modify source_manager.go to add the MkdirAll. It was already there. If you are sure you want to change this to Mkdir, I can.

sdboyer

That looks like overkill. It's pretty typical for CLI tools to simply report that they could not create a directory - they don't go and walk back up the tree to explain that it was because the parent dir is missing. I don't see a compelling need to do more than that.

The ordering relationship you're concerned about is what i was trying to address in my comment from yesterday. To make it clearer, I've made some quick inline comments to indicate where i think the calls should be made in order to avoid directory creation ordering problems.

sdboyer · 2017-11-15T05:38:09Z

cmd/dep/main.go

+			}
+
+			// Cachedir is loaded from env if present. `$GOPATH/pkg/dep` is used as the
+			// fallback cache location.


"default" is better than "fallback" here.

sdboyer · 2017-11-15T05:39:57Z

context.go

+		// When `DEPCACHEDIR` isn't set in the env, fallback to `$GOPATH/pkg/dep`.
+		cachedir = filepath.Join(c.GOPATH, "pkg", "dep")
+	}
+


This is probably where where first the os.Mkdir() call should be made to attempt to ensure that the cachedir exists prior to setting up the SourceManager, so that the calls within NewSourceManager() won't have a problem.

sudo-suhas · 2017-11-18T03:01:54Z

I am going to list all the scenarios just so that we(mostly me 😅) are clear.

$DEPCACHEDIR is not set. Same behavior as earlier, no confusion here.
$DEPCACHEDIR is set:
1. It is a valid path.
  1. The parent directory exists. Everything works OK.
  2. The parent directory does not exist. It errors out in cmd/dep/main.go when we call fs.IsValidPath. Since you are suggesting that the cache directory be created in with Mkdir in ctx.SourceManager(context.go), which will never even be called if the parent directory does not exist, I need a clarification. Should I change the behavior of fs.IsValidPath so that it does not return false for this particular scenario? And if yes, could you please help me figure out how?
2. It is not a valid path, maybe something like \000. Or the user does not have rw permissions to the given path. We error out in cmd/dep/main.go.

sdboyer · 2017-12-01T03:13:24Z

Since you are suggesting that the cache directory be created in with Mkdir in ctx.SourceManager(context.go)

i'm not suggesting that the cachedir be created there. I'm suggesting that filepath.Join(cachedir, "sources") be created there, much as it is now. it's the work of dep/cmd/main.go to create cachedir itself.

does that clarify sufficiently?

sudo-suhas · 2017-12-02T06:02:46Z

Thank you @sdboyer for being so patient with me. I have made the changes as per your suggestions.

sdboyer

i think i'm happy with the code here, now, with the exception of that appveyor error.

sudo-suhas · 2017-12-04T03:07:07Z

The test doesn't produce consistent results. It failed for me once on my system.

I also tried to add some logging and it works correctly:

diff --git a/cmd/dep/integration_test.go b/cmd/dep/integration_test.go
index 0ecebaff..26e192de 100644
--- a/cmd/dep/integration_test.go
+++ b/cmd/dep/integration_test.go
@@ -82,6 +82,23 @@ func TestDepCachedir(t *testing.T) {
                        t.Logf("`dep ensure` error output: \n%s", testProj.GetStderr())
                        t.Errorf("got an unexpected error: %s", err)
                }
+               visit := func(path string, info os.FileInfo, err error) error {
+                       path = strings.Replace(path, cachedir, "[cachedir]", -1)
+                       if info.IsDir() {
+                               if info.Name() == ".git" {
+                                       return filepath.SkipDir
+                               }
+                               fmt.Println("dir:  ", path)
+                       } else {
+                               fmt.Println("file: ", path)
+                       }
+                       return nil
+               }
+
+               err := filepath.Walk(cachedir, visit)
+               if err != nil {
+                       t.Fatal(err)
+               }

                // Check that the cache was created in the cachedir. Our fixture has the dependency
                // `github.com/sdboyer/deptest`

Test output:

go test -v -timeout 120s github.com/golang/dep/cmd/dep -run ^TestDepCachedir$

=== RUN   TestDepCachedir
=== RUN   TestDepCachedir/env-cachedir
=== RUN   TestDepCachedir/env-invalid-cachedir
dir:   [cachedir]
dir:   [cachedir]\sources
dir:   [cachedir]\sources\https---github.com-sdboyer-deptest
file:  [cachedir]\sources\https---github.com-sdboyer-deptest\README.md
file:  [cachedir]\sources\https---github.com-sdboyer-deptest\deptest.go
--- PASS: TestDepCachedir (0.01s)
    --- PASS: TestDepCachedir/env-invalid-cachedir (0.01s)
    --- PASS: TestDepCachedir/env-cachedir (4.05s)
PASS
ok  	github.com/golang/dep/cmd/dep	5.851s
Success: Tests passed.

Not sure how I can fix it. Does it have anything to do with the parallel execution?

sdboyer · 2017-12-06T15:42:35Z

i'm not sure what the problem is there, honestly, and i still don't have access to a windows box to be able to test it myself.

if it's working on your windows system, then honestly, i'd be OK with bypassing the test on windows, so long as an explanatory note is included in a comment.

darkowlzz · 2017-12-06T19:52:25Z

I think this is fine. I've tried reproducing this failure 10 times in appveyor (with an intent to debug the issue), but couldn't reproduce the failure even once. And in the past, we have seen some weird failures on Windows due to parallel execution. This could be like one of those.

sudo-suhas · 2017-12-08T02:55:57Z

I agree with @darkowlzz but I am okay with disabling the test on Windows. @sdboyer, what do you suggest?

sdboyer · 2017-12-08T03:24:43Z

yep, let's disable the test on windows, with a comment.

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

- 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.

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

sdboyer · 2017-12-08T15:51:41Z

ok, in we go! 🎉 🎉 thanks for your patience.

sudo-suhas · 2017-12-08T16:08:01Z

Thank you for your guidance!

sudo-suhas · 2017-12-09T06:06:47Z

@sdboyer While I not the sharpest tool in the shed and need some hand-holding, I still enjoy contributing here immensely. If you think there is anything else I could work on, please do ping. I'd be delighted to.

sudo-suhas requested a review from sdboyer as a code owner October 3, 2017 04:50

googlebot added the cla: yes label Oct 3, 2017

sudo-suhas mentioned this pull request Oct 3, 2017

Non-deterministic cache location #1066

Closed

jmank88 approved these changes Oct 3, 2017

View reviewed changes

sudo-suhas force-pushed the env-dep-cachedir branch from bf6fcea to e3930c6 Compare October 7, 2017 05:29

sdboyer approved these changes Oct 12, 2017

View reviewed changes

sudo-suhas force-pushed the env-dep-cachedir branch 2 times, most recently from 8fce7de to 740f9bb Compare October 13, 2017 04:38

sdboyer suggested changes Oct 15, 2017

View reviewed changes

sudo-suhas force-pushed the env-dep-cachedir branch from 69c070f to e52321c Compare October 17, 2017 06:57

darkowlzz reviewed Oct 20, 2017

View reviewed changes

darkowlzz added this to the v0.3.3 milestone Oct 20, 2017

sudo-suhas force-pushed the env-dep-cachedir branch from e52321c to eec7c00 Compare October 21, 2017 02:07

sudo-suhas force-pushed the env-dep-cachedir branch from eec7c00 to a46f562 Compare October 26, 2017 03:46

sudo-suhas force-pushed the env-dep-cachedir branch from a46f562 to e96719a Compare November 4, 2017 03:03

sudo-suhas force-pushed the env-dep-cachedir branch from e96719a to ab78af5 Compare November 14, 2017 03:51

sdboyer suggested changes Nov 15, 2017

View reviewed changes

sudo-suhas force-pushed the env-dep-cachedir branch from ab78af5 to 4698075 Compare December 2, 2017 06:00

sudo-suhas requested review from carolynvs and ibrasho as code owners December 2, 2017 06:27

sudo-suhas force-pushed the env-dep-cachedir branch 2 times, most recently from a4cbceb to 398de52 Compare December 2, 2017 07:06

carolynvs removed their request for review December 2, 2017 13:56

sudo-suhas force-pushed the env-dep-cachedir branch from c4c6c51 to bff0adc Compare December 3, 2017 15:22

sdboyer reviewed Dec 3, 2017

View reviewed changes

sdboyer mentioned this pull request Dec 6, 2017

Dep ensure results in exit status 128 for private repo #1430

Closed

sudo-suhas added 4 commits December 8, 2017 09:02

skip test for cachedir on Windows

e88d062

sudo-suhas force-pushed the env-dep-cachedir branch from bff0adc to e88d062 Compare December 8, 2017 03:34

sdboyer approved these changes Dec 8, 2017

View reviewed changes

sdboyer merged commit b745572 into golang:master Dec 8, 2017

sudo-suhas deleted the env-dep-cachedir branch December 9, 2017 06:05

radu-matei mentioned this pull request Oct 31, 2018

"dep ensure -add" fails on WSL (Bash on Windows on Linux) with "failed to strip vendor" error #1091

Closed

allow cachedir override using env var #1234

allow cachedir override using env var #1234

Conversation

sudo-suhas commented Oct 3, 2017

What does this do / why do we need it?

What should your reviewer look out for in this PR?

Do you need help or clarification on anything?

Which issue(s) does this PR fix?

Changelog

jmank88 Oct 3, 2017

Choose a reason for hiding this comment

sudo-suhas Oct 3, 2017

Choose a reason for hiding this comment

sdboyer Oct 6, 2017

Choose a reason for hiding this comment

sudo-suhas commented Oct 3, 2017

sdboyer commented Oct 7, 2017

sudo-suhas commented Oct 7, 2017

sdboyer commented Oct 12, 2017

sudo-suhas commented Oct 13, 2017

sdboyer commented Oct 15, 2017

sdboyer left a comment

Choose a reason for hiding this comment

sudo-suhas commented Oct 16, 2017 • edited Loading

darkowlzz Oct 20, 2017 • edited Loading

Choose a reason for hiding this comment

sudo-suhas Oct 21, 2017

Choose a reason for hiding this comment

sudo-suhas commented Oct 21, 2017 • edited Loading

sudo-suhas commented Nov 4, 2017

sudo-suhas commented Nov 11, 2017

sdboyer commented Nov 11, 2017 via email

sudo-suhas commented Nov 11, 2017

sdboyer commented Nov 14, 2017

sudo-suhas commented Nov 14, 2017

sdboyer left a comment

Choose a reason for hiding this comment

sdboyer Nov 15, 2017

Choose a reason for hiding this comment

sdboyer Nov 15, 2017

Choose a reason for hiding this comment

sudo-suhas commented Nov 18, 2017

sdboyer commented Dec 1, 2017

sudo-suhas commented Dec 2, 2017

sdboyer left a comment

Choose a reason for hiding this comment

sudo-suhas commented Dec 4, 2017

sdboyer commented Dec 6, 2017

darkowlzz commented Dec 6, 2017 • edited Loading

sudo-suhas commented Dec 8, 2017

sdboyer commented Dec 8, 2017

sdboyer commented Dec 8, 2017

sudo-suhas commented Dec 8, 2017

sudo-suhas commented Dec 9, 2017

sudo-suhas commented Oct 16, 2017 •

edited

Loading

darkowlzz Oct 20, 2017 •

edited

Loading

sudo-suhas commented Oct 21, 2017 •

edited

Loading

darkowlzz commented Dec 6, 2017 •

edited

Loading