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

timjchin · 2017-07-15T20:00:45Z

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

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

darkowlzz · 2017-07-15T21:14:50Z

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 · 2017-07-16T01:48:34Z

Can I tackle this one for my first contribution?

darkowlzz · 2017-07-16T05:59:45Z

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

sebdah · 2017-07-20T03:23:52Z

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 · 2017-07-20T03:53:03Z

@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 · 2017-07-20T04:01:29Z

@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 · 2017-07-20T04:05:38Z

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 · 2017-07-20T04:06:31Z

Thanks for the pointers, will 🔍

sdboyer · 2017-07-20T04:07:12Z

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".

This commit addresses an issue where the godep importer would confuse semver suffixes for being bzr revisions. Now we have stricter checks on the bzr revision checks which will be able to distinguish between semver with a suffix and a bzr revision. The new check enforces bzr revisions to contain an @ symbol. Closes golang#827.

timjchin changed the title ~~Godeps.json with extra hash on comment string confuses dep init~~ Godeps.json with extra hash on comment string confuses dep init parsing semver Jul 15, 2017

timjchin mentioned this issue Jul 15, 2017

Dep init Hangs #823

Closed

darkowlzz added area: init bug labels Jul 15, 2017

darkowlzz added the good first issue label Jul 15, 2017

sebdah mentioned this issue Jul 21, 2017

Address mixup between bzr revisions and semver suffixes #869

Merged

carolynvs closed this as completed in #869 Jul 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

timjchin commented Jul 15, 2017 •

edited

Loading

darkowlzz commented Jul 15, 2017

aschlesener commented Jul 16, 2017

darkowlzz commented Jul 16, 2017

sebdah commented Jul 20, 2017 •

edited

Loading

sdboyer commented Jul 20, 2017

sebdah commented Jul 20, 2017

sdboyer commented Jul 20, 2017

sebdah commented Jul 20, 2017

sdboyer commented Jul 20, 2017

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

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

Comments

timjchin commented Jul 15, 2017 • edited Loading

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

Description

darkowlzz commented Jul 15, 2017

aschlesener commented Jul 16, 2017

darkowlzz commented Jul 16, 2017

sebdah commented Jul 20, 2017 • edited Loading

sdboyer commented Jul 20, 2017

sebdah commented Jul 20, 2017

sdboyer commented Jul 20, 2017

sebdah commented Jul 20, 2017

sdboyer commented Jul 20, 2017

timjchin commented Jul 15, 2017 •

edited

Loading

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

sebdah commented Jul 20, 2017 •

edited

Loading