-
Notifications
You must be signed in to change notification settings - Fork 512
[CMAKE] update cmake files in examples directory #3421
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CMAKE] update cmake files in examples directory #3421
Conversation
✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3421 +/- ##
==========================================
+ Coverage 90.04% 90.05% +0.02%
==========================================
Files 212 212
Lines 6932 6932
==========================================
+ Hits 6241 6242 +1
+ Misses 691 690 -1 🚀 New features to boost your workflow:
|
…arget. Update the dll component install test to rely on the tranisitive include directories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
BTW: Not a problem of this PR, but should OPENTELEMETRY_BUILD_IMPORT_DLL be interface definition of opentelemetry_cpp in ext/src/dll/CMakeLists.txt)? @ThomsonTan
marcalff
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the fix
Yes, I think this is good suggestion. Opened below issue for tracking. |
Fixes #3399
Updates the examples' cmake files to use the read-only ALIAS targets of
opentelemetry-cppadded by #3368 and to use explicitly scoped target properties.Changes
opentelemetry-cppALIAS targets (ie:opentelemetry-cpp::apiin place ofopentelemetry_api)target_compile_definitionsin place ofadd_definitionstarget_include_directoriesin place ofinclude_directoriesPRIVATE,PUBLIC) totarget_link_librariesPUBLICtarget properties (definitions, linked libraries, include directories)CMAKE_THREAD_LIBS_INITwith theThreads::Threadstarget where needed, otherwise remove it and rely on theopentelemetry-cpp::commontarget's public transitive link to Threads.opentelemetry-cpp::opentelemetry_cpp) to include the directories for headers it depends on. Update theext_dllcomponent install test to verify.For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes