Skip to content
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

Fixes to compile SDK with Visual Studio 2019 Update 16.10 (C++20) #820

Merged
merged 9 commits into from
Jun 10, 2021

Conversation

maxgolov
Copy link
Contributor

@maxgolov maxgolov commented Jun 3, 2021

Fixes #819

Changes

Quick recap:

  • latest update to Visual Studio 2019 compiler (16.10) introduces support for constinit. However, this new feature breaks protobuf ( vs2019 update 16.10 : illegal initialization of constinit entity with a non-constant expression protocolbuffers/protobuf#8688 ). Either because the feature is not fully implemented, or because protobuf uses it in some interesting way.

  • I am verifying that with the fix all code now builds and work well with WITH_STL ( -DHAVE_CPP_STDLIB) in C++20. And that's where we now get to compile protobuf-generated stubs for OTLP with C++20. The issue was that it all worked literally last week with 16.9, got broken in 16.10. Which is very frustrating.. Because GitHub Action runner images have also all already been updated to pull the latest Visual Studio 2019 Update 16.10. There is no way back. Means if we don't fix it now, we can't verify C++20 builds with msvc-2019-16.10 in our CI.

Fixes

Local patch here includes:

It may take some time before either of those fixes get merged.

There are no changes to API. No functionality changes expected other than fixing the build break. All files in tools/ports/protobuf are original files from mainline vcpkg port. With a minor fix (mainline PRs above) - applied on top, pretty much to disable the usage of constinit. Since it appears to be not ready to be used yet. At least not in Protobuf codebase.

tools/setup-buildtools.cmd now covers overlays for:

  • Google Benchmark (was there before)
  • Protobuf

Pinning both of them to versions that work well; with patches on top to make sure it works well even with latest vs2019.

I also updated vcpkg submodule snapshot to:

commit 5568f110b509a9fd90711978a7cb76bae75bb092 (HEAD, tag: 2021.05.12)
Author: Victor Romero <romerosanchezv@gmail.com>
Date:   Thu May 13 12:53:39 2021 -0700

That's the latest vcpkg release published here, so I figured the patched overlays must be compatible with that exact latest version of vcpkg since it is giving us the latest stable versions of everything.

@maxgolov maxgolov requested a review from a team June 3, 2021 00:55
@codecov
Copy link

codecov bot commented Jun 3, 2021

Codecov Report

Merging #820 (21ea274) into main (c2c7976) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #820   +/-   ##
=======================================
  Coverage   95.50%   95.50%           
=======================================
  Files         156      156           
  Lines        6614     6614           
=======================================
  Hits         6316     6316           
  Misses        298      298           

@lalitb
Copy link
Member

lalitb commented Jun 3, 2021

@maxgolov - Just to be clear, these are the temporary changes and supposed to be removed once the mainstream patches are available for vcpkg, and/or fix is provided by protobuf?

@maxgolov
Copy link
Contributor Author

maxgolov commented Jun 3, 2021

@maxgolov - Just to be clear, these are the temporary changes and supposed to be removed once the mainstream patches are available for vcpkg, and/or fix is provided by protobuf?

Yes.

  • same (identical) patch is now merged in protobuf master. I submitted it in there and it's already merged.
  • same exact patch is still on review in vcpkg.
  • it may take several weeks-to-months before protobuf posts the new release AND this release is picked up by vcpkg.

My goal is to set up the build loops with vs2015(c++11) and vs2019(c++20) sooner than the patches are integrated in the upstream.

@maxgolov
Copy link
Contributor Author

maxgolov commented Jun 4, 2021

Identical fix approved in vcpkg repo as well. But then we'd need to upgrade the vcpkg submodule to master once this fix is merged.. I was trying to keep our overall vcpkg version at latest well-tested release, so overlay may be a better idea short-term until the next major vcpkg release.

@owent owent mentioned this pull request Jun 5, 2021
3 tasks
Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

LGTM to have a temporary overlay. We can have a separate issue for cleanup once vcpkg brings the new protobuf version.

@maxgolov
Copy link
Contributor Author

maxgolov commented Jun 7, 2021

We need to handle the removal of the overlay in a few months from now (ETA: 3-5 months), when the next major vcpkg mainline gets released with that patch included. Don't want to get stuck depending on vcpkg release management cycle. This may not be a big blocker, assuming we're not upgrading gRPC / protobuf too much too fast. And so the overlay patch may stay longer than 6 months. Real practical need is only if we need to move on to protobuf from May 2021 and newer.

@ThomsonTan
Copy link
Contributor

If the same PR is already under review in vcpkg repo, could we just update our submodule once it is merged there? Instead of dup the fix here?

@maxgolov
Copy link
Contributor Author

maxgolov commented Jun 8, 2021

If the same PR is already under review in vcpkg repo, could we just update our submodule

We were mostly trying to anchor to their major releases, not latest master. So once it's merged - I am not sure how soon we have to wait for their major release tag? That's why I was thinking to live with the overlay for another few months. vcpkg PR is this one: microsoft/vcpkg#18251

It's approved. All checks passed. But not merged yet. Once it's merged, then I guess we'd have to wait for a few weeks for the next major release tag. That's too slow. Even if we overlay, for another 5-6 months we'd be fine with the patch even if we intake their new tag.. So I'd rather put the update to latest vcpkg on a back-burner until end of this year. Then we'd have to carefully review the overlays for protobuf and benchmark, and obsolete them (delete these patches in our repo) if they are no longer applicable.

@maxgolov
Copy link
Contributor Author

maxgolov commented Jun 8, 2021

This test is flaky (unrelated to my change, have been failing before):

	179 - trace.CircularBufferTest.Simulation (Failed)

Logged separate issue for that #841

@maxgolov maxgolov added the pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.) label Jun 9, 2021
@lalitb lalitb merged commit 0dbd620 into main Jun 10, 2021
@lalitb lalitb deleted the maxgolov/protobuf_fix_vs2019_16.10 branch June 10, 2021 15:38
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
3 participants