-
Notifications
You must be signed in to change notification settings - Fork 126
Remove Thrift headers from Jaeger public headers #121
Comments
@sebastian-garofalo the main issue here is that |
After installing thrift it got solved, weird that the example app didn't need it. Thanks |
@isaachier - if you use hunter to install thrift, those headers aren't going to be available in the regularly searched include paths. is it possible to not include any of the thrift dependencies in jaeger's public headers? |
@rnburn if that were true, Travis would have had compilation failures. Hunter manipulates the search path to include the dependency on Thrift. As an aside, I could potentially limit Thrift headers to implementation files, but it might be a big rewrite. Not sure if it makes sense when the dependency on the Thrift library would still be necessary. |
@isaachier - you statically link everything into the jaeger library, so you don't need thrift after building jaeger. Lots of people don't use cmake to build their projects. In that case, the hunter include directories wouldn't be searched unless they explicitly add flags for them to their build system. But given that they're not needed outside the implementation files, why include them? if nothing else, it's going to make compilation slower to add unnecessary header files like that. |
Right I agree that might be a solvable issue. Will reopen and retitle this issue. |
Adresses jaegertracing#121 so that thrift is a private dependency of jaeger. Signed-off-by: Benoit De Backer <[email protected]>
Adresses #121 so that thrift is a private dependency of jaeger. Signed-off-by: Benoit De Backer <[email protected]>
Hi
The example app works, although one of the unit tests fails, but then when I try to use the library in my projects I get the following error.
So I tried to update the thrift submodule as you mention in the README but doesn't seems to be working fine, I get:
And the original error is still present.
Does anyone knows what is going on and how can I solve it?
Thanks
The text was updated successfully, but these errors were encountered: