isaachier
commented
6 years ago Sorry about this. I agree overall I need to beef up the docs here (see #33). Do you have an issue using Hunter or you just want to avoid a tool that isn't used in Postgres?
Closed ringerc closed 2 years ago
isaachier
commented
6 years ago Sorry about this. I agree overall I need to beef up the docs here (see #33). Do you have an issue using Hunter or you just want to avoid a tool that isn't used in Postgres?
ringerc
commented
6 years ago I can't use Hunter.
If I even try to suggest adding support for downloading random things off the Internet in the PostgreSQL build system I'll be run out of town. There's increasing work being done for reproducible builds, and offline builds are a hard non-negotiable requirement for many if not most PostgreSQL users and deployments. Package builders would have a heart attack; you're not likely to see the Jaeger cpp agent landing up in Fedora, Debian/Ubuntu, etc without an offline build that can use locally installed dependencies.
Not to mention that it's autotools based still (alas), which probably makes using Hunter "fun" even if it were an option.
I'm trying to do what I can with limited time because I'm really excited by what opentracing and jeager looks like it can offer. But I bet the great majority would've already gone "too hard basket". Maybe I'm just a reactionary old fogie, though, after all I work with a community that still requires Perl 5.8.8 support in its test harness 🤮.
I wrote you a draft README anyway, to try to record what I've figured out so far or had pointed out to me on the chat channel. You'll see it on #33.
The above separate commit https://github.com/ringerc/cpp-client/commit/f6ebf97eeab5c99f48e02a375674ecaa905a42e0 adds support for local package finding using typical CMake find package modules. It's not quite there yet:
thrift::thrift_static despite:
if(HUNTER_ENABLED)
list(APPEND LIBS thrift::thrift_static)
else()
list(APPEND LIBS ${THRIFT_LIBRARIES})
endif()and the fact that cmake/Findthrift.cmake just does FIND_LIBRARY(THRIFT_LIBRARIES thrift). Where's thrift_static coming from?
Haven't sent you a PR due to the above.
ringerc
commented
6 years ago @isaachier Forgot to @ mention you, see above
ringerc
commented
6 years ago Figured out what was wrong with the test builds, fixed.
Now having difficulty because the source tree seems to contain generated files from Apache Thrift. But:
Thrift expects/advises that you use a matching Thrift compiler and Thrift library version, and jaeger-cpp does nothing to ensure that's the case; and
There doesn't seem to be any tooling in the cmake build to regenerate them. I don't see where they came from in the sources, for that matter; can they be regenerated?
Without that I don't see how I can compile it.
The specific error is:
/home/craig/projects/2Q/opentracing-jaeger-cpp-client/src/jaegertracing/thrift-gen/Agent.cpp: In member function ‘uint32_t jaegertracing::agent::thrift::Agent_emitZipkinBatch_args::write(apache::thrift::protocol::TProtocol*) const’:
/home/craig/projects/2Q/opentracing-jaeger-cpp-client/src/jaegertracing/thrift-gen/Agent.cpp:70:10: error: ‘class apache::thrift::protocol::TProtocol’ has no member named ‘incrementRecursionDepth’
oprot->incrementRecursionDepth();
^~~~~~~~~~~~~~~~~~~~~~~
/home/craig/projects/2Q/opentracing-jaeger-cpp-client/src/jaegertracing/thrift-gen/Agent.cpp:87:10: error: ‘class apache::thrift::protocol::TProtocol’ has no member named ‘decrementRecursionDepth’
oprot->decrementRecursionDepth();
^~~~~~~~~~~~~~~~~~~~~~~and it's because my local Thrift is 0.9.1, but the Jaeger files were generated with 0.9.2.
Where are the inputs for the Thrift generator?
isaachier
commented
6 years ago I can help with the non-Hunter build. Sorry you are having so many issues with that.
jmprusi
commented
6 years ago I'm looking into building the jaeger-client-cpp in an air gap system, do you have some documentation around this point? are you planning on merging your fork into upstream? I can try to vendor Hunter dependencies, but not really elegant.
isaachier
commented
6 years ago I do owe this repo more documentation, although I have plans for an overhaul of this project soon. As of now, there are no plans to merge @ringerc's work upstream (it would be outdated by now anyway). He has helped me recognize this issue, and I appreciate the effort, but it won't be possible right now.
Regarding your issues building, I believe this is better addressed in the Hunter documentation. Look at the HUNTER_ENABLED variable and the git submodule solution described. Hopefully, that will help you build with your set-up. For questions about Hunter, you can also visit the Hunter chat on https://gitter.im.
Sorry about the lack of documentation and I hope this helps you.
ringerc
commented
6 years ago I wrote some docs on it, and some patches, but they aren't merged and I can't maintain them if they won't be merged upstream. The case for merging them is weakened by the fact that:
so it's not worth it.
Instead you might want to look at using the opentracing-c++ API and its dynamic loading support, then load a jaeger-cpp that's been compiled to statically link the needed libs, so it's just a simple .so . So long as the C++ ABI is compatible you will be fine. It's totally unacceptable for distribution packaging, and really bad for continuous integration workflows, reproducible builds, etc, but it'll likely meet your needs.
When I get the chance to resume this, I'll be adding a C-adapter over the opentracing c++ API, and using its dynloader. But I'm waiting on a patch for opentracing c++ API to make span contexts copyable; see https://github.com/opentracing/opentracing-cpp/issues/74 . And I have no time presently due to Other Work Stuff.
So I don't plan on working further on making jaeger-cpp offline-buildable, reproducible-buildable, or able to use distro dependencies. Someone else can if they want, but I don't think it's viable unless the Thrift bug is fixed at least. (Even if you changed Jaeger to apply a patch on top of the generated output, the patch has to differ depending on the underlying Thrift version, so you don't gain much).
This isn't any complaint at @isaachier btw, they've been super helpful and given their time on things that aren't really their own priorities to help me out. Including even working on a c-client. It's just how it is. I don't see any good answers without major changes including to Jaeger IDL etc.
I haven't closed the issue, but it might as well be.
Jaeger's c++ bindings seem to leap out to the Internet to build private copies of build dependencies like Boost.
A bit of reading of the build code shows that
is sufficient to suppress this behaviour, at which point you can configure it with local dependencies. It'd be nice if this were in the
README.md.I got partway with:
then building the opentracing C++ API from https://github.com/opentracing/opentracing-cpp and installing that first.
It still gets stuck on the rpm packaging for json-devel (nlohmann) since there's no bundled nlohmann_jsonConfig.cmake . Seems it doesn't know how to do discovery for that, I'll see if I can add it. Same with the yaml libraries.
But even when I make configs fro nlohmann and json-devel and then
it still fails with
It could be related to version, but the CMake code doesn't do any tests for required versions and there's no documentation on required verisons, so I'm struggling to know what's required. I have:
Anyway, it'd be nice to document the dependencies and that building without "grab it all from the Internet" is supported in the README.md.
I'll experiment with it for now by letting Hunter do its thing, but I really don't want to do things like build Boost all the time...