build: bazel changes to build on Windows #3786

sesmith177 · 2018-07-03T18:31:08Z

Description:

This is the first step in addressing #129

This PR just contains the build changes required to build a debug version of envoy.exe on Windows. The biggest difficulty is that Bazel hits the 32,768 character command line limit on Windows in 2 places:

generating protobufs with the cc_proto_library rule from https://github.com/google/protobuf/blob/master/protobuf.bzl
compiling the last couple .cc files (usually main.cc or main_common.cc)

To work around the protobuf issue, we use Bazel's native cc_proto_library rule on Windows. This has the side effect of not being able to use PGV / protobuf validation.

There's an issue open on Bazel to fix the long compile command issue: bazelbuild/bazel#5163. Until that is fixed, we work around this issue by not building the majority of the extensions on Windows. Once the issue is fixed, the extensions can begin to be enabled.

We plan to submit a follow-up PR that includes the actual code changes to make Envoy work on Windows.

Risk Level:
low

Testing:
bazel test //test/... on Linux

Docs Changes:
none so far

Release Notes:
none so far

cc @aminjam

This commit just contains the build changes required to build envoy on Windows. As is, it will not compile without code changes. We expect to add the code changes in an upcoming PR Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Amin Jamali <ajamali@pivotal.io>

mattklein123 · 2018-07-03T19:07:49Z

@jmillikin-stripe @htuch @lizan

@htuch do you know if there is someone from the Bazel Windows team that might be able to help review? Excited to see this land!

This is so it doesn't build when Linux ci runs `bazel build @envoy_api//envoy/...` Only one of the pgv_cc_proto_library or the native cc_proto_library can build at a time. Signed-off-by: Amin Jamali <ajamali@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

htuch · 2018-07-03T20:14:11Z

@dslomov do you know who we can get from Bazel Windows team to help out here?

dslomov · 2018-07-03T20:38:07Z

Laszlo (cc'd) is a TL for Bazel on Windows htuch <notifications@github.com> schrieb am Di., 3. Juli 2018, 22:14:

…

@dslomov <https://github.com/dslomov> do you know who we can get from Bazel Windows team to help out here? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3786 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AExrXzVT1UPXCsdwBNB2Z1ET1b6JQKNpks5uC9CdgaJpZM4VBbw0> .

htuch · 2018-07-03T21:19:28Z

@dslomov thanks, I assume the GH handle is @laszlocsomor.

laszlocsomor · 2018-07-04T12:09:16Z

I won't be able to review this PR for at least the next few days. Do you have any specific concerns about Windows? Or specific files that are risky and someone (maybe me, maybe someone else with Windows expertise) should look at?

laszlocsomor · 2018-07-05T09:38:19Z

/cc @meteorcloudy

jmillikin-stripe · 2018-07-06T17:06:14Z

I don't have any Windows machines and have never tried running Bazel or Envoy on Windows, so I may not be the best person to review this. Is there any particular part you'd like me to look at?

mattklein123 · 2018-07-06T17:08:24Z

@jmillikin-stripe if you could just sanity check that it won't break any of the linux stuff or is obviously doing something strange that would be great. We will find someone on the Bazel windows team to help review the windows stuff.

laszlocsomor · 2018-07-09T09:41:38Z

bazel/repositories.bat

@@ -0,0 +1,4 @@
+echo "Start"
+@ECHO OFF
+bash -c "./repositories.sh  %*"


Which Bash this will execute depends on the PATH. Do you expect to run MSYS Bash or the Bash installed by Windows Subsystem for Linux (available on Window 10, 1607)?

If the latter, you can make this unambiguous by using an absolute path (via the %SYSTEMROOT% environment variable):

%SYSTEMROOT%\system32\bash.exe -c "./repositories.sh %*"

We set %BAZEL_SH% to the MSYS2 bash here: https://github.com/greenhouse-org/envoy/blob/a8eefb8d2dbd9d0eb05ec5e839c8eea3b7994897/windows/setup/workstation_setup.ps1#L50

So we will update this script to use that environment variable

laszlocsomor · 2018-07-09T09:47:29Z

bazel/repositories.bzl

@@ -7,6 +7,11 @@ load(":genrule_repository.bzl", "genrule_repository")
 load(":patched_http_archive.bzl", "patched_http_archive")
 load(":repository_locations.bzl", "REPOSITORY_LOCATIONS")
 load(":target_recipes.bzl", "TARGET_RECIPES")
+load("@bazel_tools//tools/cpp:windows_cc_configure.bzl",


@meteorcloudy: do you think it's OK to use windows_cc_configure.bzl? As far as I know it has no public interface. If Envoy starts depending on it, we won't be able to change it without potentially breaking them.

I think it's fine. windows_cc_configure.bzl does expose find_vc_path, setup_vc_env_vars, find_msvc_tool.
I believe they are quite common functions that're needed when setting up MSVC related repo. The function interface didn't change since the beginning, so I think it's stable enough to let others to depend on it.

The question is why do we need to setup the MSVC environment variables? @sesmith177

The dependencies that are built with scripts (e.g. https://github.com/envoyproxy/envoy/blob/75909a7d16f0291ccd0bbe29517ac387a34701ea/ci/build_container/build_recipes/cares.sh) need to have the MSVC environment variables set. So we set the variables up when calling the repositories.bat script that ends up building them

Thanks for explanation, then this looks good to me!

laszlocsomor · 2018-07-09T09:55:08Z

ci/build_container/build_recipes/benchmark.sh

+make_cmd=make
+benchmark_lib="libbenchmark.a"
+
+if [[ "${OS}" == "Windows_NT" ]]; then


$OS is defined under MSYS Bash and Cygwin Bash, but not under WSL Bash (Ubuntu 14.04 on Windows Subsystem for Linux).

Which Bash do you expect to run this script under?

(Same question goes for other shell scripts that use $OS.)

We expect this to run under MSYS2 Bash

@sesmith177 can you add some build documentation to https://github.com/envoyproxy/envoy/blob/master/bazel/README.md that capture the build flow and also these assumptions?

Would it make sense to PR the build documentation when we PR source code changes? As it stands, Envoy won't actually build

I would prefer to land the documentation either here or in a distinct PR for docs alone; it doesn't really make sense to make the source delta PR larger with this.

OK, we'll follow up the source PR with a docs PR

laszlocsomor · 2018-07-09T09:56:33Z

ci/build_container/build_recipes/libevent.sh

-./configure --prefix="$THIRDPARTY_BUILD" --enable-shared=no --disable-libevent-regress --disable-openssl
-make V=1 install
+if [[ "${OS}" == "Windows_NT" ]]; then
+  wget -O libevent-"$VERSION".tar.gz https://github.com/libevent/libevent/archive/release-"$VERSION".tar.gz


AFAIK wget is not part of the default MSYS installation. You may need to document it as a requirement somewhere.

laszlocsomor · 2018-07-09T10:36:49Z

windows/bazel/get_workspace_status

+if [ -f SOURCE_VERSION ]
+then
+    echo "BUILD_SCM_REVISION $(cat SOURCE_VERSION)"
+    echo "STABLE_BUILD_SCM_REVISION $(cat SOURCE_VERSION)"


BUILD_SCM_VERSION is a known volatile key:

https://github.com/bazelbuild/bazel/blob/4304542a7438acaeb09559e71a971e56fbecc4e5/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java#L355-L359

What's the reason to export the same info as a stable key?

This script was copied from here: https://github.com/envoyproxy/envoy/blob/master/bazel/get_workspace_status

If we understand correctly, these variables are used for linkstamping: https://github.com/envoyproxy/envoy/blob/998b8119ed57269e77d0a03b1b92e7a98c5d30d1/source/common/common/version_linkstamp.cc

Since we haven't gotten linkstamping to work on Windows, we will just remove this

laszlocsomor · 2018-07-09T10:41:51Z

windows/bazel/get_workspace_status.bat

@@ -0,0 +1,3 @@
+@ECHO OFF
+bash -c "bazel/get_workspace_status %*"


Same question again as earlier: which Bash do you expect to use here?
Maybe it'd advantageous to check that the right Bash is the first on the PATH.

curl exists in MSYS2 by default. Signed-off-by: Sam Smith <sesmith177@gmail.com>

This was used for linkstamping on linux that would not work on windows. Signed-off-by: Amin Jamali <ajamali@pivotal.io>

workstation_setup.ps1 will set BAZEL_SH after installing MSYS2 Signed-off-by: Sam Smith <sesmith177@gmail.com>

htuch · 2018-07-09T17:28:16Z

api/bazel/api_build_system.bzl

@@ -131,6 +131,14 @@ def api_proto_library(name, visibility = ["//visibility:private"], srcs = [], de
        ],
        visibility = ["//visibility:public"],
    )
+
+    native.cc_proto_library(


So, without the PGV validation, there are two things we lose:

The ability to reject configurations that are somewhat nonsensical.

The guarantee that various preconditions on Envoy config processing code are met.

I think (1) is fine for an interim solution, but (2) is positively dangerous, since configuration can easily crash or pose security hazards to an Envoy instance. E.g. the config processing code might access an element in an array that has not been allocated, because it believes that due to PGV validation that the array is at least N elements long. If we want to accept (2) and the non-PGV variant for Windows, we probably want to slap a bunch of TODOs and warnings, together with tracking bugs, to ensure we bridge this gap.

When building with the PGV validation, we ran into a few issues:

rules_go wasn't properly handling Windows paths. This appears to be fixed in 1c7fd7e58310b832decc04963ec13db204f79150, but this commit has not been released

in PGV, _go_proto_library_gen_impl: https://github.com/lyft/protoc-gen-validate/blob/master/bazel/go_proto_library.bzl#L130 was not generating an OS agnostic script

The cc_proto_library rule from @com_google_protobuf//:protobuf.bzl did not de-duplicate command line arguments, causing it to hit the 32,768 character command line limit. The native Bazel cc_proto_library rule does not have this issue.

So it looks like the way forward would be something like:

Update project to latest rules_go once it is released

Fix up PGV _go_proto_library_gen_impl so it generates valid scripts for both Linux and Windows

PGV uses native Bazel cc_proto_library rules.

From the comment in api/bazel/api_build_system.bzl, it looks like the third step might not be possible at the moment

Yeah, I think there's going to be some non-trivial changes to get us to native cc_proto_library. I don't think we can do release binaries for Windows or consider Windows supported beyond experimental until this is fixed though. We should add TODOs and tracking issues for now.

htuch · 2018-07-09T17:29:44Z

api/bazel/api_build_system.bzl

+    native.cc_proto_library(
+        name = _Suffix(name, _CC_SUFFIX) + "_native",
+        deps = [name],
+        tags=["manual"],


Nit: prefer consistent spacing around = here.

htuch · 2018-07-09T17:30:02Z

api/bazel/api_build_system.bzl

@@ -131,6 +131,14 @@ def api_proto_library(name, visibility = ["//visibility:private"], srcs = [], de
        ],
        visibility = ["//visibility:public"],
    )
+
+    native.cc_proto_library(
+        name = _Suffix(name, _CC_SUFFIX) + "_native",


Nit: you might want to use a _CC_NATIVE_SUFFIX here instead.

htuch · 2018-07-09T17:32:54Z

bazel/BUILD

+)
+
+config_setting(
+    name = "windows_fastbuild_build",


I'm not that familiar with Bazel's platforms/toolchains support, but is this something we should be using to make the Windows vs. Linux vs. Mac OS X situation cleaner? In particular, I think we have multiple orthogonal build concerns:

Target OS (Windows (native vs. Linux compat?), Linux, Mac OS X)

Target runtime (libc++ vs. libstdc++)

Target toolchain (clang, gcc, Visual Studio)

Etc.

We added the windows_*_build config settings because there is no way to perform a select on multiple conditions. This allows envoy_copts to conditionally append --ggdb3 on Linux and never to append it on Windows: https://github.com/greenhouse-org/envoy/blob/windows-build-pr/bazel/envoy_build_system.bzl#L36-L41

With respect to the more general question, we are open to suggestions and what the community decides as the best approach.

@laszlocsomor @jmillikin-stripe is there a sensible way to handle this with https://docs.bazel.build/versions/master/platforms.html?

@laszlocsomor @jmillikin-stripe is there a sensible way to handle this with https://docs.bazel.build/versions/master/platforms.html?

pinging @katre to answer

I think adding a cflag on one platform should be a property of the cc_toolchain, if I understand it properly (given how confusing C++ is, I possibly don't). Are you defining your own cc_toolchain targets, or using the ones from auto-configuration?

If I understand correctly, on Windows we are just using the ones from auto-configuration, though on Linux there's a wrapper script: https://github.com/envoyproxy/envoy/blob/master/bazel/cc_configure.bzl

In that case, the existing code is probably the best way to do things.

htuch · 2018-07-09T17:34:14Z

ci/build_container/build_recipes/benchmark.sh

+make_cmd=make
+benchmark_lib="libbenchmark.a"
+
+if [[ "${OS}" == "Windows_NT" ]]; then


@sesmith177 can you add some build documentation to https://github.com/envoyproxy/envoy/blob/master/bazel/README.md that capture the build flow and also these assumptions?

htuch · 2018-07-09T17:34:50Z

ci/build_container/build_recipes/benchmark.sh

  -DCMAKE_BUILD_TYPE=RELEASE \
  -DBENCHMARK_ENABLE_GTEST_TESTS=OFF
-make
-cp src/libbenchmark.a "$THIRDPARTY_BUILD"/lib
+$make_cmd


Nit: "$make_cmd".

htuch · 2018-07-09T17:35:43Z

ci/build_container/build_recipes/benchmark.sh

+
+if [[ "${OS}" == "Windows_NT" ]]; then
+  cmake_generator="Ninja"
+  make_cmd=ninja


Why Ninja? Is there no way to do GNU make on Win?

We decided to use Ninja because that is what Microsoft uses in their VCPKG tool for managing/building C/C++ libraries. For benchmark specifically, see: https://github.com/Microsoft/vcpkg/blob/d977ac231e995250c169ac1778b7de34f7f57ead/ports/benchmark/portfile.cmake#L22

and https://github.com/Microsoft/vcpkg/blob/master/docs/maintainers/vcpkg_configure_cmake.md#prefer_ninja

Out of curiosity, does it fail if you use GNU make? I'm curious what the Bazel folks who are working on external dependency integration with cmake plan on Windows, CC @iirina.

For benchmark at least, we were not able to get it to work with GNU make

Signed-off-by: Sam Smith <sesmith177@gmail.com>

lizan · 2018-07-10T23:05:29Z

ci/prebuilt/BUILD

@@ -4,21 +4,30 @@ package(default_visibility = ["//visibility:public"])

 cc_library(
    name = "ares",
-    srcs = ["thirdparty_build/lib/libcares.a"],
+    srcs = glob([


Why do you need glob? Shouldn't this be select based on platform?

We initially had select based on platform, but our method for doing so was fighting with the Bazel buildifier. Thanks for having us take another look -- we will push a change that does the select correctly

Signed-off-by: Natalie Arellano <narellano@pivotal.io>

htuch · 2018-07-11T19:39:50Z

bazel/repositories.bzl

+        current_path = get_env_var(ctxt, "PATH", None, False)
+        env = setup_vc_env_vars(ctxt, vc_path)
+        env["PATH"]+=(";%s" % current_path)
+        env["CC"]="cl"


Can you merge master? We just merged a PR that applied Buildifier on Skylark files that should clean a lot of this up for consistency.

htuch · 2018-07-11T19:40:56Z

ci/build_container/build_recipes/nghttp2.sh

+diff --git a/lib/CMakeLists.txt b/lib/CMakeLists.txt
+index 17e422b..b2e7a6e 100644
+--- a/lib/CMakeLists.txt
+++ b/lib/CMakeLists.txt


Are there plans to upstream this?

PR: nghttp2/nghttp2#1198

htuch · 2018-07-11T19:41:46Z

ci/build_container/build_recipes/yaml-cpp.sh

+    -DYAML_CPP_BUILD_TESTS=off \
+    -DCMAKE_BUILD_TYPE=Debug ..
+  ninja
+  ninja install


Maybe we could switch to ninja on non-Window? I would prefer greater consistency for maintainability here.

From our perspective, we are explicitly trying not to change anything about how non-Windows builds happen in this PR. Perhaps a future PR?

Also, switching to ninja on non-Windows introduces a new build dependency, which is maybe not good?

I have a more fundamental question while we're thinking about this. How are we going to do CI, gate submissions and do build regression handling for Windows? I know a while back we chatted (@mattklein123 ) and had some idea that Windows would be best-effort, with the Microsoft folks owning the regression fixes. Is this where we are still at? I don't think CircleCI has Windows support.

We spoke with @mattklein123 about this a couple of weeks ago. It seems like Windows support would still be best-effort at this time. We've used AppVeyor for Windows CI in the past, and that's what we were thinking of using. We could help get this set up so that the automation is in place, but not gate on CI passing (for now).

htuch · 2018-07-11T19:43:00Z

ci/do_ci.ps1

+  echo "Building..."
+  pushd "$env:ENVOY_SRCDIR"
+    bazel  $env:BAZEL_BASE_OPTIONS.Split(" ") build $env:BAZEL_BUILD_OPTIONS.Split(" ") -c dbg "//source/exe:envoy-static"
+    $exit = $LASTEXITCODE


I sadly don't even know how to review PowerShell; are there any @envoyproxy/maintainers who do?

sesmith177 · 2018-07-12T14:11:01Z

@mattklein123 @laszlocsomor after merging in master, we had to disable all Envoy extensions to get the final compile command lines under the limit (bazelbuild/bazel#5163).

Signed-off-by: Matthew Horan <mhoran@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

Signed-off-by: Sam Smith <sesmith177@gmail.com>

Signed-off-by: Matthew Horan <mhoran@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Matthew Horan <mhoran@pivotal.io>

Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Matthew Horan <mhoran@pivotal.io>

jmillikin-stripe · 2018-07-16T15:06:21Z

I've tried to fully review this PR a few times, but keep getting lost partway through -- it's quite large, and touches multiple parts of the build in ways that I don't understand the interactions of.

Is it possible to split it up into multiple PRs? In particular, the order would be something like:

Simple and non-controversial changes, like config_setting targets, Windows-specific copts, Windows-specific extension lists, and the batch files. Lets just get those merged in.
The make -> ninja patches, which look straightforward but deserves a bit of thought. Specifically I'm wondering if we could switch all platforms over to Ninja instead of bifurcating the build logic.
Hacks/workarounds for the protobuf validation plugin. I don't think these should be merged as-is, and would strongly prefer an approach that fixes PGV upstream. I can help with this if needed, since I have some experience in porting things between rules_go versions.
Hacks/workarounds for C++ protobuf codegen. I talked with some of the Bazel devs just now, and they're going to see if the upstream protobuf.bzl could be factored out into a rules_proto repository. This would hopefully let us fix the too-long cl.exe commands without plumbing protobuf_cc_deps through the entire repo.

htuch · 2018-07-27T01:40:41Z

Static registration for message checks could work I think (providing there is no ordering issues), so no objection really if you can do the lifting there.

We can always fork protobuf.bzl if we can't get your changes upstream. We really, really dislike forking due to the problems of maintenance and version skew, @jmillikin-stripe and I have had a lot of experience here. But, it's acceptable if it's the least awful thing to do to make progress.

Ideally, we stop using protobuf.bzl altogether and use native proto_library based rules in PGV. I'm not sure how far from this we are today.

jmillikin-stripe · 2018-07-27T17:14:16Z

We can also patch upstream dependencies with diff -u files or plain commands, if the changes needed are relatively small (like, not a full refactoring).

sesmith177 · 2018-07-30T18:59:07Z

@htuch @jmillikin-stripe Here's our PR to PGV to bump rules_go: bufbuild/protoc-gen-validate#87

stale · 2018-08-06T19:49:13Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

jmillikin-stripe · 2018-08-06T19:52:31Z

Lets not mark things as stale just because they're difficult, stalebot.

sesmith177 · 2018-08-07T15:47:33Z

@htuch @jmillikin-stripe pr to PGV to break dependency on protobuf.bzl; bufbuild/protoc-gen-validate#92

sesmith177 · 2018-08-21T14:19:28Z

@htuch @jmillikin-stripe here's our PR to get PGV working on Windows: bufbuild/protoc-gen-validate#94

htuch · 2018-08-22T15:48:15Z

@sesmith177 great, thanks for the awesome work moving that forward. Can you refresh us on what the next steps are?

sesmith177 · 2018-08-22T17:50:30Z

@htuch the next step would be to bump PGV in Envoy (though that is currently blocked by lyft/protoc-gen-star#28). Once that is done, it looks like all the build changes will have been taken care of, and this PR could be closed.

After that, we would begin the process of getting code changes upstream.

Note that we currently have to use a forked version of bazel to build/test our Envoy fork due to bazelbuild/bazel#5163. So we're not blocked from working on our fork at the moment, but we'd probably want other people to be able to easily build our code before getting it upstream

moderation · 2018-08-22T18:11:18Z

Working with @akonradi I've done some testing trying to integrate this latest PGV into Envoy. Diffs below and upgraded to rules_go 0.14.0. I'm past all of the compiler issues but hit bufbuild/protoc-gen-validate#89 now.

diff --git a/WORKSPACE b/WORKSPACE
index 5545e2ac4..ebdb050a7 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -10,7 +10,5 @@ load("@envoy_api//bazel:repositories.bzl", "api_dependencies")
 api_dependencies()

 load("@io_bazel_rules_go//go:def.bzl", "go_rules_dependencies", "go_register_toolchains")
-load("@com_lyft_protoc_gen_validate//bazel:go_proto_library.bzl", "go_proto_repositories")
-go_proto_repositories(shared=0)
 go_rules_dependencies()
 go_register_toolchains()

diff --git a/api/bazel/api_build_system.bzl b/api/bazel/api_build_system.bzl
index 497d82c5c..67ed53658 100644
--- a/api/bazel/api_build_system.bzl
+++ b/api/bazel/api_build_system.bzl
@@ -122,9 +122,8 @@ def api_proto_library(name, visibility = ["//visibility:private"], srcs = [], de
     # the proto_library above are aligned.
     pgv_cc_proto_library(
         name = _Suffix(name, _CC_SUFFIX),
-        srcs = srcs,
-        deps = [_LibrarySuffix(d, _CC_SUFFIX) for d in deps],
-        external_deps = [
+        deps = [name],
+        cc_deps = [_LibrarySuffix(d, _CC_SUFFIX) for d in deps] + [
             "@com_google_protobuf//:cc_wkt_protos",
             "@googleapis//:http_api_protos",
             "@googleapis//:rpc_status_protos",

diff --git a/api/bazel/repositories.bzl b/api/bazel/repositories.bzl
index 2e497d712..cdcde74d2 100644
--- a/api/bazel/repositories.bzl
+++ b/api/bazel/repositories.bzl
@@ -1,9 +1,9 @@

-PGV_GIT_SHA = "f9d2b11e44149635b23a002693b76512b01ae515"
+PGV_GIT_SHA = "f15f532855984e68dc1474598e4692fc6a08d841"  # Aug 21, 2018

 load("@bazel_tools//tools/build_defs/repo:git.bzl", "git_repository")

@@ -33,7 +33,10 @@ filegroup(
 proto_library(
     name = "api_httpbody_protos_proto",
     srcs = [":api_httpbody_protos_src"],
-    deps = ["@com_google_protobuf//:descriptor_proto"],
+    deps = [
+        "@com_google_protobuf//:any_proto",
+        "@com_google_protobuf//:descriptor_proto",
+    ],
     visibility = ["//visibility:public"],
 )

htuch · 2018-08-22T22:32:12Z

@sesmith177 @moderation thanks for the update. I think this is actually blocking #4233 now; the reason this is because #4233 fails on OS X builds due to PGV paths being too long, I think your move to depsets fixed that, but I can't import it due to the failures you folks have been chasing. So, please do let me know when that lands, happy to review the PR to move things along.

htuch · 2018-08-30T16:46:44Z

@sesmith177 @rodaine do you know where we are with PGV update for Envoy? I'd like to add some PGV features, but it seems we're blocked on the above item unless it has been resolved. Thanks.

sesmith177 · 2018-08-31T19:53:10Z

@htuch bufbuild/protoc-gen-validate#89 is still an outstanding blocker, as far as I know

htuch · 2018-09-27T18:18:15Z

@sesmith177 now that bufbuild/protoc-gen-validate#103 has merged, this should be unblocked.

htuch · 2018-09-27T18:36:45Z

@sesmith177 I tried to bump the existing build (see WiP branch https://github.com/htuch/envoy/tree/pgv-bump). Unfortunately, it looks like there are some issues with the new C++ validation rules (replacing the previous weak linking). Here's what I see:

ERROR: file 'external/envoy_api/envoy/api/v2/validate.pb.validate.h' is generated by these conflicting actions:
Label: @envoy_api//envoy/api/v2:lds_cc_validate, @envoy_api//envoy/api/v2:discovery_cc_validate
RuleClass: cc_proto_gen_validate rule
Configuration: 07268499d71d5934dbb9817e10a7700e
Mnemonic: ProtoGenValidateCcGenerate
Action key: 624b2eb7eba961553e95f187bcb1cd37, e7ec72e99dc1c0a1270d3ebbe2b16f23
Progress message: ProtoGenValidateCcGenerate external/envoy_api/envoy/api/v2/lds.pb.validate.h, ProtoGenValidateCcGenerate external/envoy_api/envoy/api/v2/discovery.pb.validate.h
PrimaryInput: File:[[<execution_root>]bazel-out/host/bin]external/com_google_protobuf/protoc
PrimaryOutput: File:[[<execution_root>]bazel-out/k8-fastbuild/genfiles]external/envoy_api/envoy/api/v2/lds.pb.validate.h, File:[[<execution_root>]bazel-out/k8-fastbuild/genfiles]external/envoy_api/envoy/api/v2/discovery.pb.validate.h
Primary outputs are different: 70586236, 617752951
Owner information: @envoy_api//envoy/api/v2:lds_cc_validate BuildConfigurationValue.Key[07268499d71d5934dbb9817e10a7700e] false, @envoy_api//envoy/api/v2:discovery_cc_validate BuildConfigurationValue.Key[07268499d71d5934dbb9817e10a7700e] false
MandatoryInputs: Attempted action contains artifacts not in previous action (first 5):
        external/envoy_api/envoy/api/v2/lds.proto
        external/envoy_api/envoy/api/v2/core/address-descriptor-set.proto.bin
        external/envoy_api/envoy/api/v2/core/grpc_service-descriptor-set.proto.bin
        external/envoy_api/envoy/api/v2/core/config_source-descriptor-set.proto.bin
        external/envoy_api/envoy/api/v2/auth/cert-descriptor-set.proto.bin
Previous action contains artifacts not in attempted action (first 5):
        external/envoy_api/envoy/api/v2/discovery.proto
Outputs: Attempted action contains artifacts not in previous action (first 5):
        external/envoy_api/envoy/api/v2/lds.pb.validate.h
        external/envoy_api/envoy/api/v2/validate.pb.validate.h
        external/envoy_api/envoy/api/v2/lds.pb.h
        external/envoy_api/envoy/api/v2/validate.pb.h
        external/envoy_api/envoy/api/v2/lds.pb.validate.cc
Previous action contains artifacts not in attempted action (first 5):
        external/envoy_api/envoy/api/v2/discovery.pb.validate.h
        external/envoy_api/envoy/api/v2/validate.pb.validate.h
        external/envoy_api/envoy/api/v2/discovery.pb.h
        external/envoy_api/envoy/api/v2/validate.pb.h
        external/envoy_api/envoy/api/v2/discovery.pb.validate.cc
ERROR: Analysis of target '@envoy_api//envoy/api/v2:lds_cc_validate' failed; build aborted
INFO: Elapsed time: 12.071s
INFO: 0 processes.

I'm not sure how validate.pb.h is appearing there, we don't have this as a physical proto.

htuch · 2018-09-27T18:47:26Z

Actually, looks like I just had to drop the @com_lyft_protoc_gen_validate//validate:validate_proto dep to fix this, despite https://github.com/lyft/protoc-gen-validate/blob/3bd820adb86cc455b16b37465905a9b99e22cbbb/bazel/pgv_proto_library.bzl#L50.

sesmith177 · 2018-09-27T18:50:13Z

That's interesting -- we are looking at this as well

sesmith177 · 2018-09-27T19:32:42Z

@htuch we have a branch that builds and passes all the tests on Linux: https://github.com/greenhouse-org/envoy/tree/pgv-on-windows
once we've confirmed it all works on Windows as well, we can PR

With regard to the misleading comment in PGV, the issue is that all of the test cases in that repo import validate/validate.proto directly, and so do depend on @com_lyft_protoc_gen_validate//validate:validate_proto. We incorrectly generalized this to any user of pgv_cc_proto_library and will PR to fix the comment

htuch · 2018-09-27T19:40:48Z

@sesmith177 great. I also have a PR ready to go.

htuch · 2018-09-27T19:46:42Z

#4551

sesmith177 · 2018-09-28T16:26:12Z

@htuch submitted #4556 -- these are the last dependency bumps to get everything building on Windows. After that, I think we can close this PR and begin the process of getting our source code changes upstream

sesmith177 · 2018-10-05T22:01:36Z

Since #4556 has been merged, this PR can be closed. We will probably make further PRs to the build system (e.g. for release builds), however the goal of this PR is now accomplished. Further progress in the Windows port will be tracked in #129

sesmith177 requested review from lizan and zuercher as code owners July 3, 2018 18:31

mattklein123 assigned jmillikin-stripe and htuch Jul 6, 2018

mhoran mentioned this pull request Jul 6, 2018

Build Envoy for Windows using Bazel microsoft/envoy#7

Open

laszlocsomor reviewed Jul 9, 2018

View reviewed changes

Amin Jamali and others added 3 commits July 9, 2018 11:12

Use curl instead of wget

1f88b54

curl exists in MSYS2 by default. Signed-off-by: Sam Smith <sesmith177@gmail.com>

Remove get_workspace_status for windows builds

a8eefb8

This was used for linkstamping on linux that would not work on windows. Signed-off-by: Amin Jamali <ajamali@pivotal.io>

BAZEL_SH is the correct bash

80b38b3

workstation_setup.ps1 will set BAZEL_SH after installing MSYS2 Signed-off-by: Sam Smith <sesmith177@gmail.com>

htuch suggested changes Jul 9, 2018

View reviewed changes

Fix nits

19e4002

Signed-off-by: Sam Smith <sesmith177@gmail.com>

lizan reviewed Jul 10, 2018

View reviewed changes

Use select for prebuilt dependencies

91a8799

Signed-off-by: Natalie Arellano <narellano@pivotal.io>

htuch suggested changes Jul 11, 2018

View reviewed changes

sesmith177 and others added 4 commits July 12, 2018 10:13

Merge branch 'master' into windows-build-pr

6ad7c28

Signed-off-by: Matthew Horan <mhoran@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

Fix Windows build after merge

f1a0385

Signed-off-by: Sam Smith <sesmith177@gmail.com>

Format fixes

b5479cf

Signed-off-by: Matthew Horan <mhoran@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Matthew Horan <mhoran@pivotal.io>

Simplify nghttp2 patch (hopefully can be upstreamed)

e786114

Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Matthew Horan <mhoran@pivotal.io>

mhoran mentioned this pull request Aug 1, 2018

NT support #129

Closed

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 6, 2018

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Aug 6, 2018

htuch added the no stalebot Disables stalebot from closing an issue label Aug 7, 2018

htuch mentioned this pull request Sep 11, 2018

proto: unify envoy_proto_library/api_proto_library. #4233

Merged

moderation mentioned this pull request Sep 19, 2018

Upgrade bazelbuild rules_go to latest release: 0.13.0 #3949

Closed

sesmith177 closed this Oct 5, 2018

sesmith177 deleted the windows-build-pr branch October 5, 2018 16:14

		@@ -0,0 +1,3 @@
		@ECHO OFF
		bash -c "bazel/get_workspace_status %*"

build: bazel changes to build on Windows #3786

build: bazel changes to build on Windows #3786

Conversation

sesmith177 commented Jul 3, 2018

mattklein123 commented Jul 3, 2018

htuch commented Jul 3, 2018

dslomov commented Jul 3, 2018 via email

htuch commented Jul 3, 2018

laszlocsomor commented Jul 4, 2018

laszlocsomor commented Jul 5, 2018

jmillikin-stripe commented Jul 6, 2018

mattklein123 commented Jul 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sesmith177 commented Jul 12, 2018

jmillikin-stripe commented Jul 16, 2018

htuch commented Jul 27, 2018

jmillikin-stripe commented Jul 27, 2018

sesmith177 commented Jul 30, 2018

stale bot commented Aug 6, 2018

jmillikin-stripe commented Aug 6, 2018

sesmith177 commented Aug 7, 2018

sesmith177 commented Aug 21, 2018

htuch commented Aug 22, 2018

sesmith177 commented Aug 22, 2018

moderation commented Aug 22, 2018

htuch commented Aug 22, 2018

htuch commented Aug 30, 2018

sesmith177 commented Aug 31, 2018

htuch commented Sep 27, 2018

htuch commented Sep 27, 2018

htuch commented Sep 27, 2018

sesmith177 commented Sep 27, 2018

sesmith177 commented Sep 27, 2018

htuch commented Sep 27, 2018

htuch commented Sep 27, 2018

sesmith177 commented Sep 28, 2018