-
Notifications
You must be signed in to change notification settings - Fork 5.4k
Windows compilation: enable compiling expanded list of extensions in envoy-static #10542
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
Merged
mattklein123
merged 54 commits into
envoyproxy:master
from
greenhouse-org:enabling-windows-common-tests
Apr 21, 2020
Merged
Changes from 24 commits
Commits
Show all changes
54 commits
Select commit
Hold shift + click to select a range
590a996
Remove erroneous test from PR #9964
sunjayBhatia 5662a35
Temporary workaround to exclude WINDOWS_SKIP_TARGETS
sunjayBhatia e7f9c05
Shorten adaptive_concurrency/concurrency_controller path names
wrowe b4b8249
All but excluded tests now compile on Windows
sunjayBhatia 519f712
Fix formatting and clean up test main invocation of WSAStartup
wrowe 5dac9d5
Windows CI steps ensures correct link.exe etc. on the PATH
sunjayBhatia b24bef2
Prepend VS tools to the path
wrowe 046849d
Add a proper msys2 toolchain as recommended by Bazel
sunjayBhatia 210ee5d
Fix download path for msys2.tar.xz
wrowe 9e0ec46
Fix tar path so it doesnt try to connect to a remote
sunjayBhatia 6b87110
Adjust tar change directory path
wrowe 5c0ce5f
Use unix style paths with tar only
sunjayBhatia 12f4d36
msys2 has a subdir named msys64, put that properly on the PATH
wrowe 466c7a0
Redirect pacman stderr to stout
sunjayBhatia 9b01e89
Use bash for pacman.exe invocation
wrowe 4e1141a
change cmd.exe invocation
sunjayBhatia 3bbbdd5
Use bash to check existence, cmd.exe is not working
wrowe 2a364ad
See if MSYS2_ARG_CONV_EXCL helps
sunjayBhatia 18f6392
Strip msys64 out of extracted msys2 path
wrowe f79a694
Fix BAZEL_SH path
sunjayBhatia 18184be
Fix path typo to find link.exe
wrowe a98a534
Cleanup unneeded content
sunjayBhatia 6cf3c9f
Shrink bazel's java heap as suggested by Lizan
wrowe 14c9918
Merge remote-tracking branch 'origin/master' into enabling-windows-co…
wrowe bba4fa6
Merge remote-tracking branch 'origin/master' into enabling-windows-co…
wrowe b587f56
Add back our changes to the windows pipeline
sunjayBhatia 94ca13d
Disable building tests for now
wrowe f5cfa64
Merge branch 'master' into enabling-windows-common-tests
sunjayBhatia 3cd9d27
Merge remote-tracking branch 'origin/master' into enabling-windows-co…
wrowe e3e5db0
Bump VC version
wrowe 2d84de9
Merge remote-tracking branch 'origin/master' into enabling-windows-co…
wrowe ceb0557
See if recent change to bazel memory usage is enough to let tests com…
sunjayBhatia d854b90
Revert "See if recent change to bazel memory usage is enough to let t…
wrowe 06f1d0c
Merge windows into unified workspace .bazelrc
wrowe a961c5e
Merge remote-tracking branch 'origin/master' into enabling-windows-co…
wrowe 3f51093
Revert back to correct way of filtering via extension name
sunjayBhatia dd87ac5
Various Windows compilation fixes
sunjayBhatia 5671603
Merge remote-tracking branch 'origin/master' into enabling-windows-co…
sunjayBhatia 7d9ba7e
Add workaround for PR 10777
sunjayBhatia d60b923
Merge remote-tracking branch 'origin/master' into enabling-windows-co…
wrowe e852c9a
Revert changes working around missing return
sunjayBhatia a00f460
Merge remote-tracking branch 'origin/master' into enabling-windows-co…
sunjayBhatia 3c111f7
Fix typo in comment
wrowe 6036c92
Remove changes in test/ tree to simplify PR
wrowe d00f3f6
Merge branch 'master' into enabling-windows-common-tests
wrowe e77b7a3
Restore WINDOWS_SKIP_TARGETS logic to PR 10542
wrowe 9eda28a
Kick CI
sunjayBhatia 50dc9b3
Merge remote-tracking branch 'origin/master' into enabling-windows-co…
wrowe 1b49268
Restore some variables tweaked for successful builds
wrowe aef5e03
Fix BAZEL_SH in Windows pipeline job
sunjayBhatia 44ab8f5
Retest for macOS
sunjayBhatia bfba02e
Merge remote-tracking branch 'origin/master' into enabling-windows-co…
wrowe bdc19bb
snakeCase not hungarian
wrowe cc4d1c8
snake_case is with underscores
sunjayBhatia File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| --- DescribeGroupsResponse.json 2020-03-25 16:12:16.373302600 -0400 | ||
| +++ DescribeGroupsResponse.json 2020-03-25 16:11:16.184156200 -0400 | ||
| @@ -63,7 +63,7 @@ | ||
| { "name": "MemberAssignment", "type": "bytes", "versions": "0+", | ||
| "about": "The current assignment provided by the group leader." } | ||
| ]}, | ||
| - { "name": "AuthorizedOperations", "type": "int32", "versions": "3+", "default": "-2147483648", | ||
| + { "name": "AuthorizedOperations", "type": "int32", "versions": "3+", "default": "INT32_MIN", | ||
| "about": "32-bit bitfield to represent authorized operations for this group." } | ||
| ]} | ||
| ] | ||
|
|
||
| --- MetadataResponse.json 2020-03-25 15:53:36.319161000 -0400 | ||
| +++ MetadataResponse.json 2020-03-25 15:54:11.510400000 -0400 | ||
| @@ -81,10 +81,10 @@ | ||
| { "name": "OfflineReplicas", "type": "[]int32", "versions": "5+", "ignorable": true, | ||
| "about": "The set of offline replicas of this partition." } | ||
| ]}, | ||
| - { "name": "TopicAuthorizedOperations", "type": "int32", "versions": "8+", "default": "-2147483648", | ||
| + { "name": "TopicAuthorizedOperations", "type": "int32", "versions": "8+", "default": "INT32_MIN", | ||
| "about": "32-bit bitfield to represent authorized operations for this topic." } | ||
| ]}, | ||
| - { "name": "ClusterAuthorizedOperations", "type": "int32", "versions": "8+", "default": "-2147483648", | ||
| + { "name": "ClusterAuthorizedOperations", "type": "int32", "versions": "8+", "default": "INT32_MIN", | ||
| "about": "32-bit bitfield to represent authorized operations for this cluster." } | ||
| ] | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Why just build but not run?
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.
This change was just an effort to ensure compilation of test sources is checked in CI. Not all the tests pass yet, we have not yet started delving into the test failures/figuring out what is flaky/fixing them fully. We didn't want to run all tests, have them fail intermittently, and break other PRs/pushes. If desired, we can tag all the tests that fail with
fails_on_windows(no guarantees we find all the flaky ones).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.
Right now, about 73 tests fail. After the next follow-up PR, we expect that about 5 corrections and abstractions will resolve 80% of those failures. Trying to keep the scope of each PR review reasonably manageable.
We've been increasing the stress on the wider community slowly. Thus far, we are simply trying to ensure new compilation breakage isn't introduced due to clang/gcc assumptions that MSVC doesn't interpret in the same way, and have been assisting PR contributors with guidance in resolving these compilation discrepancies for envoy-static. We'll continue to do so with the newly introduced //test/... compilation tests.
The resulting behavior differences of the Windows architecture begin to be addressed in next major PR, with a basket of remaining broken tests marked fails_on_windows on a short-term basis, to keep focus on not introducing regressions. The remaining 20% of failures will each demand more detailed fixes (likely 1 PR per failure) and closer scrutiny.
The end goal is 100% success and relies on additional volunteers (outside of our company) who are joining the effort already.