Godeps.json with extra hash on comment string confuses dep init parsing semver

[timjchin]

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

Go version
go1.7.4 darwin/amd64

Dep Version
v0.1.0-215-g911cd22

Description

Our existing Godeps.json has comments that aren't valid semver. For example:

		{
			"ImportPath": "github.com/Shopify/sarama",
			"Comment": "v1.11.0-44-g3efb95d",
			"Rev": "3efb95dad8fbcd194d3c06f7b9c40eabeb719b36"
		},

The comment has an extra -44-g3efb95d, which does not match a tag in Sarama. Dep seems to use the extra hash as the tag, which can lead to the version being not found.

Some lines from the output during dep init

  Using v1.11.0-44-g3efb95d as initial hint for imported dep github.com/Shopify/sarama
No versions of github.com/Shopify/sarama met constraints:
	v1.11.0-44-g3efb95d: unable to update repository:
	v1.12.0: Could not introduce github.com/Shopify/sarama@v1.12.0, as it is not allowed by constraint v1.11.0-44-g3efb95d from project stash.in.ionicsecurity.com/ip/gyre.
	v1.11.0: Could not introduce github.com/Shopify/sarama@v1.11.0, as it is not allowed by constraint v1.11.0-44-g3efb95d from project stash.in.ionicsecurity.com/ip/gyre.
	v1.10.1: Could not introduce github.com/Shopify/sarama@v1.10.1, as it is not allowed by constraint v1.11.0-44-g3efb95d from project stash.in.ionicsecurity.com/ip/gyre.
	v1.10.0: Could not introduce github.com/Shopify/sarama@v1.10.0, as it is not allowed by constraint v1.11.0-44-g3efb95d from project stash.in.ionicsecurity.com/ip/gyre.
	v1.9.0: Could not introduce github.com/Shopify/sarama@v1.9.0, as it is not allowed by constraint v1.11.0-44-g3efb95d from project stash.in.ionicsecurity.com/ip/gyre.
	v1.8.0: Could not introduce github.com/Shopify/sarama@v1.8.0, as it is not allowed by constraint v1.11.0-44-g3efb95d from project stash.in.ionicsecurity.com/ip/gyre.
	v1.7.0: Could not introduce github.com/Shopify/sarama@v1.7.0, as it is not allowed by constraint v1.11.0-44-g3efb95d from project stash.in.ionicsecurity.com/ip/gyre.
	v1.6.1: Could not introduce github.com/Shopify/sarama@v1.6.1, as it is not allowed by constraint v1.11.0-44-g3efb95d from project stash.in.ionicsecurity.com/ip/gyre.
	v1.6.0: Could not introduce github.com/Shopify/sarama@v1.6.0, as it is not allowed by constraint v1.11.0-44-g3efb95d from project stash.in.ionicsecurity.com/ip/gyre.
	v1.5.0: Could not introduce github.com/Shopify/sarama@v1.5.0, as it is not allowed by constraint v1.11.0-44-g3efb95d from project stash.in.ionicsecurity.com/ip/gyre.
	v1.4.3: Could not introduce github.com/Shopify/sarama@v1.4.3, as it is not allowed by constraint v1.11.0-44-g3efb95d from project stash.in.ionicsecurity.com/ip/gyre.
	v1.4.2: Could not introduce github.com/Shopify/sarama@v1.4.2, as it is not allowed by constraint v1.11.0-44-g3efb95d from project stash.in.ionicsecurity.com/ip/gyre.
	v1.4.1: Could not introduce github.com/Shopify/sarama@v1.4.1, as it is not allowed by constraint v1.11.0-44-g3efb95d from project stash.in.ionicsecurity.com/ip/gyre.
	v1.4.0: Could not introduce github.com/Shopify/sarama@v1.4.0, as it is not allowed by constraint v1.11.0-44-g3efb95d from project stash.in.ionicsecurity.com/ip/gyre.
	v1.3.0: Could not introduce github.com/Shopify/sarama@v1.3.0, as it is not allowed by constraint v1.11.0-44-g3efb95d from project stash.in.ionicsecurity.com/ip/gyre.
	v1.2.0: Could not introduce github.com/Shopify/sarama@v1.2.0, as it is not allowed by constraint v1.11.0-44-g3efb95d from project stash.in.ionicsecurity.com/ip/gyre.
	v1.1.0: Could not introduce github.com/Shopify/sarama@v1.1.0, as it is not allowed by constraint v1.11.0-44-g3efb95d from project stash.in.ionicsecurity.com/ip/gyre.
	v1.0.0: Could not introduce github.com/Shopify/sarama@v1.0.0, as it is not allowed by constraint v1.11.0-44-g3efb95d from project stash.in.ionicsecurity.com/ip/gyre.
	master: Could not introduce github.com/Shopify/sarama@master, as it is not allowed by constraint v1.11.0-44-g3efb95d from project stash.in.ionicsecurity.com/ip/gyre.
	gh-pages: Could not introduce github.com/Shopify/sarama@gh-pages, as it is not allowed by constraint v1.11.0-44-g3efb95d from project stash.in.ionicsecurity.com/ip/gyre.
	http_sse_example: Could not introduce github.com/Shopify/sarama@http_sse_example, as it is not allowed by constraint v1.11.0-44-g3efb95d from project stash.in.ionicsecurity.com/ip/gyre.
	producer-partition-buffer: Could not introduce github.com/Shopify/sarama@producer-partition-buffer, as it is not allowed by constraint v1.11.0-44-g3efb95d from project stash.in.ionicsecurity.com/ip/gyre.
	server: Could not introduce github.com/Shopify/sarama@server, as it is not allowed by constraint v1.11.0-44-g3efb95d from project stash.in.ionicsecurity.com/ip/gyre.

Deleting the hash manually from the comment does allow dep to find the correct version.

Here's a few more examples from our Godeps.json, some values from the commentdon't reference semver (and still have the extra hash):

		{
			"ImportPath": "github.com/gocql/gocql",
			"Comment": "pre-node-events-365-g58b3c71",
			"Rev": "58b3c7120a7ef9a077e4d089dd1badcb9ec756a8"
		},
		{
			"ImportPath": "github.com/kr/pretty",
			"Comment": "go.weekly.2011-12-22-29-gadd1dbc",
			"Rev": "add1dbc86daf0f983cd4a48ceb39deb95c729b67"
		},
		{
			"ImportPath": "github.com/lib/pq",
			"Comment": "go1.0-cutoff-127-gd8eeeb8",
			"Rev": "d8eeeb8bae8896dd8e1b7e514ab0d396c4f12a1b"
		},
		{
			"ImportPath": "gopkg.in/mgo.v2/bson",
			"Comment": "r2016.08.01-5-g3f83fa5",
			"Rev": "3f83fa5005286a7fe593b055f0d7771a7dce4655"
		},

It looks like Godeps uses git describe --tags for the comment field?
https://github.com/tools/godep/blob/3c0ccb9a2415fbda40b982b622735906bcd1760f/godepfile.go#L142
https://github.com/tools/godep/blob/3c0ccb9a2415fbda40b982b622735906bcd1760f/vcs.go#L43

Running git describe --tags on sarama, I see: v1.11.0-44-g3efb95d

[darkowlzz]

We have a good-first-bug here 😃.

godep importer should parse the Comment and try to evaluate if the first part of the string is a valid semver.

• If it's a semver, use the semver part as a version constraint.
• If it's not a semver, use the string as a branch constraint.

For example: if Comment value is v1.6.27-3-g0876266. Check if the first part v1.6.27 is a valid semver. If it is, use v1.6.27 to infer constraint. Else use the whole comment value to infer constraint.

Refer: https://github.com/golang/dep/blob/master/cmd/dep/godep_importer.go#L141

[aschlesener]

Can I tackle this one for my first contribution?

[darkowlzz]

@aschlesener that would be awesome 🎉
Let us know if you need any help 😊

[sebdah]

I'm looking at this for fun and have one question. Would appreciate if someone could kick me in the right direction.

It seems like the issue is that the version v1.6.27-3-g0876266 is getting caught by the bzr check we have in source_manager.go#L482. If I change that line to:

if strings.Count(s, "-") >= 2 && strings.Contains(s, "@") { (still need to read up on whether this is correct for all cases for bzr.

When the above check is in place, semver is returning 1.6.27-3-g0876266, which should be a fine semantic version.

My question is if having the following output in dep is correct or not:

[[constraint]]
  name = "github.com/Shopify/sarama"
  version = ">=1.12.0, <=12.0.0-g2fd980e"

It looks a bit funny, but is according to semver correct semantics.

[sdboyer]

@aschlesener indeed, feel free to ask away!

@sebdah ohhh, INTERESTING. i had a feeling that bzr logic might come back to bite us.

Yes, that range should be logically valid, though it may not have quite the desired effect. It's valid semver - -3-g0876266 is treated as a prerelease suffix - but the spec states that prereleases with that shape are ordered lexicographically. So, unless Shopify is releasing versions of sarama that follow that ordering rule, you may not be getting quite what you want with that upper range bound.

[sebdah]

@sdboyer Thanks for the swift response. In my opinion that would be outside the control of dep if Shopify is not following that rule. It seems worse to drop the suffix, since that too could point at the wrong package version.

I'm also considering having a regexp instead to validate bzr versions. Might be safer that the current approach. But will read up more on how their versioning works.

Would you be fine with the below as the end result, or is there something smarter we can do here that I'm overlooking?

[[constraint]]
  name = "github.com/Shopify/sarama"
  version = ">=1.12.0, <=12.0.0-g2fd980e"

[sdboyer]

In my opinion that would be outside the control of dep if Shopify is not following that rule.

💯

I'm also considering having a regexp instead to validate bzr versions. Might be safer that the current approach. But will read up more on how their versioning works.

Check the git blame on that line - I remember that, in the PR where we added that logic, one of our contributors actually went and read the bzr source code to find out how it generates the identifiers.

The right solution here also might be reordering when we check each constraint type. If we look at revisions last, this problem might just go away?

[sebdah]

Thanks for the pointers, will 🔍

[sdboyer]

Would you be fine with the below as the end result, or is there something smarter we can do here that I'm overlooking?

and, without having looked at sarama's release list, that seems like it would probably be fine. i don't know exactly what your goal is, though, so i'm not sure what the objective function here is by which we'd evaluate "smarter".