Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

Non-deterministic cache location #1066

Closed
sudo-suhas opened this issue Aug 28, 2017 · 11 comments · Fixed by #1234
Closed

Non-deterministic cache location #1066

sudo-suhas opened this issue Aug 28, 2017 · 11 comments · Fixed by #1234

Comments

@sudo-suhas
Copy link
Contributor

What version of Go (go version) and dep (git describe --tags) are you using?

E:\Installs\go-tools\src\github.com\golang\dep (master)
λ go version
go version go1.8.3 windows/amd64

E:\Installs\go-tools\src\github.com\golang\dep (master)
λ git describe --tags
v0.3.0-162-gf134697

What dep command did you run?

E:\workspace\golang\src\github.com\golang\dep (master)
λ dep ensure

What did you expect to see?

I like to differentiate the Go tools I use with Go like gometalinter and install them to a separate location than %gopath%. I did the same with dep(installed to E:\Installs\go-tools). I was expecting that the cache would be stored in this Go workspace(E:\Installs\go-tools\pkg\dep).

What did you see instead?

The cache was generated in %gopath%/pkg/dep and not relative to the actual installation path of dep. While this is by no means a serious issue, this can negatively affect performance for anybody who uses multiple go workspaces. I think it will be better to have a cache location which is independent of the %gopath%. I apologise if this is a known limitation or if it has been reported already.

@sdboyer
Copy link
Member

sdboyer commented Aug 28, 2017

yep, having the cache location tied to a single GOPATH is less than ideal, largely for this reason. we selected GOPATH/pkg/dep because it was easy, but this is a major drawback, and will only become worse going forward. this did allow us to avoid dealing with bikesheddy questions about where the cache should be located, though. small blessings.

i'm tentatively OK with adding an environment variable that can be used to override the default behavior.

@ibrasho
Copy link
Collaborator

ibrasho commented Aug 28, 2017

Can we change the cache's default location to a more typical location like $HOME/.cache/pkg? :grin
Or is that going to start a new bikeshed discussion?

@sdboyer
Copy link
Member

sdboyer commented Aug 28, 2017

Or is that going to start a new bikeshed discussion?

yes, yes it will 😄

@sdboyer
Copy link
Member

sdboyer commented Oct 2, 2017

@sudo-suhas still interested in working on this?

@sudo-suhas
Copy link
Contributor Author

Yes @sdboyer, I am interested in working on this, Could you give me a couple of pointers for where I would need to do the changes?

@sdboyer
Copy link
Member

sdboyer commented Oct 2, 2017

@sudo-suhas the path that's ultimately used is set centrally here. however, it's not allowed to read directly from the env at that point - external state like that should be injected at the time the dep.Ctx is created.

i don't think there should be much more involved than work around those two points, though.

@sudo-suhas
Copy link
Contributor Author

@sdboyer Need a little help with writing the tests.

  • I am trying to check the output of ctx.SourceManager() after creating the context with and without the Cachedir attribute (which I just added). However, to check the private field cachedir I would need to place this test in source_manager_test.go which seems counter-intuitive to me. Shall I export the field from SourceMgr instead?
  • Do I need to add a test to check that DEPCACHEDIR is correctly read from the env? If so, what would be the correct place for doing so?

Also, what would be the right place to document this?

@sdboyer
Copy link
Member

sdboyer commented Oct 2, 2017

I am trying to check the output of ctx.SourceManager() after creating the context with and without the Cachedir attribute (which I just added). However, to check the private field cachedir I would need to place this test in source_manager_test.go which seems counter-intuitive to me. Shall I export the field from SourceMgr instead?

i don't think we need to worry about this - it's really not in the scope of that package's responsibility to make sure that it's written to the correct place on disk. that's gps' job, and that's where the test belong for it.

it's probably sufficient here to just have an integration test that makes sure the env var is being respected. that'll mean compiling and invoking a subprocess in the test - you could probably borrow from the testing harness for that, though i doubt you can use the testing harness directly for the check you need to do.

@sudo-suhas
Copy link
Contributor Author

Turns out I cannot add the test in source_manager_test.go because that results in a circular import.

Regarding the integration test, I did the following:

  • Added fixtures for cachedir (copied from testdata/harness_tests/default/initial)
  • Used the helpers to create an instance of integration.TestProject
  • Set the env variable for DEPCACHEDIR to a temp directory (testProj.TempDir("cachedir"))
  • Ran the dep ensure command using testProj.DoRun

This ends up pulling something into the cache directory. What it be enough to check that the folder cachdir/sources/https---github.com-sdboyer-deptest exists?

@sdboyer
Copy link
Member

sdboyer commented Oct 2, 2017

could we move more itemized discussion to a PR?

@sudo-suhas
Copy link
Contributor Author

@sdboyer My apologies. Have created a PR - #1234. Please take a look.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants