build: external deps build on Windows by sesmith177 · Pull Request #3892 · envoyproxy/envoy

sesmith177 · 2018-07-18T20:32:09Z

Description:

This PR is a continuation of breaking up #3786 into smaller chunks. It:

Converts all external deps (excluding gperftools and luajit which do not support cmake) to use Ninja instead of make
Ensures these deps build on Windows. This involves using curl instead of wget and copying *.pdb files to the appropriate location

In the process of switching from make to Ninja, libevent.sh now outputs just a libevent.a archive on Linux - a separate libevent_pthreads.a archive is no longer created or necessary.

Risk Level:
~~Low~~
After discussion here: #3892 (comment) changing risk level to medium (potential for performance regression in c-ares)
Testing:
Ran bazel build //source/exe:envoy-static and bazel test //test/... on Linux
Docs Changes:
None
Release Notes:
None

This is in preparation for building these dependencies on Windows. gperftools and luajit do not support cmake, so they are unchanged. Note: libevent now outputs only one libevent.a archive (that includes the evthread object files), there is no longer a separate libevent_pthreads.a archive Signed-off-by: Sam Smith <sesmith177@gmail.com>

Signed-off-by: Akshat Gokhale <agokhale@pivotal.io>

sesmith177 · 2018-07-18T21:04:31Z

We think that the CI failures are due to the fact that libevent.a in the build container does not contain the evthread* object files, but the libevent.a generated by Ninja does

lizan · 2018-07-19T20:05:29Z

We think that the CI failures are due to the fact that libevent.a in the build container does not contain the evthread* object files, but the libevent.a generated by Ninja does

Then you might want to split this PR into 2, one for sh to use ninja, and update the builder container, then update BUILD files.

- will follow up in separate PR Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Akshat Gokhale <agokhale@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

lizan · 2018-07-19T23:01:09Z

Can you temporarily add a glob (with a TODO to fix that) to https://github.com/envoyproxy/envoy/blob/master/ci/prebuilt/BUILD#L28 ? This will fix Mac CI and CI runs after updating build container.

- so builds will pass before and after the .sh file changes Signed-off-by: Sam Smith <sesmith177@gmail.com>

htuch

Looks, good, a few comments.

htuch · 2018-07-24T17:14:11Z

ci/build_container/build_recipes/nghttp2.sh

-make V=1 install
+
+# Allow nghttp2 to build as static lib on Windows
+cat > nghttp2_cmakelists.diff << 'EOF'


Can you upstream this? Or at least add a TODO for this?

We made a PR here: nghttp2/nghttp2#1198, we can add a TODO to remove if /when it is merged

htuch · 2018-07-24T17:14:36Z

ci/build_container/build_recipes/nghttp2.sh

+EOF
+
+if [[ "${OS}" == "Windows_NT" ]]; then
+  git apply nghttp2_cmakelists.diff


Can you just use patch for this?

patch doesn't come installed by default with MSYS on Windows

htuch · 2018-07-24T17:14:55Z

ci/build_container/build_recipes/yaml-cpp.sh

+
+build_type=RelWithDebInfo
+if [[ "${OS}" == "Windows_NT" ]]; then
+  build_type=Debug


Why do we have these debug exceptions for Windows everywhere?

Envoy and all of its dependencies need to be compiled to use the same version of the C Runtime Library: https://docs.microsoft.com/en-us/cpp/build/reference/md-mt-ld-use-run-time-library

If we build Envoy with -c dbg, it will use the Debug version of the C Runtime Library. We needed to set this option to force these dependencies to use the Debug library as well.

Right now, we've only got bazel building Envoy with -c dbg -- we haven't had a chance to get fastbuild or opt working. When we do that, there will probably have to be some more logic to select the appropriate build type

Please then add some comments to this effect at these sites, thanks.

htuch · 2018-07-24T17:15:16Z

ci/prebuilt/BUILD

+    # TODO: Remove 'event_pthreads' once no BUIlD files use it
    name = "event_pthreads",
-    srcs = ["thirdparty_build/lib/libevent_pthreads.a"],
+    srcs = glob(["thirdparty_build/lib/libevent_pthreads.a"]),


See context above, ninja build doesn't produce libevent_pthreads.a and all symbols are included in libevent.a, but the before the CI image update it have to include libevent_pthreads.a from prebuilt, glob here is a hack to make CI passes before #3909.

I think a comment explaining this reasoning would then be appropriate.

htuch · 2018-07-24T17:16:15Z

ci/build_container/build_recipes/cares.sh

+mkdir build
+cd build
+
+build_type=RelWithDebInfo


I'm wondering if there have been any noticeable changes in performance or build object when switching from the configure flags like --enable-optimize to the build_type here.

Using configure, it looks like the compiler flags were:

<c> DW_AT_producer : (indirect string, offset: 0x6747): GNU C11 5.4.0 20160609 -mtune=generic -march=x86-64 -ggdb3 -g -O2 -fvisibility= hidden -fno-omit-frame-pointer -fPIC -fstack-protector-strong

using ninja, it looks like the compiler flags were:

<c> DW_AT_producer : (indirect string, offset: 0x602f): GNU C11 5.4.0 20160609 -mtune=generic -march=x86-64 -ggdb3 -g -O2 -O2 -fno-omit-frame-pointer -fstack-protector-strong

We got these compiler flags by running objdump --dwarf libcares.a | grep DW_AT_producer

What is the usual way to catch a performance regression?

We don't have a standardized way to do this. I would flag this in the "Risk level" as medium and point out that there is potential for performance regression. I think it should be fine based on those flags (?).

Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Arjun Sreedharan <asreedharan@pivotal.io>

Signed-off-by: Sam Smith <sesmith177@gmail.com>

htuch

Thanks for the comments, this looks good to ship. Appreciate that you took the time to convert us over to ninja everywhere.

As requested in #3892, breaking the BUILD file changes out into a separate PR Risk Level: Low Testing: bazel build //source/exe:envoy-static and bazel test //test/... on Linux Docs Changes: None Release Notes: None Signed-off-by: Sam Smith <sesmith177@gmail.com>

Following PR envoyproxy#3892, the build uses `curl` instead of `wget`. As a result, package curl has become a dependency. Signed-off-by: Tal Nordan <tal.nordan@solo.io>

Following PR #3892, the build uses `curl` instead of `wget`. As a result, package curl has become a dependency. Risk Level: Low Testing: Bazel build on Ubuntu 18.04 Docs Changes: N/A Release Notes: N/A Signed-off-by: Tal Nordan <tal.nordan@solo.io>

ajgokhale and others added 2 commits July 18, 2018 14:56

External deps now build on Windows

2a8c39e

Signed-off-by: Akshat Gokhale <agokhale@pivotal.io>

Don't change build files

b1d74d6

- will follow up in separate PR Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Akshat Gokhale <agokhale@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

sesmith177 mentioned this pull request Jul 19, 2018

build: BUILD file changes necessary for #3892 #3909

Merged

Glob libevent_pthreads

3c3724e

- so builds will pass before and after the .sh file changes Signed-off-by: Sam Smith <sesmith177@gmail.com>

lizan previously approved these changes Jul 20, 2018

View reviewed changes

lizan requested a review from htuch July 20, 2018 20:12

junr03 assigned htuch Jul 20, 2018

htuch suggested changes Jul 24, 2018

View reviewed changes

Add TODO referencing PR

f80ec45

Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Arjun Sreedharan <asreedharan@pivotal.io>

sesmith177 dismissed lizan’s stale review via f80ec45 July 24, 2018 17:44

sesmith177 added 2 commits July 24, 2018 17:56

Add comment explaining glob

daaf5a1

Signed-off-by: Sam Smith <sesmith177@gmail.com>

Explain Debug builds for external deps on Windows

de70604

Signed-off-by: Sam Smith <sesmith177@gmail.com>

htuch approved these changes Jul 24, 2018

View reviewed changes

htuch merged commit 1f445bd into envoyproxy:master Jul 24, 2018

talnordan mentioned this pull request Aug 10, 2018

docs: add curl to Ubuntu deps #4104

Merged

sesmith177 deleted the windows-ninja branch October 5, 2018 16:14

Conversation

sesmith177 commented Jul 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sesmith177 commented Jul 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lizan commented Jul 19, 2018

Uh oh!

lizan commented Jul 19, 2018

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sesmith177 commented Jul 18, 2018 •

edited

Loading

sesmith177 commented Jul 18, 2018 •

edited

Loading