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

Enable Windows CI //source/exe:envoy-static build #10293

Merged
merged 49 commits into from
Mar 14, 2020
Merged

Enable Windows CI //source/exe:envoy-static build #10293

merged 49 commits into from
Mar 14, 2020

Conversation

wrowe
Copy link
Contributor

@wrowe wrowe commented Mar 6, 2020

Description:

  • Build and CI tooling to get windows compiling
  • (Reintroduces an experimental ci/windows_build_steps.ps1)
  • Source changes required for Windows compilation on msvc
  • Pick up a flavor of nameser.h for Windows only from c-ares dependency
  • Excludes all source fixes unrelated to compilation passing on Windows

Risk Level: Low
Testing: local on linux/gcc windows/msvc
Docs Changes: n/a
Release Notes: n/a

wrowe and others added 2 commits March 6, 2020 17:15
- Pick up nameser.h for Windows from c-ares dependency

Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
@wrowe
Copy link
Contributor Author

wrowe commented Mar 6, 2020

Will review CI PATH issues on Monday, hopefully in a cleaner way to inject devenv variables and avoid msys2 link.exe
/wait

- This PATH is too explicit, and we likely want to use devenv in the
  VC toolkit to resolve the correct build envvars in conjunction with
  the specific build instance. Perhaps a build machine local .bazelrc
  is a possible workaround?

Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
See bazel-contrib/rules_foreign_cc#334

Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
@@ -0,0 +1,16 @@
#powershell.exe -Command
Copy link
Member

Choose a reason for hiding this comment

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

For the maintainers to consider: we added this version of the window_ci_steps script in case we wanted to converge on using powershell only instead of a mix of bash and powershell, up to y'all, we can remove it if we want to stick with bash scripts

Copy link
Member

Choose a reason for hiding this comment

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

If powershell is desired, we can refactor with an "invoke bazel" function etc. so we dont have to keep rewriting the error code checking

Copy link
Member

Choose a reason for hiding this comment

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

I'm OK either way as long as we have one canonical solution.

Copy link
Member

Choose a reason for hiding this comment

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

ok, removing this one

wrowe and others added 20 commits March 10, 2020 16:29
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
- we have to assume the path of the bash mounted filesystem

Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
- working directory of actual bazel tasks is in D: drive, so /tmp resolves to D:\tmp
- having both covers our bases

Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
bazel/envoy_internal.bzl Outdated Show resolved Hide resolved
@@ -208,7 +208,8 @@ CompressorFilter::chooseEncoding(const Http::ResponseHeaderMap& headers) const {

// Find all encodings accepted by the user agent and adjust the list of allowed compressors.
for (const auto token : StringUtil::splitToken(*accept_encoding_, ",", false /* keep_empty */)) {
EncPair pair = std::make_pair(StringUtil::trim(StringUtil::cropRight(token, ";")), 1);
EncPair pair =
std::make_pair(StringUtil::trim(StringUtil::cropRight(token, ";")), static_cast<float>(1));
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -18,4 +18,4 @@ echo "ENVOY_BAZEL_ROOT: $env:ENVOY_BAZEL_ROOT"
echo "ENVOY_SRCDIR: $env:ENVOY_SRCDIR"
Copy link
Member

Choose a reason for hiding this comment

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

For the maintainers to consider: this file is not actually used in CI and only is referenced in do_ci.ps1, should we change over our CI pipelines to use the do_ci pattern like on Linux? (or should we just simply keep it around until we can do the full set of builds/tests?)

Copy link
Member

Choose a reason for hiding this comment

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

we're deferring changing around how CI is done on Windows for now

@@ -14,41 +14,53 @@ function bazel_binary_build($type) {
}
Copy link
Member

Choose a reason for hiding this comment

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

For the maintainers to consider: should we change over our CI pipelines to use the do_ci pattern like on Linux? (or should we just simply keep it around until we can do the full set of builds/tests?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps we defer this discussion until this is merged? Good conversation, and probably doesn't have to hold up adoption of this specific PR.

…-build

Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
@sunjayBhatia
Copy link
Member

merging master to get recent fix that should get coverage tests passing

Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
@sunjayBhatia
Copy link
Member

MacOS CI should only be failing due to: #10357

Copy link
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

Fantastic, thanks! Just a few comment.

bazel/envoy_internal.bzl Outdated Show resolved Hide resolved
@@ -0,0 +1,16 @@
#powershell.exe -Command
Copy link
Member

Choose a reason for hiding this comment

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

I'm OK either way as long as we have one canonical solution.

@lizan lizan self-assigned this Mar 12, 2020
@lizan lizan added the waiting label Mar 12, 2020
Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
sunjayBhatia and others added 2 commits March 13, 2020 11:11
Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
@wrowe wrowe requested review from dio and lizan March 13, 2020 15:57
sunjayBhatia and others added 3 commits March 13, 2020 14:34
- measure time
- fastbuild and debug are producing exes that fail an absl assertion

Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
…-build

Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

# Required to work around quiche build defect
# Unguarded gcc pragmas are not recognized by MSVC
build:msvc-cl --copt="/wd4068"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the walkaround for #10239?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well I don't think it is from the comment... How did you walk around that failure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, nothing to do with 10239, that fix is not offered in this PR. That fix will become required when we begin compiling //test/... (which this PR doesn't attempt to do). This PR simply gets us compiling //source/exe:envoy-static

You can see the workaround to 10239 at https://github.com/greenhouse-org/envoy/commit/66eff7c1e12544a37792140039867481b9bc91f8 which we will offer in the next PR that addresses compiling //test/...

This fix above resolves gcc pragmas which cause fatal warnings-as-errors failure when we trip over the gcc pragmas in poorly constructed quiche headers. See;
https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-1-c4068?view=vs-2019

Copy link
Member

@sunjayBhatia sunjayBhatia Mar 13, 2020

Choose a reason for hiding this comment

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

Ah, no that was a comment that snuck in from when we were trying to fix the quiche thing before, will remove the comment that warning is around unrecognized pragmas: https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-1-c4068?view=vs-2019, not the same issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note it is extra confusing because we transform the source tree from clang pragmas into gcc pragmas, both of which cannot be understood by msvc-cl. (clang-cl, when we get that working, would probably recognize the clang flavor of the pragmas)

@sunjayBhatia
Copy link
Member

/retest

seeing if the coverage tests are flakes, they passed previously so actual failures would be introduced by more recent commits to master

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #10293 (comment) was created by @sunjayBhatia.

see: more, trace.

@wrowe
Copy link
Contributor Author

wrowe commented Mar 14, 2020

Bitten again by flakes; please

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #10293 (comment) was created by @wrowe.

see: more, trace.

@lizan lizan merged commit e7fc5fc into envoyproxy:master Mar 14, 2020
@wrowe wrowe deleted the test-windows-source-build branch March 14, 2020 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants