-
Notifications
You must be signed in to change notification settings - Fork 5.5k
build: external deps build on Windows #3892
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
Changes from 4 commits
ab10663
2a8c39e
b1d74d6
3c3724e
f80ec45
daaf5a1
de70604
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 |
|---|---|---|
|
|
@@ -7,11 +7,40 @@ set -e | |
| # TODO(PiotrSikora): switch back to releases once v1.33.0 is out. | ||
| VERSION=e5b3f9addd49bca27e2f99c5c65a564eb5c0cf6d # 2018-06-09 | ||
|
|
||
| wget -O nghttp2-"$VERSION".tar.gz https://github.com/nghttp2/nghttp2/archive/"$VERSION".tar.gz | ||
| curl https://github.com/nghttp2/nghttp2/archive/"$VERSION".tar.gz -sLo nghttp2-"$VERSION".tar.gz | ||
| tar xf nghttp2-"$VERSION".tar.gz | ||
| cd nghttp2-"$VERSION" | ||
| autoreconf -i | ||
| automake | ||
| autoconf | ||
| ./configure --prefix="$THIRDPARTY_BUILD" --enable-shared=no --enable-lib-only | ||
| make V=1 install | ||
|
|
||
| # Allow nghttp2 to build as static lib on Windows | ||
| cat > nghttp2_cmakelists.diff << 'EOF' | ||
|
Member
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 upstream this? Or at least add a TODO for this?
Member
Author
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 made a PR here: nghttp2/nghttp2#1198, we can add a TODO to remove if /when it is merged |
||
| diff --git a/lib/CMakeLists.txt b/lib/CMakeLists.txt | ||
| index 17e422b2..e58070f5 100644 | ||
| --- a/lib/CMakeLists.txt | ||
| +++ b/lib/CMakeLists.txt | ||
| @@ -56,6 +56,7 @@ if(HAVE_CUNIT OR ENABLE_STATIC_LIB) | ||
| COMPILE_FLAGS "${WARNCFLAGS}" | ||
| VERSION ${LT_VERSION} SOVERSION ${LT_SOVERSION} | ||
| ARCHIVE_OUTPUT_NAME nghttp2 | ||
| + ARCHIVE_OUTPUT_DIRECTORY static | ||
| ) | ||
| target_compile_definitions(nghttp2_static PUBLIC "-DNGHTTP2_STATICLIB") | ||
| if(ENABLE_STATIC_LIB) | ||
| EOF | ||
|
|
||
| if [[ "${OS}" == "Windows_NT" ]]; then | ||
| git apply nghttp2_cmakelists.diff | ||
|
Member
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 just use
Member
Author
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.
|
||
| fi | ||
|
|
||
| mkdir build | ||
| cd build | ||
|
|
||
| cmake -G "Ninja" -DCMAKE_INSTALL_PREFIX="$THIRDPARTY_BUILD" \ | ||
| -DENABLE_STATIC_LIB=on \ | ||
| -DENABLE_LIB_ONLY=on \ | ||
| .. | ||
| ninja | ||
| ninja install | ||
|
|
||
| if [[ "${OS}" == "Windows_NT" ]]; then | ||
| cp "lib/CMakeFiles/nghttp2_static.dir/nghttp2_static.pdb" "$THIRDPARTY_BUILD/lib/nghttp2_static.pdb" | ||
| fi | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,11 +4,26 @@ set -e | |
|
|
||
| VERSION=0.6.2 | ||
|
|
||
| wget -O yaml-cpp-"$VERSION".tar.gz https://github.com/jbeder/yaml-cpp/archive/yaml-cpp-"$VERSION".tar.gz | ||
| curl https://github.com/jbeder/yaml-cpp/archive/yaml-cpp-"$VERSION".tar.gz -sLo yaml-cpp-"$VERSION".tar.gz | ||
| tar xf yaml-cpp-"$VERSION".tar.gz | ||
| cd yaml-cpp-yaml-cpp-"$VERSION" | ||
| cmake -DCMAKE_INSTALL_PREFIX:PATH="$THIRDPARTY_BUILD" \ | ||
|
|
||
| mkdir build | ||
| cd build | ||
|
|
||
| build_type=RelWithDebInfo | ||
| if [[ "${OS}" == "Windows_NT" ]]; then | ||
| build_type=Debug | ||
|
Member
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 do we have these debug exceptions for Windows everywhere?
Member
Author
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. 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 Right now, we've only got bazel building Envoy with
Member
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. Please then add some comments to this effect at these sites, thanks. |
||
| fi | ||
|
|
||
| cmake -G "Ninja" -DCMAKE_INSTALL_PREFIX:PATH="$THIRDPARTY_BUILD" \ | ||
| -DCMAKE_CXX_FLAGS:STRING="${CXXFLAGS} ${CPPFLAGS}" \ | ||
| -DCMAKE_C_FLAGS:STRING="${CFLAGS} ${CPPFLAGS}" \ | ||
| -DCMAKE_BUILD_TYPE=RelWithDebInfo . | ||
| make VERBOSE=1 install | ||
| -DYAML_CPP_BUILD_TESTS=off \ | ||
| -DCMAKE_BUILD_TYPE="$build_type" \ | ||
| .. | ||
| ninja install | ||
|
|
||
| if [[ "${OS}" == "Windows_NT" ]]; then | ||
| cp "CMakeFiles/yaml-cpp.dir/yaml-cpp.pdb" "$THIRDPARTY_BUILD/lib/yaml-cpp.pdb" | ||
| fi | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,8 +24,9 @@ cc_library( | |
| ) | ||
|
|
||
| cc_library( | ||
| # 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"]), | ||
|
Member
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 glob?
Member
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. See context above, ninja build doesn't produce
Member
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 a comment explaining this reasoning would then be appropriate. |
||
| deps = [":event"], | ||
| ) | ||
|
|
||
|
|
||
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'm wondering if there have been any noticeable changes in performance or build object when switching from the
configureflags like--enable-optimizeto thebuild_typehere.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.
Using
configure, it looks like the compiler flags were:using
ninja, it looks like the compiler flags were:We got these compiler flags by running
objdump --dwarf libcares.a | grep DW_AT_producerWhat is the usual way to catch a performance regression?
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 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 (?).
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.
Done