build: bazel changes to build on Windows #3786
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,3 +15,4 @@ SOURCE_VERSION | |
| .cache | ||
| .vimrc | ||
| .vscode | ||
| .vs | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,35 @@ genrule( | |
| stamp = 1, | ||
| ) | ||
|
|
||
| config_setting( | ||
| name = "windows_x86_64", | ||
| values = {"cpu": "x64_windows"}, | ||
| ) | ||
|
|
||
| config_setting( | ||
| name = "windows_opt_build", | ||
| values = { | ||
| "cpu": "x64_windows", | ||
| "compilation_mode": "opt", | ||
| }, | ||
| ) | ||
|
|
||
| config_setting( | ||
| name = "windows_dbg_build", | ||
| values = { | ||
| "cpu": "x64_windows", | ||
| "compilation_mode": "dbg", | ||
| }, | ||
| ) | ||
|
|
||
| config_setting( | ||
| name = "windows_fastbuild_build", | ||
|
There was a problem hiding this comment. 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:
There was a problem hiding this comment. We added the With respect to the more general question, we are open to suggestions and what the community decides as the best approach. There was a problem hiding this comment. @laszlocsomor @jmillikin-stripe is there a sensible way to handle this with https://docs.bazel.build/versions/master/platforms.html? There was a problem hiding this comment.
pinging @katre to answer There was a problem hiding this comment. 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? There was a problem hiding this comment. 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 There was a problem hiding this comment. In that case, the existing code is probably the best way to do things. |
||
| values = { | ||
| "cpu": "x64_windows", | ||
| "compilation_mode": "fastbuild", | ||
| }, | ||
| ) | ||
|
|
||
| config_setting( | ||
| name = "opt_build", | ||
| values = {"compilation_mode": "opt"}, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| echo "Start" | ||
| @ECHO OFF | ||
| bash -c "./repositories.sh %*" | ||
|
There was a problem hiding this comment. Which Bash this will execute depends on the If the latter, you can make this unambiguous by using an absolute path (via the There was a problem hiding this comment. We set So we will update this script to use that environment variable |
||
| exit %ERRORLEVEL% | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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", | ||
|
There was a problem hiding this comment. @meteorcloudy: do you think it's OK to use There was a problem hiding this comment. I think it's fine. There was a problem hiding this comment. The question is why do we need to setup the MSVC environment variables? @sesmith177 There was a problem hiding this comment. 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 There was a problem hiding this comment. Thanks for explanation, then this looks good to me! |
||
| "find_vc_path", | ||
| "setup_vc_env_vars", | ||
| ) | ||
| load("@bazel_tools//tools/cpp:lib_cc_configure.bzl", "get_env_var") | ||
|
|
||
| def _repository_impl(name, **kwargs): | ||
| # `existing_rule_keys` contains the names of repositories that have already | ||
|
|
@@ -62,6 +67,7 @@ def _repository_impl(name, **kwargs): | |
| def _build_recipe_repository_impl(ctxt): | ||
| # Setup the build directory with links to the relevant files. | ||
| ctxt.symlink(Label("//bazel:repositories.sh"), "repositories.sh") | ||
| ctxt.symlink(Label("//bazel:repositories.bat"), "repositories.bat") | ||
| ctxt.symlink(Label("//ci/build_container:build_and_install_deps.sh"), | ||
| "build_and_install_deps.sh") | ||
| ctxt.symlink(Label("//ci/build_container:recipe_wrapper.sh"), "recipe_wrapper.sh") | ||
|
|
@@ -72,11 +78,25 @@ def _build_recipe_repository_impl(ctxt): | |
| ctxt.symlink(Label("//ci/prebuilt:BUILD"), "BUILD") | ||
|
|
||
| # Run the build script. | ||
| command = [] | ||
| env = {} | ||
| if ctxt.os.name.upper().startswith("WINDOWS"): | ||
| vc_path = find_vc_path(ctxt) | ||
| 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" | ||
|
There was a problem hiding this comment. 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. |
||
| env["CXX"]="cl" | ||
| env["CXXFLAGS"] = "-DNDEBUG" | ||
| env["CFLAGS"] = "-DNDEBUG" | ||
| command = ["./repositories.bat"] + ctxt.attr.recipes | ||
| else: | ||
| command = ["./repositories.sh"] + ctxt.attr.recipes | ||
|
|
||
| print("Fetching external dependencies...") | ||
| result = ctxt.execute( | ||
| command, | ||
| environment = env, | ||
| quiet = False, | ||
| ) | ||
| print(result.stdout) | ||
|
|
@@ -186,6 +206,10 @@ def _envoy_api_deps(): | |
| name = "http_api_protos", | ||
| actual = "@googleapis//:http_api_protos", | ||
| ) | ||
| native.bind( | ||
| name = "http_api_protos_native", | ||
| actual = "@googleapis//:http_api_protos_native", | ||
| ) | ||
| _repository_impl( | ||
| name = "six_archive", | ||
| build_file = "@com_google_protobuf//:six.BUILD", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,13 +6,23 @@ export COMMIT="e1c3a83b8197cf02e794f61228461c27d4e78cfb" # benchmark @ Jan 11, | |
| git clone https://github.com/google/benchmark.git | ||
| (cd benchmark; git reset --hard "$COMMIT") | ||
| mkdir build | ||
| cd build | ||
|
|
||
| cmake_generator="Unix Makefiles" | ||
| make_cmd=make | ||
| benchmark_lib="libbenchmark.a" | ||
|
|
||
| if [[ "${OS}" == "Windows_NT" ]]; then | ||
|
There was a problem hiding this comment.
Which Bash do you expect to run this script under? (Same question goes for other shell scripts that use There was a problem hiding this comment. We expect this to run under MSYS2 Bash There was a problem hiding this comment. @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? There was a problem hiding this comment. Would it make sense to PR the build documentation when we PR source code changes? As it stands, Envoy won't actually build There was a problem hiding this comment. 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. There was a problem hiding this comment. OK, we'll follow up the source PR with a docs PR |
||
| cmake_generator="Ninja" | ||
| make_cmd=ninja | ||
|
There was a problem hiding this comment. Why Ninja? Is there no way to do GNU make on Win? There was a problem hiding this comment. 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 There was a problem hiding this comment. 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. There was a problem hiding this comment. For benchmark at least, we were not able to get it to work with GNU make |
||
| benchmark_lib="benchmark.lib" | ||
| fi | ||
|
|
||
| cmake -G "$cmake_generator" ../benchmark \ | ||
| -DCMAKE_BUILD_TYPE=RELEASE \ | ||
| -DBENCHMARK_ENABLE_GTEST_TESTS=OFF | ||
| $make_cmd | ||
|
|
||
| cp "src/$benchmark_lib" "$THIRDPARTY_BUILD"/lib | ||
| cd ../benchmark | ||
|
|
||
| INCLUDE_DIR="$THIRDPARTY_BUILD/include/testing/base/public" | ||
|
|
||
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.
So, without the PGV validation, there are two things we lose:
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.
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.
When building with the PGV validation, we ran into a few issues:
_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 scriptcc_proto_libraryrule from@com_google_protobuf//:protobuf.bzldid not de-duplicate command line arguments, causing it to hit the 32,768 character command line limit. The native Bazelcc_proto_libraryrule does not have this issue.So it looks like the way forward would be something like:
_go_proto_library_gen_implso it generates valid scripts for both Linux and Windowscc_proto_libraryrules.From the comment in
api/bazel/api_build_system.bzl, it looks like the third step might not be possible at the momentThere 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.
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.