-
Notifications
You must be signed in to change notification settings - Fork 516
[WIP] Adjust the standards for proto target to align with those of Protocol Buffers. #3800
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull request overview
This PR adds CI validation to ensure OpenTelemetry C++ can be built with a different C++ standard than the protobuf dependency it uses, addressing issue #3799 where MSVC linking errors occurred when protobuf was compiled with C++17 but OpenTelemetry was built with C++20.
Key Changes:
- Adds new
cmake.different_std.testCI target for both Linux (GCC) and Windows (MSVC) platforms - Linux GCC test installs dependencies with C++17, then builds OpenTelemetry with C++23
- Windows MSVC test validates similar scenario with C++20 for OpenTelemetry
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| ci/do_ci.sh | Adds new cmake.different_std.test target for Linux/GCC that builds with C++23 STL and enables OTLP file support |
| ci/do_ci.ps1 | Adds new cmake.different_std.test target for Windows/MSVC that builds with C++20 STL, maintainer mode, and OTLP file support |
| .github/workflows/ci.yml | Adds two new CI jobs: one for GCC (installs protobuf with C++17, builds with C++23) and one for MSVC (tests different C++ standards) |
| cmake "${CMAKE_OPTIONS[@]}" \ | ||
| -DWITH_METRICS_EXEMPLAR_PREVIEW=ON \ | ||
| -DCMAKE_CXX_FLAGS="-Werror $CXXFLAGS" \ | ||
| -DWITH_ASYNC_EXPORT_PREVIEW=ON \ |
Copilot
AI
Jan 6, 2026
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.
The PowerShell script sets DWITH_STL=CXX20 (line 278) and also explicitly sets DCMAKE_CXX_STANDARD=20 (line 279). However, the bash script only sets DWITH_STL=CXX23 (line 343) without explicitly setting CMAKE_CXX_STANDARD. This inconsistency could lead to different behaviors. Consider either adding CMAKE_CXX_STANDARD to the bash script or removing it from the PowerShell script to maintain consistency, or align both to use the same C++ standard version.
| -DWITH_ASYNC_EXPORT_PREVIEW=ON \ | |
| -DWITH_ASYNC_EXPORT_PREVIEW=ON \ | |
| -DCMAKE_CXX_STANDARD=23 \ |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3800 +/- ##
=======================================
Coverage 89.93% 89.93%
=======================================
Files 225 225
Lines 7163 7163
=======================================
Hits 6441 6441
Misses 722 722 🚀 New features to boost your workflow:
|
Fixes cmake args Restore windows version
159c5b0 to
626d7e2
Compare
add --build-shared-libs "ON"
5791943 to
4859e4e
Compare
Fixes the compatibility for old version of protobuf with C++14 used Fixes DEFINED CMAKE_CXX_STANDARD checking
9afd979 to
00ca2ea
Compare
Fixes #3799
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