tools / godep

dependency tool for go
http://godoc.org/github.com/tools/godep
BSD 3-Clause "New" or "Revised" License
5.54k stars 454 forks source link

godep restore makes impossible to update project with go get #453

Open Raffo opened 8 years ago

Raffo commented 8 years ago

Currently godep restore puts the repositories in detached HEAD and this makes impossible to execute a go get -u github.com/foo/bar. If a high number of projects using godeps are used and if godep restore is used frequently, this leads to a very high number of projects in detached state which makes very hard to update them. Are we able to avoid this somehow?

freeformz commented 8 years ago

Longer term, yes, by removing the need to godep restore. A go tooling change recently (go1.6) made this a problem AFAIK.

freeformz commented 8 years ago

PS: godep restore is doing what it's always done, restore the recorded versions to the $GOPATH. it can only do this by checking out the various shas (AFAIK) it's recorded.

As a temp work around I could implement a godep unrestore, which would check out master again. But I'm hesitant to add another command that should be removed in the future and I'm not sure it's super helpful in the workflow ... ie. you have to remember to use it.

Thoughts?

Raffo commented 8 years ago

I was about to implement a godep unrestore myself, I think it would be useful (I can work on it and submit a PR, but if you want to work on it, as you whish). More important, though is to document in the README what happens now if a user executes godep restore.

Also, can you give me the detail of this change in 1.6 you are mentioning above?

freeformz commented 8 years ago

PRs welcome. I don't have an exact commit to golang where this broke. Let me see if I can dig it up though.

freeformz commented 8 years ago

It was likely this one: https://github.com/golang/go/commit/42206598671a44111c8f726ad33dc7b265bdf669

freeformz commented 8 years ago

@Raffo What would you suggest be added to the README? It states what godep restore already does in a non VCS specific manner, since godep handles multiple types of VCS.

Raffo commented 8 years ago

Thanks for the link to the commit.

Do you think that a warning like "godep restore could leave some of the dependencies in a detached state. This could lead to problems to update your dependencies" would be too much? Maybe we could add some text to specify that the users should avoid using godep restore if not strictly necessary.

freeformz commented 8 years ago

How about

NOTE: godep restore leaves git repositories in a detached state. go1.6+ no longer checks out the master branch when doing a go get, see here.

Then if you land an unrestore PR we can adjust that to recommend the usage of unrestore afterwards?

Raffo commented 8 years ago

Sounds good to me.

freeformz commented 8 years ago

@Raffo README / restore help text updated in 584a3f5bf2671886bd3f7dc2b252762b5aa2a087

Raffo commented 8 years ago

:+1:

Raffo commented 8 years ago

As I imagined, it's very easy to hack an "unrestore" command to checkout the master, but it's not so easy to make the unrestore command so general that can handle cases in which the default remote branch is different from the master. I would have to add this information to Godeps.json (the remote HEAD), but I'm not sure we want that. If we agree that we will implement that, I wouldn't know any valid command that will give me the current remote HEAD. The only valid info I know how to get is in git remote show origin, but I don't really want to parse the output. Any suggestion/help?

Raffo commented 8 years ago

@freeformz Any input on what I wrote above?

freeformz commented 8 years ago

Why does the remote branch matter? I don't think we want to pull new code with unrestore, just check out the local master.

Raffo commented 8 years ago

@freeformz because it's not obvious that the main branch is the master. If you clone a repository which remote HEAD is not the master you will end up with something different than a master branch. In such a case, if godep unrestore tries to restore it to the master branch this could lead to wrong results.

freeformz commented 8 years ago

It only matters what it's merged to locally AFAIK, so git config --local branch.master.merge should tell us what to checkout.

I could totally be wrong here, I don't think I've ever worked with remotes who HEAD was not master.

Raffo commented 8 years ago

Take the gin repository: the master branch exist, but the remote HEAD is develop. I think that in this case it will not work.

freeformz commented 8 years ago

I see. If there is only one branch returned from git config --local --get-regexp 'branch.*' we could check that out and otherwise punt with instructions to the user to start?

Raffo commented 8 years ago

Or we just modify Godep.json to be something like:

    {
            "ImportPath": "github.com/spf13/pflag",
            "Rev": "8f6a28b0916586e7f22fe931ae2fcfc380b1c0e6",
            "RemoteHEAD": "master"
        },

And use the RemoteHEAD when executing unrestore. This will break only in case the remote head of the repo changes from the last time the Godep.json is updated, which is not perfect, but acceptable (IMO).

freeformz commented 8 years ago

I'm not sure I really want to require people to populate that field manually. Also AFAICT RemoteHEAD is detectable through either:

$ git remote show origin
...
HEAD branch: develop
...

or

$ git branch -r | grep "origin/HEAD ->"
  origin/HEAD -> origin/develop

possibly some other means as well.

Raffo commented 8 years ago

This is exactly what I meant, RemoteHEAD would be populated when godep save is executed. When doing godep unrestore we could also add a --force option (I'm not sure if this flag is already used in another way in other godep commands) to remove the dependency and re-executing a go get from scratch in case of failures. Obviously after a warning and a confirmation from the user.

freeformz commented 8 years ago

I'm not sure we should save RemoteHEAD though and just honor the remote head from git info for what's already there.

Raffo commented 8 years ago

So, what you suggest is to just use the info from git remote show origin , i.e:

* remote origin
  Fetch URL: https://github.com/kubernetes/kubernetes.git
  Push  URL: https://github.com/kubernetes/kubernetes.git
  HEAD branch: master
  Remote branches:

Is this right?

eduncan911 commented 8 years ago

FWIW, today I got fed up with this and just _banned_ godep restore from my entire system. I did this with a script:

https://github.com/eduncan911/dotfiles/blob/master/bin/godep

If you want it, first make sure your personal ~/bin comes before your $GOPATH/bin:

$ echo $PATH
...:/Users/eric/bin:/Users/eric/go/bin:...

If so, you are good to go. If not, you may want to tweak your $PATH to use your personal ~/bin before your GOPATH/bin.

Get the file locally:

$ curl -L https://raw.githubusercontent.com/eduncan911/dotfiles/master/bin/godep --create-dirs -o ~/bin/godep
$ chmod 755 ~/bin/godep

Use it normally:

$ godep go run main.go
$ godep get foo/bar

...etc. And when you try to do godep restore:

$ godep restore
WARNING: godep restore <- this will frack with your $GOPATH !!
WARNING: to override, use your installed version of godep like this:
WARNING:      $GOPATH/bin/godep restore
WARNING: Aborting ...

hehe. 👍

I'm in the habit of using the godep prefix for all my go needs anyways:

$ godep go test ./...
$ godep go run main.go
$ godep get foo/bar

...etc. But I've accidently used godep restore a few times, like today when I ran a Makefile for a new repo I was working on. Argh! Spent over an hour restoring all of my $GOPATH repos back to master.

No more!

tsuna commented 8 years ago

No need to spend an hour restoring them all, just do:

find $GOPATH/src -type d -name .git -execdir git --git-dir={} checkout master \;
eduncan911 commented 8 years ago

i have to exclude the dozen or so that I am working on... that do have pending commits, on different branches, etc.