allow cachedir override using env var

[sudo-suhas]

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]

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

[sudo-suhas]

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

[sdboyer]

yep, i agree with @jmank88's suggestion.

[sudo-suhas]

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]

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]

@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]

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]

@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]

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]

@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]

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!

[sudo-suhas]

Done with rebase.

[sudo-suhas]

@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]

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

[sudo-suhas]

@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]

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]

My apologies. Hope you enjoy your vacation 😄 .

[sdboyer]

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]

@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]

"default" is better than "fallback" here.

[sdboyer]

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]

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

1. $DEPCACHEDIR is not set. Same behavior as earlier, no confusion here.
2. $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]

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]

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]

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]

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]

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]

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

[sdboyer]

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

[sdboyer]

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

[sudo-suhas]

Thank you for your guidance!

[sudo-suhas]

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