-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
build: bazel changes to build on Windows #3786
Changes from 7 commits
e2ab992
5bd4ab1
1f88b54
a8eefb8
80b38b3
19e4002
91a8799
6ad7c28
f1a0385
b5479cf
e786114
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
pinging @katre to answer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 | ||
%BAZEL_SH% -c "./repositories.sh %*" | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @meteorcloudy: do you think it's OK to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's fine. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The question is why do we need to setup the MSVC environment variables? @sesmith177 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. | ||
environment = {} | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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( | ||
["./repositories.sh"] + ctxt.attr.recipes, | ||
environment = environment, | ||
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 -G "Unix Makefiles" ../benchmark \ | ||
|
||
cmake_generator="Unix Makefiles" | ||
make_cmd=make | ||
benchmark_lib="libbenchmark.a" | ||
|
||
if [[ "${OS}" == "Windows_NT" ]]; then | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We expect this to run under MSYS2 Bash There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why Ninja? Is there no way to do GNU make on Win? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 | ||
cp src/libbenchmark.a "$THIRDPARTY_BUILD"/lib | ||
"$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_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 Bazelcc_proto_library
rule does not have this issue.So it looks like the way forward would be something like:
_go_proto_library_gen_impl
so it generates valid scripts for both Linux and Windowscc_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 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.