Skip to content

build: Envoy has release builds on Windows#5006

Closed
sesmith177 wants to merge 1 commit intoenvoyproxy:masterfrom
greenhouse-org:release-build-windows
Closed

build: Envoy has release builds on Windows#5006
sesmith177 wants to merge 1 commit intoenvoyproxy:masterfrom
greenhouse-org:release-build-windows

Conversation

@sesmith177
Copy link
Member

Description:

Add ability to create a release build of Envoy on Windows.

The only tricky part here is that the dependencies built by the scripts in ci/build_container/build_recipes need to know whether the build is a release build or a debug build -- otherwise they will not link properly. The solution we chose was to use a BAZEL_WINDOWS_BUILD_TYPE environment variable which would be set by the ci/do_ci.ps1 script. The makefile is also updated so that these dependencies are rebuilt when switching between release + debug

Risk Level:
Low
Testing:
bazel build //source/... && bazel test //test/.... We also switched between release + debug builds on our Windows fork to verify this all worked as expected
Docs Changes:
None
Release Notes:
N/A

Signed-off-by: Sam Smith <sesmith177@gmail.com>
Signed-off-by: Arjun Sreedharan <asreedharan@pivotal.io>
CXXFLAGS ?= -ggdb3 -fno-omit-frame-pointer -O2
CFLAGS ?= -ggdb3 -fno-omit-frame-pointer -O2
CPPFLAGS ?= -DNDEBUG
BAZEL_WINDOWS_BUILD_TYPE ?= opt
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we set this in https://github.com/envoyproxy/envoy/blob/master/bazel/repositories.bzl#L88 based on the config_setting for Windows build? I.e. not have to specify it manually? Even better, I would name this BAZEL_WINDOWS_DEBUG_BUILD to make it clear, I think that's the key thing you want to extract from the config_setting here.

@sesmith177
Copy link
Member Author

@htuch It looks like it is difficult for a a repository_rule to depend on the config -- select statements can't be used in WORKSPACE file and we were not able to access the cc_toolchain.

However, we did take another look at #4516 and we were able to get both release + debug builds working on Windows. If we think that PR is the direction we are going to be headed, it may make sense to hold off on this for now

@htuch
Copy link
Member

htuch commented Nov 12, 2018

@sesmith177 yeah, #4516 is where we want to go. The main sticking point is having the features required move out of experimental, but we could probably move the Windows port to using this right now.

Are all the external deps you care about that need this support cmake based and can work with #4516? Or do we still need some additional work to be done for autoconf and ahoc make-based external deps?

@sesmith177
Copy link
Member Author

I think so. Of the external deps, that we still build with shell scripts, the only two that are not CMake-based are luajit + gperftools. It looks like luajit is only used by the lua filter (which we don't build) and gperftools doesn't even build on Windows

@htuch
Copy link
Member

htuch commented Nov 13, 2018

@sesmith177 SGTM. I'll try and get #4516 to cover all external cmake then and we can make it somehow configurable to use that (and you can default it for the Windows build). I'll probably get around to this later this week, if you want it faster, take the cookie :)

@sesmith177
Copy link
Member Author

@htuch we worked off #4516 to get all the external cmake deps building on Windows + Linux: https://github.com/greenhouse-org/envoy/commit/b3f26639c57c402956b46d25033c552fdf59979f

We probably will not get around to making it configurable (external cmake vs. build_recipes shell scripts), though

@htuch
Copy link
Member

htuch commented Nov 15, 2018

@sesmith177 thanks for the reference, I will base on this. This is an awesome start, I think a small cleanup and configuration changes and we're there.

@stale
Copy link

stale bot commented Nov 22, 2018

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!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Nov 22, 2018
@sesmith177
Copy link
Member Author

This PR probably doesn't need to be open any more, since it looks like we're going with the external cmake strategy.

@sesmith177 sesmith177 closed this Nov 28, 2018
@sesmith177 sesmith177 deleted the release-build-windows branch November 28, 2018 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale stalebot believes this issue/PR has not been touched recently

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants