Incorrect branch taken on n+1 dependency #1112

mremond · 2017-09-01T14:40:14Z

What version of `dep` are you using (`dep version`)?

Master head from today. I tried previously with dep 0.3.0

What `dep` command did you run?

I have build an exemple project.

Commit 1.

Initial try. I did dep ensure on the project after dep init and changing the constraint on go_cql to a specific revision (see https://github.com/processone/dep-resolv/blob/master/Gopkg.toml#L26).

dep used the outdated branch internal_changes (the first one alphabetically) as value for branch, instead of master.

PROJECT                          CONSTRAINT  VERSION                  REVISION  LATEST   PKGS USED
github.com/gocql/gocql           *                                    7e9748             4  
github.com/golang/snappy         *           branch master            553a641   553a641  1  
github.com/hailocab/go-hostpool  *           branch internal_changes  d94a633   d94a633  1  
golang.org/x/net                 *           branch master            66aacef   66aacef  1  
gopkg.in/inf.v0                  *           v0.9.0                   3887ee9   3887ee9  1

Second stage. I added constraint on the sub dependencies:

See: processone/dep-resolv@1d6b1d3#diff-836546cc53507f6b2d581088903b1785R30

Doing dep ensure -update does nothing.

dep status is well aware of the discrepency:

$ dep status 
PROJECT                          CONSTRAINT     VERSION                  REVISION  LATEST   PKGS USED
github.com/gocql/gocql           *                                       7e9748             4  
github.com/golang/snappy         *              branch master            553a641   553a641  1  
github.com/hailocab/go-hostpool  branch master  branch internal_changes  d94a633   e80d13c  1  
golang.org/x/net                 *              branch master            66aacef   66aacef  1  
gopkg.in/inf.v0                  *              v0.9.0                   3887ee9   3887ee9  1

What did you expect to see?

I expect that Gopkg.lock would have used master branch as default branch without the need to add a specific constraint.
I expect adding the contraint and running go ensure -update to select the right dependency for second level dependency.

What did you see instead?

Project is stuck with outdated version of a subdependency. Only way to use the right version is to fix Gopkg.lock manually and do dep ensure.

The text was updated successfully, but these errors were encountered:

darkowlzz · 2017-09-02T14:43:11Z

@mremond hi, thanks for trying dep.

I tried to reproduce this but couldn't get the transitive dependency go-hostpool to be in branch internal_changes.
I cloned your repo dep-resolv and deleted Gopkg.toml, Gopkg.lock and vendor/. And then ran dep init. Got the dependencies as:

$ dep status
PROJECT                          CONSTRAINT     VERSION        REVISION  LATEST   PKGS USED
github.com/gocql/gocql           branch master  branch master  066e974   066e974  4
github.com/golang/snappy         *              branch master  553a641   553a641  1
github.com/hailocab/go-hostpool  *              branch master  e80d13c   e80d13c  1
gopkg.in/inf.v0                  *              v0.9.0         3887ee9   3887ee9  1

And then I changed gocql constraint to revision 7e9748 and ran ensure. Dependencies:

$ dep status
PROJECT                          CONSTRAINT  VERSION        REVISION  LATEST   PKGS USED
github.com/gocql/gocql           *                          7e9748             4
github.com/golang/snappy         *           branch master  553a641   553a641  1
github.com/hailocab/go-hostpool  *           branch master  e80d13c   e80d13c  1
golang.org/x/net                 *           branch master  66aacef   66aacef  1
gopkg.in/inf.v0                  *           v0.9.0         3887ee9   3887ee9  1

go-hostpool seems to remain in master branch. Am I missing some step? Can you reproduce it again?

Regarding setting constraint for go-hostpool, since it's a transitive dependency dep would ignore its constraints from Gopkg.toml. We have a FAQ entry for constraining transitive dependencies, How do I constrain a transitive dependency's version?.

mremond · 2017-09-04T14:31:32Z

@darkowlzz Thank you very much for your reply.
Strangely, I can reproduce the problem. I did check out the repository, deleted Gopkg.lock and vendor dir. After a dep ensure I end up have the go-hostpool dep check out from internal-changes branch:

 dep status 
PROJECT                          CONSTRAINT  VERSION                  REVISION  LATEST   PKGS USED
github.com/gocql/gocql           *                                    7e9748             4  
github.com/golang/snappy         *           branch master            553a641   553a641  1  
github.com/hailocab/go-hostpool  *           branch internal_changes  d94a633   d94a633  1  
golang.org/x/net                 *           branch master            66aacef   66aacef  1  
gopkg.in/inf.v0                  *           v0.9.0                   3887ee9   3887ee9  1

What can I do to find why it is taking that branch ? Is there special debug options ?
What else can have an impact on the resolution algorithm ?

sdboyer · 2017-09-04T17:52:56Z

go_cql to a specific revision (see https://github.com/processone/dep-resolv/blob/master/Gopkg.toml#L26).

bit of a side note here, but Gopkg.toml doesn't accept abbreviated revision hash digests - you have to put the full one in if you really want to pin to a revision. (which, in general, we do not recommend.) that this -

dep status is well aware of the discrepency:

happens is a bit misleading; that's really just dep status passing through directly what it finds without validating it first.

Strangely, I can reproduce the problem.

you didn't walk it back as far as @darkowlzz did, though - you let Gopkg.toml stick around. that probably explains the discrepancy, though...i admit i'd a bit mystified as to why. so:

What can I do to find why it is taking that branch ?

pass -v. that'll show a trace of the resolution algorithm, allowing you to see why other versions were dismissed.

What else can have an impact on the resolution algorithm ?

right now, just package existence - if you're importing a package that does not exist at the target version, that version will be discarded and then next available version will be attempted.

mremond · 2017-09-04T19:39:44Z

Thanks @sdboyer

We will lock the dependency on the full revision id. We had to do this as we are waiting for a bug fix and needed to lock the lib on the last working version for us.

Regarding the test, I did restart from scratch, to be sure, as you pointed you. I removed Gopkg.toml, Gopkg.lock and vendor dir.

I have the same behaviour with just the Go file in the directory, without locking gocql to a given version (just checkout, delete and dep init).

Here is the printout of dep init in verbose mode:

$ dep init -v
Getting direct dependencies...
Checked 1 directories for packages.
Found 1 direct dependencies.
Root project is "github.com/processone/dep-resolv"
 1 transitively valid internal packages
 1 external packages imported from 1 projects
(0)   ✓ select (root)
(1)	? attempt github.com/gocql/gocql with 1 pkgs; 5 versions to try
(1)	    try github.com/gocql/gocql@master
(1)	✓ select github.com/gocql/gocql@master w/4 pkgs
(2)	? attempt gopkg.in/inf.v0 with 1 pkgs; 1 versions to try
(2)	    try gopkg.in/[email protected]
(2)	✓ select gopkg.in/[email protected] w/1 pkgs
(3)	? attempt github.com/hailocab/go-hostpool with 1 pkgs; 3 versions to try
(3)	    try github.com/hailocab/go-hostpool@internal_changes
(3)	✓ select github.com/hailocab/go-hostpool@internal_changes w/1 pkgs
(4)	? attempt github.com/golang/snappy with 1 pkgs; 2 versions to try
(4)	    try github.com/golang/snappy@master
(4)	✓ select github.com/golang/snappy@master w/1 pkgs
  ✓ found solution with 7 packages from 4 projects

Solver wall times by segment:
     b-list-versions: 7.021659952s
     b-source-exists: 5.607790189s
         b-list-pkgs: 707.973885ms
              b-gmal: 681.382004ms
         select-atom:    422.666µs
  b-deduce-proj-root:    355.994µs
             satisfy:    254.807µs
            new-atom:     133.72µs
         select-root:     52.211µs
               other:     17.698µs

  TOTAL: 14.020043126s

  Locking in master (553a641) for transitive dep github.com/golang/snappy
  Using master as constraint for direct dep github.com/gocql/gocql
  Locking in master (066e974) for direct dep github.com/gocql/gocql
  Locking in v0.9.0 (3887ee9) for transitive dep gopkg.in/inf.v0
  Locking in internal_changes (d94a633) for transitive dep github.com/hailocab/go-hostpool
(1/4) Wrote gopkg.in/[email protected]
(2/4) Wrote github.com/golang/snappy@master
(3/4) Wrote github.com/hailocab/go-hostpool@internal_changes
(4/4) Wrote github.com/gocql/gocql@master

It seems the first branch tried on github.com/hailocab/go-hostpool is the first one in alphabetical order, and it is accepted on first try. It does not seem to consider master.

I hope this helps,

sdboyer · 2017-09-05T01:16:52Z

We will lock the dependency on the full revision id. We had to do this as we are waiting for a bug fix and needed to lock the lib on the last working version for us.

sure, that's not an uncommon approach. it's generally not strictly necessary, as running dep ensure -update with specific named dependencies will almost always keep gocql from moving, but it is, of course, up to you on how you construct your workflow and manage tradeoffs.

Regarding the test, I did restart from scratch, to be sure, as you pointed you. I removed Gopkg.toml, Gopkg.lock and vendor dir.

oh, ok - your previous response indicated that you only removed the latter two:

I did check out the repository, deleted Gopkg.lock and vendor dir

so, we're back in very, very weird territory. i can't replicate your situation locally, either - i see the same results as @darkowlzz.

the only thing i can think that might be causing it to pick that old version over another is if you had internal_changes checked out at $GOPATH/src/github.com/hailocab/go-hostpool, but that should only come into play if you pass -gopath, which you haven't.

could you have a look at $GOPATH/pkg/dep/sources/https---git.f4.workers.dev-hailocab-go-hostpool, and tell me the output of git branch -av?

sdboyer · 2017-09-05T01:18:54Z

It seems the first branch tried on github.com/hailocab/go-hostpool is the first one in alphabetical order, and it is accepted on first try. It does not seem to consider master.

oh, just to be explicit - the logic definitely puts default branches before non-default branches (which are then sorted lexicographically). that's why both of us are seeing master. i'm just mystified on why you'd be getting something different - but hopefully that git branch -av output will help clarify some things.

oh, also, the output of git ls-remote https://github.com/hailocab/go-hostpool would be helpful.

mremond · 2017-09-05T07:43:48Z

Thanks !

I tried on another machine with and almost empty GOPath and it works as expected, so this confirm what you knew: It is an issue related to my environment.

Let's try to find the difference.

Here are some info that your requested.

From go/pkg/dep/sources/https---git.f4.workers.dev-hailocab-go--hostpool:

$ git branch -av
* (HEAD detached at d94a633)      d94a633 refactor to selector interface
  master                          e80d13c Merge pull request #3 from hailocab/upstream-merge
  remotes/origin/HEAD             -> origin/master
  remotes/origin/internal_changes d94a633 refactor to selector interface
  remotes/origin/lockfree         3b501aa update benchmark to work with new private api
  remotes/origin/master           e80d13c Merge pull request #3 from hailocab/upstream-merge

Here is git ls-remote:

$ git ls-remote https://github.com/hailocab/go-hostpool
Warning: untrusted X11 forwarding setup failed: xauth key data not generated
e80d13ce29ede4452c43dea11e79b9bc8a15b478	HEAD
d94a633b9e116923d4998c671a68d0bcd7103d40	refs/heads/internal_changes
3b501aa84aea8c0164cbc39c996d52abfc37255c	refs/heads/lockfree
e80d13ce29ede4452c43dea11e79b9bc8a15b478	refs/heads/master
a977ea43b7cb5c26fdaf40e4d8982c306708a5ee	refs/pull/1/head
23f696ab60bb047f244932aae97823e395be93db	refs/pull/2/head
36b27c05fb7211517d96baba7189e1e048274da6	refs/pull/2/merge
8b0d3ed227b90eab68e2c0fd146db4586714813d	refs/pull/3/head
2cc96589091cef4073f6602f61fc407da31c60cf	refs/pull/4/head
3062b474cc05330c1d58c3cb48a1b078f764d83e	refs/pull/4/merge
6715a4b8a5e3fa2fccff36f4e97f75bc80b456bb	refs/pull/5/head

sdboyer · 2017-09-05T12:42:57Z

i'm glad you weren't able to replicate it on another machine - really, a relief. but it's still pretty disturbing that you were observe, and replicate, this behavior on any machine. that git output all looks the way i'd expect it to, which leaves me quite mystified as to what might have been causing this 😢

at this point, all i can think of to have you do on the problem machine is hack up SourceManager.ListVersions() to fmt.Println() the results of srcg.listVersions(context.TODO()) before returning it.

mremond · 2017-09-05T16:47:14Z

Here is the result: [master internal_changes lockfree]
It seems to imply it should try (and select) master first.
You gave me a nice entry point in code, so I will debug more and report.

mremond · 2017-09-05T19:18:36Z

Strangely, retrying now from home and I get a different order from srcg.listVersions(context.TODO()): [internal_changes lockfree master]

Interestingly, ListVersions comment says:

The list is not sorted; while it may be returned in the order that the underlying VCS reports version information, no guarantee is made. It is expected that the caller either not care about order, or sort the result themselves.

So, it seems to imply that if VCS return list in different order, we will try the list of branches in a different order.

mremond · 2017-09-05T20:33:37Z

I think I found the issue. I will try to test some possible fix / workaround and report here.

mremond · 2017-09-05T21:03:53Z

The issue happens when you need to use private repository.

To use private repository on Github with go get and dep, you need to set up git to use SSH with your key instead of https protocol. You can do this with following git configuration command:

git config --global url."[email protected]:".insteadOf "https://github.com/"

When you do this, you get through ssh and can have SSH warning at the beginning of git commands. In my case, when I issue the command git ls-remote [email protected]:hailocab/go-hostpool.git, I get:

Warning: untrusted X11 forwarding setup failed: xauth key data not generated
e80d13ce29ede4452c43dea11e79b9bc8a15b478 HEAD
d94a633b9e116923d4998c671a68d0bcd7103d40 refs/heads/internal_changes
3b501aa84aea8c0164cbc39c996d52abfc37255c refs/heads/lockfree
e80d13ce29ede4452c43dea11e79b9bc8a15b478 refs/heads/master
a977ea43b7cb5c26fdaf40e4d8982c306708a5ee refs/pull/1/head
23f696ab60bb047f244932aae97823e395be93db refs/pull/2/head
36b27c05fb7211517d96baba7189e1e048274da6 refs/pull/2/merge
8b0d3ed227b90eab68e2c0fd146db4586714813d refs/pull/3/head
2cc96589091cef4073f6602f61fc407da31c60cf refs/pull/4/head
3062b474cc05330c1d58c3cb48a1b078f764d83e refs/pull/4/merge
6715a4b8a5e3fa2fccff36f4e97f75bc80b456bb refs/pull/5/head

The first warning line is confusing the function gitSource.listVersions(ctx context.Context) as head revision is taken on the first line:

    headrev := Revision(all[0][:40])

I confirm that removing the "insteadOf" option on Github solves the issue and dep is starting to get the correct dependency because it can resolve HEAD properly and start getting the default branch.

I also confirm that I have a patch that fix the issue. I will clean it up tomorrow to submit a proper pull request to fix the issue.

Thanks a lot for your initial pointer and guidance to help me get into the dep source code :)

sdboyer · 2017-09-05T23:01:15Z

Strangely, retrying now from home and I get a different order from srcg.listVersions(context.TODO()): [internal_changes lockfree master]

ugh, i realized i sent you to not the most helpful spot. the interface contract of ListVersions() does not imply any particular order, so these changes aren't meaningful (they're mostly a product of passing through Go's map order randomization). bridge.listVersions() is where the sorting actually happens.

You can do this with following git configuration command:

insteadOf is one way of doing it, though dep will eventually cycle through to attempting ssh - it shouldn't actually be necessary to do. our FAQ also has instructions on authenticating.

The first warning line is confusing the function

ah, yes! that would definitely do it.

note that we have another PR open, #1053, which is also working on this same bit of logic.

Thanks a lot for your initial pointer and guidance to help me get into the dep source code :)

absolutely! 🎉

mremond · 2017-09-06T12:54:56Z

Sure, I will check this PR and see how I can make sure I propose a PR that is compliant / easy to merge with #1053

mremond · 2017-09-07T12:02:42Z

@sdboyer I tested #1053 and as I suspected, it also solve my issue as it does not assume HEAD is on first line. So, I guess it is the way forward as it solves two issues.

What can I do to help with with patch proposed on #1053 ?

darkowlzz · 2017-09-08T13:56:54Z

@mremond #1053 seems to be ready. I have updated the PR description to close this issue once that is fixed 🙂

mremond mentioned this issue Sep 7, 2017

Don't assume every git repository has a HEAD #1053

Merged

darkowlzz closed this as completed in #1053 Sep 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Incorrect branch taken on n+1 dependency #1112

Incorrect branch taken on n+1 dependency #1112

mremond commented Sep 1, 2017

darkowlzz commented Sep 2, 2017

mremond commented Sep 4, 2017

sdboyer commented Sep 4, 2017

mremond commented Sep 4, 2017

sdboyer commented Sep 5, 2017 •

edited

Loading

sdboyer commented Sep 5, 2017

mremond commented Sep 5, 2017

sdboyer commented Sep 5, 2017

mremond commented Sep 5, 2017

mremond commented Sep 5, 2017 •

edited

Loading

mremond commented Sep 5, 2017

mremond commented Sep 5, 2017 •

edited

Loading

sdboyer commented Sep 5, 2017

mremond commented Sep 6, 2017

mremond commented Sep 7, 2017

darkowlzz commented Sep 8, 2017

Incorrect branch taken on n+1 dependency #1112

Incorrect branch taken on n+1 dependency #1112

Comments

mremond commented Sep 1, 2017

What version of dep are you using (dep version)?

What dep command did you run?

What did you expect to see?

What did you see instead?

darkowlzz commented Sep 2, 2017

mremond commented Sep 4, 2017

sdboyer commented Sep 4, 2017

mremond commented Sep 4, 2017

sdboyer commented Sep 5, 2017 • edited Loading

sdboyer commented Sep 5, 2017

mremond commented Sep 5, 2017

sdboyer commented Sep 5, 2017

mremond commented Sep 5, 2017

mremond commented Sep 5, 2017 • edited Loading

mremond commented Sep 5, 2017

mremond commented Sep 5, 2017 • edited Loading

sdboyer commented Sep 5, 2017

mremond commented Sep 6, 2017

mremond commented Sep 7, 2017

darkowlzz commented Sep 8, 2017

What version of `dep` are you using (`dep version`)?

What `dep` command did you run?

sdboyer commented Sep 5, 2017 •

edited

Loading

mremond commented Sep 5, 2017 •

edited

Loading

mremond commented Sep 5, 2017 •

edited

Loading