Patches to enable Visual Studio 2015 - Update 2 support #565
Conversation
Codecov Report
@@ Coverage Diff @@
## main #565 +/- ##
==========================================
+ Coverage 94.39% 94.43% +0.03%
==========================================
Files 200 200
Lines 9067 9068 +1
==========================================
+ Hits 8559 8563 +4
+ Misses 508 505 -3
|
| if (base_ != nullptr) | ||
| { | ||
| std::copy(base_, base_ + old_size, temp); | ||
| // vs2015 does not like this construct considering it unsafe: |
There was a problem hiding this comment.
Check Visual Studio 2015 compiler like via _MSC_VER to separate the workaround for it?
There was a problem hiding this comment.
Does std::copy itself perform a bounds checking? i.e. what if new temp size - new_capacity is less than the old capacity? Since it's a raw pointer, I think the old code had a problem: it was copying beyond the allocated buffer, buffer overflow.
| This code is required to compile OpenTelemetry code in vs2015 because MPark Variant implementation is not compatible with vs2015. | ||
|
|
||
| Build option `HAVE_ABSEIL_VARIANT` allows to enable the build with `absl::variant`, `absl::get` and `absl::visit` as defalt implementation for `nostd::` classes. |
There was a problem hiding this comment.
Is there a reason we have to ship two variant implementations? If absl is needed for VS2015 (and we decide we want to support that), can't we just generally use the absl implementation (instead of the MPark one)?
There was a problem hiding this comment.
I'd like to take a two-staged approach. We should use Abseil everywhere, but I'd like to keep it under the build option right now until I fully test all existing exporters with it. Within the same OS class the ABI compatibility is retained irrespective of what implementation we take, e.g. Mpark works for Linux and Mac no problem. Whereas Abseil works for Windows. I want to send a follow-up PR that adds a build loop for Windows to switch over to Abseil for Variant. Then we can do this for Linux and Mac.
There was a problem hiding this comment.
I think long-term before v1.0 GA we should switch to Abseil for variant. That'd be great for another reason: we can get rid of Boost license in our list of licenses because only MPark variant is Boost-licensed, whereas Abseil is also Apache License v2.0 as the rest of code. But I'd like to do this switch in a separate PR.
There was a problem hiding this comment.
Makes sense. Can you add an additional issue for this (apart from the license one), otherwise this might be lost when just looking at issue titles.
I'd be fine with switching to absl as default, having MPark under a switch (then our CI pipeline would verify how it's working). However, I assume you expect changes to be necessary for it to work on Linux.
pyohannes
left a comment
pyohannes
left a comment
There was a problem hiding this comment.
Approved, provided we work toward getting rid of the MPark variant implementation.
Fixes #314 . It does not yet add the build system for supporting vs2015 builds in CI, as it is quite involved (there is no GitHub Actions runner with preinstalled vs2015 and installing one may need some tweaking, and IS SLOW.. unless we cache some docker container image for that). Best we can do is deploy additional vs2015 community build tools. I'll cover CI for vs2015 in a separate PR. I verified the code locally with ETW exporter on Windows with Visual Studio 2015 - Update 2. It compiles and works well.
Changes
MPark variant that we use in the SDK is NOT compiling with Visual Studio 2015. Abseil Variant is compiling and working well with Visual Studio 2015 Update 2+. This visual studio version requirement is coming from Abseil itself, but it's a reasonable requirement. The solution is to add a build option
HAVE_ABSEIL_VARIANTto compile with that one instead of MPark variant.Visual Studio 2015 secure development practices does not like
std::copyand breaks the build. The fix is to replace thestd::copywith bounds-checked implementation that usesfor-loopVisual Studio 2015 does not like passing context by value. There is a build error related to alignment. Passing by const reference works.
I manually verified that the code compiles and works in Visual Studio 2015 with ETW exporter. I will cover full CI and other exporters (OTLP, Elastic, etc.) in a follow-up PR that would add Visual Studio 2015 to build loop.