Skip to content

Fix build break in protobuf by pinning to older version in our local vcpkg port file#235

Merged
reyang merged 1 commit into
open-telemetry:masterfrom
maxgolov:maxgolov/vcpkg_fix
Jul 31, 2020
Merged

Fix build break in protobuf by pinning to older version in our local vcpkg port file#235
reyang merged 1 commit into
open-telemetry:masterfrom
maxgolov:maxgolov/vcpkg_fix

Conversation

@maxgolov
Copy link
Copy Markdown
Contributor

@maxgolov maxgolov commented Jul 31, 2020

Our current CI loop CI / CMake -> exporter proto is broken due to mainline vcpkg issue.

Solution is to save local copies of vcpkg port files in our repo. That way if mainline vcpkg breaks something in those packages, we're not affected.

Also adding port file for Google Benchmark to be compatible with both vs2017 and vs2019 compilers. So that one library built once can be shared between the two different compilers. Without that patch it is impossible to reuse the library across two different compilers: since vs2019 introduces some new exception handling function that is not available in older runtime - if you build Google Bechmark from vcpkg with vs2019, then try that prebuilt - it is not gonna link in vs2017! This is yet another "LOL" at ABI compat requirement, showing that not everything is controlled by stable class definition. Something we have to keep in mind -- the build flag for that "backwards-compat" mode in vs2019 is /d2FH4-. Described here:
https://devblogs.microsoft.com/cppblog/making-cpp-exception-handling-smaller-x64/
I added it to ci/ports/benchmark/portfile.cmake -

Alternate solution: we could start using .gitmodules and pin to older version of the mainline vcpkg repo that did not have this issue.

@maxgolov maxgolov requested a review from a team July 31, 2020 18:40
@maxgolov maxgolov self-assigned this Jul 31, 2020
@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 31, 2020

Codecov Report

Merging #235 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #235   +/-   ##
=======================================
  Coverage   94.22%   94.22%           
=======================================
  Files         115      115           
  Lines        3882     3882           
=======================================
  Hits         3658     3658           
  Misses        224      224           

Copy link
Copy Markdown
Contributor

@pyohannes pyohannes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for providing this fix!

@maxgolov
Copy link
Copy Markdown
Contributor Author

@reyang @pyohannes - I fixed the issue, but there's totally unrelated osx_test - Failed - nothing to deal with PowerShell / vcpkg fixes. I think this one should be good to merge.

@maxgolov maxgolov changed the title [WIP] Fix build break by pinning to our local port files Fix build break in protobuf by pinning to older version in our local vcpkg port file Jul 31, 2020
@maxgolov maxgolov requested review from g-easy and reyang July 31, 2020 20:08
@pyohannes pyohannes added the pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.) label Jul 31, 2020
@pyohannes
Copy link
Copy Markdown
Contributor

I fixed the issue, but there's totally unrelated osx_test - Failed

That's a Circle CI tests, I think we should get rid of those anyway. Maybe it's just some flakiness? All the GitHub Actions test pass, so I think this is good to go.

@maxgolov
Copy link
Copy Markdown
Contributor Author

Maybe it's just some flakiness?

Let me rerun it. I think I saw that on other commits too.

@maxgolov
Copy link
Copy Markdown
Contributor Author

@pyohannes @reyang - I wanted to write-up a separate doc on this, but basically - one observation I captured in my repo, and added in this commit:

If we build OpenTelemetry SDK for Windows with Visual Studio 2019 with exceptions enabled, then in order for that to be compatible with other projects built using Visual Studio 2015 or 2017, the build MUST use /d2FH4- flag (don't use new exception handling..). Otherwise Visual Studio 2019 adds the 'new, better, improved, but backwards-incompatible' call to exception handler, rendering our library (if built as static) not linkable to .exe built with an older compiler. I think .dll flavor should be fine though, even if built with new exception semantics.

Copy link
Copy Markdown
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@reyang reyang merged commit 735fbfc into open-telemetry:master Jul 31, 2020
erichsueh3 pushed a commit to open-o11y/opentelemetry-cpp that referenced this pull request Aug 15, 2020
GerHobbelt pushed a commit to GerHobbelt/opentelemetry-cpp that referenced this pull request Jun 17, 2025
…install-winget-digest

chore(deps): update cyberboss/install-winget digest to d1b590c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants