-
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: Initial bazel build file changes for Windows #3884
Conversation
This just contains the "non-controversial changes" to get Envoy building on Windows. Future PRs will address: 1. getting external deps to build on Windows 2. getting PGV working on Windows Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Amin Jamali <ajamali@pivotal.io> Signed-off-by: Matthew Horan <mhoran@pivotal.io> Signed-off-by: Akshat Gokhale <agokhale@pivotal.io>
LGTM |
values = {"cpu": "x64_windows"}, | ||
) | ||
|
||
config_setting( |
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 do you define 3 different config_setting
rules for Windows for the compilation_mode
options? They are all used the same way, you could substitute them with //bazel:windows_x86_64
.
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.
We need the 3 different config_setting
rules for this select
here: https://github.com/greenhouse-org/envoy/blob/windows-build-pr/bazel/envoy_build_system.bzl#L36-L41
Specifically, we want to never append -ggdb3
to the copts
on Windows, but we want to keep the selection mechanism on Linux. If we just add //bazel:windows_x86_64
to that select
, it will fail since multiple options will be true (e.g. //bazel:windows_x86_64
and //bazel:dbg_build
).
As far as we know, this is the best way to select
on multiple conditions -- did we miss some other way to do this?
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.
Ah, I see. No, I think you're right. Thanks for the explanation!
ci/build_setup.ps1
Outdated
echo "ENVOY_BAZEL_ROOT: $env:ENVOY_BAZEL_ROOT" | ||
echo "ENVOY_SRCDIR: $env:ENVOY_SRCDIR" | ||
|
||
$env:BAZEL_BASE_OPTIONS="--nomaster_bazelrc --output_base=$env:ENVOY_BAZEL_ROOT --bazelrc=$env:ENVOY_SRCDIR\windows\tools\bazel.rc --batch" |
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.
--batch
is deprecated. You'll have to manually shut down the Bazel server afterwards.
ci/build_setup.ps1
Outdated
echo "ENVOY_SRCDIR: $env:ENVOY_SRCDIR" | ||
|
||
$env:BAZEL_BASE_OPTIONS="--nomaster_bazelrc --output_base=$env:ENVOY_BAZEL_ROOT --bazelrc=$env:ENVOY_SRCDIR\windows\tools\bazel.rc --batch" | ||
$env:BAZEL_BUILD_OPTIONS="--strategy=Genrule=standalone --spawn_strategy=standalone --verbose_failures --action_env=HOME --action_env=PYTHONUSERBASE --jobs=$env:NUM_CPUS --show_task_finish $env:BAZEL_BUILD_EXTRA_OPTIONS" |
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.
Does $HOME
have any value on Windows? (Also in BAZEL_TEST_OPTIONS
below.)
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.
It does not. It doesn't look like we need any of the other action_env
or test_env
environment variables on Windows, so we'll remove them as well
windows/setup/workstation_setup.ps1
Outdated
|
||
trap { $host.SetShouldExit(1) } | ||
|
||
Invoke-WebRequest -UseBasicParsing "https://aka.ms/vs/15/release/vs_buildtools.exe" -OutFile "$env:TEMP\vs_buildtools.exe" |
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.
FYI: For file downloads, Start-BitsTransfer
is faster than Invoke-WebRequest
. You may be able to use that.
windows/tools/bazel.rc
Outdated
@@ -0,0 +1,6 @@ | |||
# Windows/Envoy specific Bazel build/test options. | |||
|
|||
build --experimental_shortened_obj_file_path |
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.
FYI, this flag:
- is
false
by default in Bazel 0.15.2 - will be
true
by default in Bazel 0.16.0 - will be unsupported by Bazel 0.17.0
- --batch is deprecated - the --action_env and --test_env settings aren't necesssary on Windows Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Akshat Gokhale <agokhale@pivotal.io>
Signed-off-by: Akshat Gokhale <agokhale@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>
will be removed in future versions of Bazel Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Akshat Gokhale <agokhale@pivotal.io>
LGTM |
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.
Awesome, thanks.
@sesmith177 @htuch this broke the example filter:
|
@sesmith177 could you investigate? Thanks. |
@sesmith177 to repro (on Linux):
|
OK, I can repro |
It looks like some of the references to "//bazel:windows_x86_64" in https://github.com/envoyproxy/envoy/blob/master/bazel/envoy_build_system.bzl weren't properly prefixed with the repository name. I noticed that some select statements in that file hardcode "@envoy", e.g. envoy/bazel/envoy_build_system.bzl Line 87 in e89c9d6
envoy/bazel/envoy_build_system.bzl Line 35 in e89c9d6
Anyways, fixing these prefixes up allows the build to succeed. How should we proceed from here? |
I would use |
Done, submitted: #3947 |
@sesmith177 hmm still seeing this with our internal build (which operates like the filter example):
on my side... investigating... |
@sesmith177 ah, looks like we need to update our build container to include ninja. sorry for the false alarm. |
Description:
This is the first step in breaking up #3786 into smaller chunks. This contains the Bazel config / compiler options to allow Envoy to build on Windows. Future PRs will address the external deps build scripts and PGV.
Risk Level:
Low
Testing:
Ran
bazel build //source/exe:envoy-static
andbazel test //test/...
on LinuxDocs Changes:
None
Release Notes:
None