-
Notifications
You must be signed in to change notification settings - Fork 5.2k
windows: fix Envoy build on Windows #42687
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
base: main
Are you sure you want to change the base?
Conversation
|
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to |
| SysCallIntResult OsSysCallsImpl::setsockopt(os_fd_t sockfd, int level, int optname, | ||
| const void* optval, socklen_t optlen) { | ||
| if (optname == IP_RECVTOS || optname == IPV6_RECVTCLASS) { | ||
| const int rc = ::WSASetRecvIPEcn(sockfd, *(int*)optval == 1); |
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.
Only available on newer Windows
bazel/foreign_cc/luajit.patch
Outdated
| new file mode 100755 | ||
| index 00000000..8fb0fbe8 | ||
| --- /dev/null | ||
| +++ b/luajit_build_win.sh |
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.
Shell version of old build.py that was deleted in refactoring
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.
this should be in a separate patch - ie luajit_win.patch
the reason being is that anyon updating can just disable that patch easily/explicitly if it doesnt apply cleanly
we are not super consistent about this - but that is how we have been handling other arches recently
$ ls bazel/*ppc*
bazel/highway-ppc64le.patch bazel/rules_java_ppc64le.patch bazel/v8_ppc64le.patch
bazel/foreign_cc/cel-cpp.patch
Outdated
| - args.use_param_file(param_file_arg = "%s", use_always = True) | ||
| - args.add_all(transitive_descriptor_sets) | ||
| + | ||
| + # Generate script to concatenate files listed in a parameter file into an output file. |
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.
Relatively large change
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.
can you upstream this its hard enough maintaining lots of patches, and even harder if its for untested OS
any windows specific patches should be in a separate clearly named patch - so maintainers can quickly disable it if it doesnt apply, and it doesnt affect any other builds
bazel/repo.bzl
Outdated
| tag = config_data["build-image"]["tag"], | ||
| )) | ||
| else: | ||
| # yq via bazel doesn't work on Windows, but we don't need the file either |
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 downloads something but doesn't symlink or copy it in a way that it can be run, probably a bazel rule bug in @yq but it doesn't matter here
source/extensions/bootstrap/reverse_tunnel/upstream_socket_interface/reverse_tunnel_acceptor.h
Show resolved
Hide resolved
| "//envoy/filesystem:filesystem_interface", | ||
| "//source/common/common:logger_lib", | ||
| "//source/common/filesystem:filesystem_lib", | ||
| "//source/common/singleton:threadsafe_singleton", |
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.
Something about clang-cl seems to detect more of these missing dependencies (correctly)
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
60681f4 to
1d6c084
Compare
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
1d6c084 to
6fcbc41
Compare
b317301 to
8b02976
Compare
phlax
left a comment
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.
Windows CI won't be restored,
that is correct, and the related point is that developers have no obligation to make things work or keep things working
patches like this are still accepted.
we can accept patches to make it work if they are low maintenance and non-intrusive - but patches for upstream really must be upstreamed
@anuraaga im wondering what the motivation for this is - ie are you imagining envoy being deployed on windows somewhere?
if that is the case, and putting aside my own thoughts on whether that is a good idea, my strong suggestion would be to iniitiate a downstream project that kept up to date and did any ci/testing that is required to maintain this
bazel/foreign_cc/cel-cpp.patch
Outdated
| - args.use_param_file(param_file_arg = "%s", use_always = True) | ||
| - args.add_all(transitive_descriptor_sets) | ||
| + | ||
| + # Generate script to concatenate files listed in a parameter file into an output file. |
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.
can you upstream this its hard enough maintaining lots of patches, and even harder if its for untested OS
any windows specific patches should be in a separate clearly named patch - so maintainers can quickly disable it if it doesnt apply, and it doesnt affect any other builds
bazel/foreign_cc/luajit.patch
Outdated
| + | ||
| + ls | ||
| + cd src | ||
| +./msvcbuild.bat static |
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.
in this cases its obviously harder to upstream the patch - but im wondering if this .bat stuff is really needed and if it is whether it can be incorporated to existing patch/script - eg by setting an env var and calling it here if set
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.
I considered adding to the existing script but since it's not using make, most of the logic is actually replicating the install command. It seemed clearer to keep them separate
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.
I was able to remove the .bat patch, instead making sure to copy vc140.pdb to the output prefix, which is required when using /Zi as msvcbuild.bat forces to use.
I still kept the separate build_win.sh since it seems worth keeping it independent from the unix one
| create_compiler_config_setting( | ||
| name = "config_msvc", | ||
| - value = "msvc-cl", | ||
| + values = ["msvc-cl", "clang-cl"], |
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.
please upstream all of this
tools/BUILD
Outdated
| "check_repositories.sh", | ||
| ]) | ||
|
|
||
| py_binary( |
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.
move this into tools/windows if its not relevant to anyone else
Not sure if I follow exactly but yes, this is the linked pyvoy. And yeah, I don't expect Envoy to bring back any CI and will need to do testing. I am wondering what part of this change is acceptable to you - if it's only the cc/h files that would still be helpful and I will just keep the build code forked. As we know, most of these dependencies aren't well maintained so while I can try to upstream changes, generally it won't work. |
i meant something like an ideally we make as few changes as possible wrt maintaining win-specific changes - i think the changes to our cc/h are unaviodable, and similar with some of the bazel selects etc - but for unsupported setups its even more important to upstream upstream changes as it significantly complicates maintaining patches @TAOXUY @dchakarwarti would you be able to review a PR to upstream these changes? |
Ah I see. That would be the workflow I plan on running in py-envoy-server which just packages Envoy for use in Python. It builds envoy and runs some tests. Not a full suite but it checks basic functionality. I don't have a big reason to separate out yet another project specifically for envoy-on-windows but in terms of the envoy repo I think it's effectively the same.
Yeah I agree that it can get hard to recreate them. TBH I was already prepping a PR for protoconverter before finding that stale PR. I'm always for upstreaming where I can. Is there any model where upstream patches with windows code can be made knowing that updates to the patch may break? I'm definitely not expecting the code to be stable for Windows and would just need to be able to react, not proact. But if even that's too much can keep those patches out of this PR. |
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
anuraaga
left a comment
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.
I was able to simplify the build patches, sent grpc-ecosystem/proto-converter#7 in case it may be merged.
cel-cpp seems to have coincidentally started looking into Windows issues recently
google/cel-cpp#1811
google/cel-cpp@f1a2e8e
The now one-line patch here seems to work for now and in the future it should be removable.
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
anuraaga
left a comment
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.
@phlax I was able to simplify the upstream patches a lot with pointers to upstreaming. Hopefully this looks better now
bazel/foreign_cc/luajit.patch
Outdated
| + | ||
| + ls | ||
| + cd src | ||
| +./msvcbuild.bat static |
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.
I was able to remove the .bat patch, instead making sure to copy vc140.pdb to the output prefix, which is required when using /Zi as msvcbuild.bat forces to use.
I still kept the separate build_win.sh since it seems worth keeping it independent from the unix one
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
|
Ping for @wbpcode - but it's vacation season so mostly this ping is to shush the tool that nags on-call about stalled PRs, we don't expect prompt reviews across vacations. |
00bdf7c to
b22a8e4
Compare
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
b22a8e4 to
c6f5fc6
Compare
phlax
left a comment
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.
@anuraaga i would feel a lot more comfortable supporting this if there were a downstream project - ie more than one user
either way - my main goals here are
- prevent lots of cruft that is not used (which we dont know if theres no ci)
- avoid adding complexity
- ensure it doesnt add any maintenance burden to devs
the last point is crucial - its why we dropped support previously (that and terrible ci)
bazel/foreign_cc/cel-cpp.patch
Outdated
| progress_message = "Joining descriptors.", | ||
| command = ("< \"$1\" xargs cat >{output}".format(output = output.path)), | ||
| arguments = [args], | ||
| + use_default_shell_env = True, |
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.
can this be upstreamed?
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.
A much larger patch upstream to remove shell usage was already merged, along with some other Windows fixes, so once the next version of cel-cpp is released and updated to, it should work fine without patching. So I have removed this patch from the PR and will maintain it downstream until then so it's easier to delete
bazel/foreign_cc/luajit.patch
Outdated
| new file mode 100755 | ||
| index 00000000..8fb0fbe8 | ||
| --- /dev/null | ||
| +++ b/luajit_build_win.sh |
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.
this should be in a separate patch - ie luajit_win.patch
the reason being is that anyon updating can just disable that patch easily/explicitly if it doesnt apply cleanly
we are not super consistent about this - but that is how we have been handling other arches recently
$ ls bazel/*ppc*
bazel/highway-ppc64le.patch bazel/rules_java_ppc64le.patch bazel/v8_ppc64le.patch| @@ -1,3 +1,32 @@ | |||
| diff --git a/build_defs/BUILD.bazel b/build_defs/BUILD.bazel | |||
| index 732514d..a6be18b 100644 | |||
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.
same with this patch - please put it to a win name
bazel/repo.bzl
Outdated
| )) | ||
| else: | ||
| # yq via bazel doesn't work on Windows, but we don't need the file either | ||
| repository_ctx.file("containers.bzl", CONTAINERS.format( |
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.
pretty sure we dont need to repeat this
wondering if this file is needed at all in the windows case, assuming it is - then probably it can just be empty
if for some reason that doesnt work - i think its marginally cleaner to do something like
repo = ""
repo_gcr = ""
...
if not windows condition:
set vars from parsed config
repository_ctx.file(...)
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.
IIUC, we can't avoid it since the values are directly loaded during repo and toolchain resolution
https://github.com/envoyproxy/envoy/blob/main/bazel/platforms/rbe/BUILD#L2
I think this is the reason we can't use genrule either. I went with the suggested style though since it does seem better
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
anuraaga
left a comment
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.
Thanks @phlax - I have applied the suggestions and hope it gets closer to a balance that doesn't get in the way of other developers. AFAIK, I am not introducing a new feature or paradigm but just fixing an existing one, best-effort building on Windows. Basically I am just trying to offer free help keeping the binary building on Windows, which I'll be using a nightly CI to do going forward too.
I think it's worth keeping in perspective that the Windows logic here also isn't that large among all the other tooling - unmaintained dependencies like proto-converter seem to be larger issues to clamp down on ;)
Anyways let me know what works for you and I will target it, I can maintain as much of this patch downstream as needed, including all of it if it's completely declined.
bazel/foreign_cc/cel-cpp.patch
Outdated
| progress_message = "Joining descriptors.", | ||
| command = ("< \"$1\" xargs cat >{output}".format(output = output.path)), | ||
| arguments = [args], | ||
| + use_default_shell_env = True, |
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.
A much larger patch upstream to remove shell usage was already merged, along with some other Windows fixes, so once the next version of cel-cpp is released and updated to, it should work fine without patching. So I have removed this patch from the PR and will maintain it downstream until then so it's easier to delete
bazel/repo.bzl
Outdated
| )) | ||
| else: | ||
| # yq via bazel doesn't work on Windows, but we don't need the file either | ||
| repository_ctx.file("containers.bzl", CONTAINERS.format( |
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.
IIUC, we can't avoid it since the values are directly loaded during repo and toolchain resolution
https://github.com/envoyproxy/envoy/blob/main/bazel/platforms/rbe/BUILD#L2
I think this is the reason we can't use genrule either. I went with the suggested style though since it does seem better
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
9a3f1ad to
a60d4d5
Compare
phlax
left a comment
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.
Commit Message: Fix source to allow building on Windows
Additional Description:
I am interested in using Envoy on Windows with dynamic modules - I have already published pyvoy 0.2.0 using a patchset against 1.36.4 which is working great. This PR now updates the latest main to allow building on Windows, which required quite some more digging to accomodate changes to hermetic toolchains since then. FWIU, while Windows CI won't be restored, patches like this are still accepted.
For reference, the magic incantation to invoke Bazel
https://github.com/curioswitch/py-envoy-server/pull/17/files#diff-2855bcd1d44f7c4a97fdb2d631b71cf271ac70f3918e8f6242e54cf038952e64R59
If this is merged, I plan on having that build run nightly to try to catch Windows issues post-merge. Of course, if Envoy team could accept a small CI for Windows, just building the binary but no tests, that would be nice :) But I don't expect it.
There are two changes to non-Windows codepaths
Otherwise, it is fixing some obvious syntax errors, including a dlopen wrapper around dynamic modules loading for Windows, replacing a convenience wrapper only available on newer windows with the older form, and other misc build tweaks.
/cc @mathetake @wbpcode
Risk Level:
Testing: CI
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A