cmake: add Find<package>.cmake find module method for thrift #1020
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ideepika
changed the title
3 tasks
ThomsonTan
reviewed
Oct 19, 2021
exporters/jaeger/CMakeLists.txt
Outdated
| @@ -32,7 +32,7 @@ target_include_directories( | |||
| target_link_libraries( | |||
| opentelemetry_exporter_jaeger_trace | |||
| PUBLIC opentelemetry_resources http_client_curl | |||
| PRIVATE thrift::thrift) | |||
| INTERFACE thrift::thrift) | |||
There was a problem hiding this comment.
It is necessary to change the dependency to INTERFACE?
There was a problem hiding this comment.
still in testing phase, not seems to be a problem for now, I have removed this change, thanks!
Codecov Report
@@ Coverage Diff @@
## main #1020 +/- ##
=======================================
Coverage 94.85% 94.85%
=======================================
Files 151 151
Lines 5972 5972
=======================================
Hits 5664 5664
Misses 308 308 |
In some cases, when using jaeger-cpp-client, we use thrift provided from distro packages, these does not contain thriftConfig.cmake. For such scenarios, we pivot to do not make find_package CONFIG mode as default, instead we use basic signature[0]. We make use of `CMAKE_MODULE_PATH` to search for FindThrift.cmake, which will find and set all thrift::thrift's target properties. This appends cmake/modules dir to CMAKE_MODULE_PATH, which is used as all modules requiring find_package mode with basic_signature. [0] https://cmake.org/cmake/help/latest/command/find_package.html The command arguments determine which of the above modes is used. When the basic signature is used, the command searches in Module mode first. If the package is not found, the search falls back to Config mode. Signed-off-by: Deepika Upadhyay <dupadhya@redhat.com>
Out of curiosity, which are these distributions providing thrift package without cmake config files? |
Sure, I can help there, I think most does, you can check them here: https://pkgs.org/search/?q=thrift |
lalitb
approved these changes
Oct 21, 2021
ThomsonTan
approved these changes
Oct 21, 2021
14 tasks
ideepika
pushed a commit
to ideepika/ceph
that referenced
this pull request
Jun 22, 2022
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 <dupadhya@redhat.com>
ideepika
pushed a commit
to ideepika/ceph
that referenced
this pull request
Jun 22, 2022
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 <dupadhya@redhat.com>
ideepika
pushed a commit
to ceph/ceph-ci
that referenced
this pull request
Jun 27, 2022
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 <dupadhya@redhat.com>
Pegonzal
pushed a commit
to ceph/ceph
that referenced
this pull request
Oct 13, 2022
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 <dupadhya@redhat.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In some cases, when using jaeger-cpp-client, we use thrift provided from
distro packages, these does not contain thriftConfig.cmake.
For such scenarios, we pivot to do not make find_package CONFIG mode as
default, instead we use basic signature[0]. We make use of
CMAKE_MODULE_PATHto search for FindThrift.cmake, which will find andset all thrift::thrift's target properties.
[0] https://cmake.org/cmake/help/latest/command/find_package.html
The command arguments determine which of the above modes is used. When
the basic signature is used, the command searches in Module mode first.
If the package is not found, the search falls back to Config mode.
Signed-off-by: Deepika Upadhyay dupadhya@redhat.com
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