Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

Jaeger build is broken #575

Closed
rakyll opened this issue Mar 13, 2018 · 5 comments
Closed

Jaeger build is broken #575

rakyll opened this issue Mar 13, 2018 · 5 comments

Comments

@rakyll
Copy link
Contributor

rakyll commented Mar 13, 2018

# go.opencensus.io/exporter/jaeger/internal/gen-go/jaeger
internal/gen-go/jaeger/jaeger.go:2146:13: not enough arguments in call to oprot.Flush
	have ()
	want (context.Context)
internal/gen-go/jaeger/jaeger.go:2163:14: not enough arguments in call to oprot.Flush
	have ()
	want (context.Context)
internal/gen-go/jaeger/jaeger.go:2176:14: not enough arguments in call to oprot.Flush
	have ()
	want (context.Context)
internal/gen-go/jaeger/jaeger.go:2190:23: not enough arguments in call to oprot.Flush
	have ()
	want (context.Context)
@h8liu
Copy link

h8liu commented Mar 13, 2018

this is due to thrift changing its API

Similar issue: go-kit/kit#677 and pull request: go-kit/kit#678

rakyll added a commit to rakyll/opencensus-go that referenced this issue Mar 13, 2018
Thrift needs to be installed from the source code.
See the https://thrift.apache.org/tutorial/go page
for more details.

Fixes census-instrumentation#575.
rakyll added a commit to rakyll/opencensus-go that referenced this issue Mar 13, 2018
Thrift needs to be installed from the source code.
See the https://thrift.apache.org/tutorial/go page
for more details.

Fixes census-instrumentation#575.
rakyll added a commit that referenced this issue Mar 13, 2018
Thrift needs to be installed from the source code.
See the https://thrift.apache.org/tutorial/go page
for more details.

Fixes #575.
@t-bast
Copy link

t-bast commented Mar 21, 2018

Hey guys, I'm having the issue the other way around:
../../vendor/go.opencensus.io/exporter/jaeger/internal/gen-go/jaeger/jaeger.go:2146:13: too many arguments in call to oprot.Flush
have (context.Context)
want ()
../../vendor/go.opencensus.io/exporter/jaeger/internal/gen-go/jaeger/jaeger.go:2163:14: too many arguments in call to oprot.Flush
have (context.Context)
want ()
../../vendor/go.opencensus.io/exporter/jaeger/internal/gen-go/jaeger/jaeger.go:2176:14: too many arguments in call to oprot.Flush
have (context.Context)
want ()
../../vendor/go.opencensus.io/exporter/jaeger/internal/gen-go/jaeger/jaeger.go:2190:23: too many arguments in call to oprot.Flush
have (context.Context)
want ()

I'm using dep, it pulled go.opencensus.io 0.5.0 and git.apache.org/thrift.git 0.11.0 (which is thrift's latest stable release) so I'm a bit surprised...
Am I supposed to instead target thrift's master branch to get this to work? If so why did you unpin from their stable release?

EDIT: indeed I found the thrift commit that changed the API, so it looks like we are pinned to thrift master instead of their 0.11.0 if others experience the same issue. I guess some features that weren't in 0.11.0 were needed for the jaeger exporter right?

@rakyll
Copy link
Contributor Author

rakyll commented Mar 21, 2018

Am I supposed to instead target thrift's master branch to get this to work? If so why did you unpin from their stable release?

In Go, it is illegal for libraries to pin to a version and vendor it at this point. We don't vendor libraries and cannot vendor them because go tool fails to handle library vendored version of a dependency vs application binary vendored dependency.

We can use dep to pin our dependencies but the library is going to misbehave for anyone who is not using dep.

I will think about how we can handle this issue. We probably can just vendor thrift because it is internally used by the jaeger package.

@t-bast
Copy link

t-bast commented Mar 22, 2018

This is a heated debate in the Go community :)

I really like the fact that Go tries to encourage no vendoring/no versions, because with the right amount of care it allows greater velocity. Unfortunately, I believe most open-source projects/libraries currently don't care enough about API stability and backwards-compatibility on their master branch, so the "no vendoring" concept makes life quite painful...

It probably works well internally to Google or organisations with a high level of code quality, but most open-source Go projects I've seen end up using a vendoring mechanism like glide, dep or vgo (and I currently do too).

But I totally understand why you currently don't vendor, especially since vgo hasn't shipped yet and dep isn't the official recommendation anymore. It's not an issue for me (and others) to point to the master branch, and I hope my comment here will help unblock other dep users who see the build fail in their project.

@h8liu
Copy link

h8liu commented Mar 22, 2018

(blatant ad here) Alternatively, you can sync to my smallrepo service:

https://smallrepo.com/

We maintain a monorepo (of a collections of repos) that is always buildable, and will block (and fix) these kind of backward incompatible changes for you.

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

No branches or pull requests

3 participants