-
Notifications
You must be signed in to change notification settings - Fork 500
[CONFIGURATION] File configuration - cmake build #3655
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3655 +/- ##
==========================================
- Coverage 90.15% 90.11% -0.04%
==========================================
Files 221 221
Lines 7135 7135
==========================================
- Hits 6432 6429 -3
- Misses 703 706 +3 🚀 New features to boost your workflow:
|
Thanks for the PR. Excited to get this building :). I left some comments and mainly interested in getting the ryml dependency managed like the others (git tag files to define the version, FetchContent support, and install with the common third party CMake project instead of the bash script). Seems reasonable to follow up with these before the next release if it's too much change in this PR. |
Thanks for all the comments. Most fixed, the cmake install tests are deferred to a follow up PR. I propose to merge this part now, it is self contained and a good size fix already, no need to make it bigger. |
set(ryml_PROVIDER "fetch_repository") | ||
|
||
set(RYML_ENABLE_TESTING OFF CACHE BOOL "" FORCE) | ||
set(RYML_ENABLE_INSTALL OFF CACHE BOOL "" FORCE) |
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.
For the follow up PR with install tests we should also change this to
set(RYML_ENABLE_INSTALL ${OPENTELEMETRY_INSTALL} CACHE BOOL "" FORCE)
This should allow the install to complete if ryml is fetched.
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.
Looks good. Thanks! Noting some potential follow up PRs.
- replace
add_definitions
withtarget_compile_definitions
in the example and elsewhere - Update ryml.cmake to install ryml if
OPENTELEMETRY_INSTALL
is ON - Test the new yaml example with the install
- Consider breaking up the opentelemetry-cpp::configuration target into
opentelemetry-cpp::configuration_common
and aopentelemetry-cpp::configuration_ryml
target that brings in the third party dependency.
Contributes to #2481
This is a partial fix, to implement the CMake build.
Changes
Please provide a brief description of the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes