-
Notifications
You must be signed in to change notification settings - Fork 440
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
[BUILD] CMakeLists.txt: Enable CMAKE_MSVC_RUNTIME_LIBRARY support #2652
Conversation
The documentation for CMAKE_MSVC_RUNTIME_LIBRARY states [1]: > This variable has effect only when policy CMP0091 is set to NEW prior to > the first project() or enable_language() command that enables a language > using a compiler targeting the MSVC ABI. so the current usage of CMAKE_MSVC_RUNTIME_LIBRARY for vcpkg does not work at all. Let's fix that by setting policy 91 to new if present. [1]: https://cmake.org/cmake/help/latest/variable/CMAKE_MSVC_RUNTIME_LIBRARY.html
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.
Sorry, removing the approval for the CI failure. Probably we should set the policy only if vcpkg is used (i.e, VCPKG_TOOLCHAIN is set)? cc @ThomsonTan
My usecase is not related to vcpkg, I'm just compiling with MSVC. |
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.
Thanks for the PR.
This change is probably not sufficient, as it triggers build failures in CI.
Please investigate and fix errors such as:
benchmark.lib(benchmark.cc.obj) : error LNK2038: mismatch detected for 'RuntimeLibrary': value 'MDd_DynamicDebug' doesn't match value 'MTd_StaticDebug' in attributes_hashmap_benchmark.obj [D:\a\opentelemetry-cpp\opentelemetry-cpp\build\sdk\test\metrics\attributes_hashmap_benchmark.vcxproj]
Thanks for the review, will do. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2652 +/- ##
==========================================
+ Coverage 87.12% 87.68% +0.56%
==========================================
Files 200 190 -10
Lines 6109 5851 -258
==========================================
- Hits 5322 5130 -192
+ Misses 787 721 -66 |
The CI should have been fixed by #2696. |
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.
CMakeLists.txt: Enable CMAKE_MSVC_RUNTIME_LIBRARY support (open-telemetry#2652)
The documentation for CMAKE_MSVC_RUNTIME_LIBRARY states 1:
so the current usage of CMAKE_MSVC_RUNTIME_LIBRARY for vcpkg does not work at all.
Let's fix that by setting policy 91 to new if present.