Why did dep not used my imported version?

[carolynvs]

Warning: I am testing dep out on Kuberentes, and am using a few fixes to the godep config file to get this far.

go version: go version go1.8.3 darwin/amd64
dep version: v0.2.1-1-ga085554 ( PR #938 )

The dependency on cloud.google.com/go was for 0.1.0-115-g3b1ae45, specifically the untagged revision 3b1ae45. After running dep init and importing the config from godep, that preferred/imported revision was ignored during solve. Or at least that's the way it looks.

dep init -v
Importing configuration from godep. These are only initial constraints, and are further refined during the solve process.
Detected godep configuration files...
  Loading /Users/carolynvs/go/src/k8s.io/kubernetes/Godeps/Godeps.json
Converting from Godeps.json ...
  ... TRUNCATED
  Using ^0.1.0-115-g3b1ae45 as initial constraint for imported dep cloud.google.com/go
  Trying * (3b1ae45) as initial lock for imported dep cloud.google.com/go
  ... TRUNCATED
Root project is "k8s.io/kubernetes"
 1506 transitively valid internal packages
 439 external packages imported from 112 projects
(0)   ✓ select (root)
   ... TRUNCATED
(3)	? attempt cloud.google.com/go with 1 pkgs; 15 versions to try
(3)	    try cloud.google.com/[email protected]
(4)	✗   cloud.google.com/[email protected] not allowed by constraint ^0.1.0-115-g3b1ae45:
(4)	    ^0.1.0-115-g3b1ae45 from (root)
(3)	    try cloud.google.com/[email protected]
(4)	✗   cloud.google.com/[email protected] not allowed by constraint ^0.1.0-115-g3b1ae45:
(4)	    ^0.1.0-115-g3b1ae45 from (root)
(3)	    try cloud.google.com/[email protected]
(4)	✗   cloud.google.com/[email protected] not allowed by constraint ^0.1.0-115-g3b1ae45:
(4)	    ^0.1.0-115-g3b1ae45 from (root)
(3)	    try cloud.google.com/[email protected]
(4)	✗   cloud.google.com/[email protected] not allowed by constraint ^0.1.0-115-g3b1ae45:
(4)	    ^0.1.0-115-g3b1ae45 from (root)
(3)	    try cloud.google.com/[email protected]
(4)	✗   cloud.google.com/[email protected] not allowed by constraint ^0.1.0-115-g3b1ae45:
(4)	    ^0.1.0-115-g3b1ae45 from (root)
(3)	    try cloud.google.com/[email protected]
(4)	✗   cloud.google.com/[email protected] not allowed by constraint ^0.1.0-115-g3b1ae45:
(4)	    ^0.1.0-115-g3b1ae45 from (root)
(3)	    try cloud.google.com/[email protected]
(4)	✗   cloud.google.com/[email protected] not allowed by constraint ^0.1.0-115-g3b1ae45:
(4)	    ^0.1.0-115-g3b1ae45 from (root)
(3)	    try cloud.google.com/[email protected]
(4)	✗   cloud.google.com/[email protected] not allowed by constraint ^0.1.0-115-g3b1ae45:
(4)	    ^0.1.0-115-g3b1ae45 from (root)
(3)	    try cloud.google.com/[email protected]
(4)	✗   cloud.google.com/[email protected] not allowed by constraint ^0.1.0-115-g3b1ae45:
(4)	    ^0.1.0-115-g3b1ae45 from (root)
(3)	    try cloud.google.com/[email protected]
(4)	✗   cloud.google.com/[email protected] not allowed by constraint ^0.1.0-115-g3b1ae45:
(4)	    ^0.1.0-115-g3b1ae45 from (root)
(3)	    try cloud.google.com/[email protected]
(3)	✓ select cloud.google.com/[email protected] w/2 pkgs

I was expecting to see a line like this instead:

  try cloud.google.com/go@3b1ae45

It's not clear from the output why the revision wasn't considered before those other versions? Is it because the constraint is for a version and not a branch?

[sdboyer]

that is indeed odd - i would've expected to see it up at the top. the first thing i'd check is that it is, indeed, actually making it into the Lock that's passed to the solver. after that, i'd start debugging in solver.findValidVersion() to see what the queue that's actually built for cloud.google.com/go looks like.

[carolynvs]

Thanks! I'll try that.

[carolynvs]

@sdboyer Okay here is what is happening:

1. The imported manifest has a semver constraint.
2. The imported lock has an untagged revision.
3. solve calls getLockVersionIfValid which detects that the revision doesn't strictly match the constraint. It checks if there's a tag for that revision which would match, doesn't find one, and it breaks the lock.

From the code perspective I get what happened, though we should at least logged something in this case, as otherwise that's a pretty sneaky thing for gps to do. 😀 I'm going to add an issue for that in a minute.

From a human perspective, when I have a constraint like >= 1.0.0, and a revision which if you ran git describe --tags on, returned 1.0.0-2-abc123, I would personally consider that to satisfy the intent of the constraint.

Do we want dep to stick with this current behavior, essentially seriously making it hard for someone to lock to an untagged revision while still trying to use semver in the manifest? In the distant future maybe that's not a huge deal when more things are properly tagged, but right now, that would quickly convince me to stop using semver, and go back to constraining to just master.

Obviously I prefer that dep be a bit looser about revisions matching semver constraints... What do you think?

[carolynvs]

Just adding this here so that I don't forget. 😊 If we end up not changing gps's behavior, the importers all need to stop turning strings like v1.0.0-2-abc123 into a semver constraint, and instead fall back to the any constraint to avoid lock breaks.

[sdboyer]

crap. sorry, i missed this, my bad.

maybe we can find a nice middle ground - if the semver value that comes back has prerelease data on it (which i believe is how these suffixes get interpreted), then we just drop the constraint. would that cover our problems?

[carolynvs]

I think that unless the revision that they are currently locked to is tagged, we shouldn't add a semver constraint during import. If they are currently on a prerelease version, then it should be safe to add a constraint, for example every moby release is now a prerelease thanks for their creative CE versioning scheme, e.g. 17.03.1-ce. But I think dep will handle that just fine (i.e. not change the locked rev during solve) as long as the revision is tagged.

[sdboyer]

sure, ok - that's probably easier and less complex. and that's really worth doing, given that it was trying to be perhaps a bit too smart that got us into this in the first place 😉

let's just make sure that we've got some pretty exhaustive tests for the possible combinations!

[carolynvs]

Working on this today and as part of it, all the logic for the importers is going to be consolidated into a single function. That way importers only deal with the config parsing, and none of the logic about converting rando strings to locks/constraints is handled individually in each importer.