Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tools pruned by dep #1260

Merged
merged 3 commits into from
Dec 23, 2018

Conversation

isaachier
Copy link
Contributor

Signed-off-by: Isaac Hier isaachier@gmail.com

Which problem is this PR solving?

Short description of the changes

  • Use required list to maintain tool dependencies.

Signed-off-by: Isaac Hier <isaachier@gmail.com>
@codecov
Copy link

codecov bot commented Dec 17, 2018

Codecov Report

Merging #1260 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1260   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         160     160           
  Lines        7181    7181           
======================================
  Hits         7181    7181

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc87c85...577ffad. Read the comment docs.

@isaachier
Copy link
Contributor Author

I think the install is fixed, but at least for me, make proto is broken.

@olivierboucher
Copy link

olivierboucher commented Dec 17, 2018

@isaachier Please look at the changes I made in #1214 . I managed to have make proto working.

@isaachier
Copy link
Contributor Author

Cool @olivierboucher will do thanks.

Signed-off-by: Isaac Hier <isaachier@gmail.com>
@isaachier
Copy link
Contributor Author

Actually, the only problem I had was because I was using protoc-3.0.0 when should have been using protoc >= 3.5.1. Works fine now.

@ghost ghost assigned yurishkuro Dec 23, 2018
@ghost ghost added the review label Dec 23, 2018
@yurishkuro
Copy link
Member

Actually, the only problem I had was because I was using protoc-3.0.0 when should have been using protoc >= 3.5.1. Works fine now.

what were the issues with older protoc?

Unfortunately, afaik protoc does not have a version command so that we could check it during the build.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Isaac!


[[constraint]]
name = "github.com/gogo/protobuf"
revision = "fd9a4790f3963525fb889cc00e0a8f828e0b3a29"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll merge it on green build, but we should figure out how to pin to versions instead of raw commit hash. E.g. gogo/googleapis has a 1.1.0 release in August, which is later than the commit we have. And gogo/protobuf has pretty frequent releases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should work with newer versions too. This is copied from the old glide.yaml.

@yurishkuro yurishkuro merged commit dc526dd into jaegertracing:master Dec 23, 2018
@ghost ghost removed the review label Dec 23, 2018
@isaachier
Copy link
Contributor Author

Glad to help! I saw the issue when I checked the gogo/protobuf repo and saw the install-protoc.sh script. It specifically downloads 3.5.1. The Ubuntu package I was using was pretty old in comparison.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants