open-telemetry / opentelemetry-cpp

The OpenTelemetry C++ Client
https://opentelemetry.io/
Apache License 2.0
850 stars 403 forks source link

[BUILD] CMakeLists.txt: Enable CMAKE_MSVC_RUNTIME_LIBRARY support #2652

Closed t-b closed 3 months ago

t-b commented 5 months ago

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.

t-b commented 5 months ago

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.

t-b commented 4 months ago

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[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 87.68%. Comparing base (497eaf4) to head (3ff8b97). Report is 76 commits behind head on main.

:exclamation: Current head 3ff8b97 differs from pull request most recent head 2566fa7

Please upload reports for the commit 2566fa7 to get more accurate results.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/2652/graphs/tree.svg?width=650&height=150&src=pr&token=FJESTYQ2AD&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry)](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/2652?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) ```diff @@ 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 ``` [see 74 files with indirect coverage changes](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/2652/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry)
ThomsonTan commented 3 months ago

The CI should have been fixed by https://github.com/open-telemetry/opentelemetry-cpp/pull/2696.