cmake: thrift requires boost headers, include them as Boost_INCLUDE_DIRS #1100
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1100 +/- ##
=======================================
Coverage 93.29% 93.29%
=======================================
Files 174 174
Lines 6404 6404
=======================================
Hits 5974 5974
Misses 430 430 |
|
Why do we need to bring FindBoost.cmake? Shouldn't this be coming by default with cmake installation: $ dpkg -L cmake-data | grep -i boost
/usr/share/cmake-3.16/Help/module/FindBoost.rst
/usr/share/cmake-3.16/Modules/FindBoost.cmake
$ |
Indeed this should be packaged with cmake, I will test and update the PR soon, thanks for the review! |
|
@lalitb amended, thanks! |
CMakeLists.txt
Outdated
| if(NOT Thrift_FOUND) | ||
| if(Thrift_FOUND) | ||
| find_package(Boost REQUIRED) | ||
| include_directories(${Boost_INCLUDE_DIRS}) |
There was a problem hiding this comment.
@ideepika - Sorry for replying late on this. Wondering why do we need to explicitly add the Boost headers? Jaeger build is successful in our CI pipeline even without this.
There was a problem hiding this comment.
@ideepika - Do you have any updates here ? Thanks.
There was a problem hiding this comment.
@lalitb sorry I must have missed the notification, I will check and post update tomorrow, thanks!
Signed-off-by: Deepika Upadhyay <[email protected]>
|
In jaeger we had thrift headers which needed boost headers and since we were using external built boost, we needed to define path for Boost Headers in cmake, that should not be an issue when using system installed boost package. |
@ideepika would it be possible to try setting |
thanks will wait for the update. I thought boost is only the build-time dependency for thrift, and not included in their API headers. Probably it's not the case. |
|
I did some testing:
https://shaman.ceph.com/builds/ceph/wip-test-deepika-jaeger/f7c3fcb7357b247541e5e319c7db426cada91ea0/jaeger/293825/ |
thrift has boost as their header dependency; when building with self compiled boost,
we will end up failing because of missing location for boost headers:
/usr/bin/ar qc ../../lib/liblibrados_impl.a CMakeFiles/librados_impl.dir/IoCtxImpl.cc.o CMakeFiles/librados_impl.dir/RadosXattrIter.cc.o CMakeFiles/librados_impl.dir/RadosClient.cc.o CMakeFiles/librados_impl.dir/librados_util.cc.o CMakeFiles/librados_impl.dir/librados_tp.cc.o
In file included from /usr/include/thrift/transport/TTransport.h:24,
from /usr/include/thrift/protocol/TProtocol.h:28,
from /usr/include/thrift/TProcessor.h:24,
from /usr/include/thrift/TDispatchProcessor.h:22,
from /build/ceph-17.0.0-10478-gd6aaf1fa/src/opentelemetry-cpp/exporters/jaeger/thrift-gen/Agent.h:10,
from /build/ceph-17.0.0-10478-gd6aaf1fa/src/opentelemetry-cpp/exporters/jaeger/thrift-gen/Agent.cpp:7:
/usr/include/thrift/transport/TTransportException.h:23:10: fatal error: boost/numeric/conversion/cast.hpp: No such file or directory
to be more cautious, we use FindBoost.cmake variable Boost_INCLUDE_DIRS
specified so that boost installing path is available for opentelemetry
to consume.
Signed-off-by: Deepika Upadhyay <[email protected]>
We were using git fetch and an unofficial remote to fetch opentelemetry-cpp, as it needed additional patch to include boost headers, with the merge of open-telemetry/opentelemetry-cpp#1100 open-telemetry/opentelemetry-cpp#1020 we do not require to maintain our own patched version. This removes compile time fetch for opentelemetry-cpp instead use official opentelemetry-cpp lib as a submodule. Signed-off-by: Deepika Upadhyay <[email protected]>
We were using git fetch and an unofficial remote to fetch opentelemetry-cpp, as it needed additional patch to include boost headers, with the merge of open-telemetry/opentelemetry-cpp#1100 open-telemetry/opentelemetry-cpp#1020 we do not require to maintain our own patched version. Also, we were using git fetch as a temporary change, now that tracing is always on, it should be right time to adapt to using opentelemetry-cpp as a submodule. This removes compile time fetch for opentelemetry-cpp instead use official opentelemetry-cpp lib as a submodule. Signed-off-by: Deepika Upadhyay <[email protected]>
We were using git fetch and an unofficial remote to fetch opentelemetry-cpp, as it needed additional patch to include boost headers, with the merge of open-telemetry/opentelemetry-cpp#1100 open-telemetry/opentelemetry-cpp#1020 we do not require to maintain our own patched version. Also, we were using git fetch as a temporary change, now that tracing is always on, it should be right time to adapt to using opentelemetry-cpp as a submodule. This removes compile time fetch for opentelemetry-cpp instead use official opentelemetry-cpp lib as a submodule. Signed-off-by: Deepika Upadhyay <[email protected]>
We were using git fetch and an unofficial remote to fetch opentelemetry-cpp, as it needed additional patch to include boost headers, with the merge of open-telemetry/opentelemetry-cpp#1100 open-telemetry/opentelemetry-cpp#1020 we do not require to maintain our own patched version. Also, we were using git fetch as a temporary change, now that tracing is always on, it should be right time to adapt to using opentelemetry-cpp as a submodule. This removes compile time fetch for opentelemetry-cpp instead use official opentelemetry-cpp lib as a submodule. Signed-off-by: Deepika Upadhyay <[email protected]>
Signed-off-by: Deepika Upadhyay [email protected]
Fixes # (issue)
Changes
Please provide a brief description of the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes