-
Notifications
You must be signed in to change notification settings - Fork 5.3k
build: external_cmake for cmake deps. #5218
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 all commits
662e76f
2fb2961
1c88f27
3554527
509d8cd
4d2adea
8e7747e
344ed29
2e500bb
97879c2
c5aadf8
4355c7a
8b4c009
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 |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| licenses(["notice"]) # Apache 2 | ||
|
|
||
| load("//bazel:envoy_build_system.bzl", "envoy_package") | ||
| load("@rules_foreign_cc//tools/build_defs:cmake.bzl", "cmake_external") | ||
|
|
||
| envoy_package() | ||
|
|
||
| cmake_external( | ||
| name = "ares", | ||
| cache_entries = { | ||
| "CARES_SHARED": "no", | ||
| "CARES_STATIC": "on", | ||
| "CMAKE_BUILD_TYPE": "RelWithDebInfo", | ||
| }, | ||
| cmake_options = ["-GNinja"], | ||
|
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. We need to set
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. Ditto. |
||
| lib_source = "@com_github_c_ares_c_ares//:all", | ||
| make_commands = [ | ||
| "ninja", | ||
| "ninja install", | ||
htuch marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ], | ||
|
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. We need to copy pdb files on Windows debug builds; see: https://github.com/greenhouse-org/envoy/blob/66776969519e993ea022bb385947897208704abe/bazel/foreign_cc/BUILD#L55-L58
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. Ack. Let's merge as-is and you can do a small diff PR for this. I can't verify these steps, so will let you folks take this on.
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. Not a problem -- we can submit a followup PR that cleans up the Windows bits |
||
| static_libraries = ["libcares.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. The output static library files are going to be different on Windows and Linux
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. Ditto. |
||
| ) | ||
|
|
||
| cmake_external( | ||
| name = "event", | ||
| cache_entries = { | ||
| "EVENT__DISABLE_OPENSSL": "on", | ||
| "EVENT__DISABLE_REGRESS": "on", | ||
| "CMAKE_BUILD_TYPE": "Release", | ||
| }, | ||
| cmake_options = ["-GNinja"], | ||
| lib_source = "@com_github_libevent_libevent//:all", | ||
| make_commands = [ | ||
| "ninja", | ||
| "ninja install", | ||
| ], | ||
| static_libraries = ["libevent.a"], | ||
| ) | ||
|
|
||
| cmake_external( | ||
| name = "nghttp2", | ||
| cache_entries = { | ||
|
Contributor
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. Add |
||
| "ENABLE_STATIC_LIB": "on", | ||
| "ENABLE_LIB_ONLY": "on", | ||
| "CMAKE_BUILD_TYPE": "RelWithDebInfo", | ||
| }, | ||
| cmake_options = ["-GNinja"], | ||
| lib_source = "@com_github_nghttp2_nghttp2//:all", | ||
| make_commands = [ | ||
| "ninja", | ||
| "ninja install", | ||
| ], | ||
| static_libraries = ["libnghttp2.a"], | ||
| ) | ||
|
|
||
| cmake_external( | ||
| name = "yaml", | ||
| cache_entries = { | ||
| "YAML_CPP_BUILD_TESTS": "off", | ||
|
Contributor
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. Add |
||
| "CMAKE_BUILD_TYPE": "RelWithDebInfo", | ||
| }, | ||
| cmake_options = ["-GNinja"], | ||
| lib_source = "@com_github_jbeder_yaml_cpp//:all", | ||
| make_commands = [ | ||
| "ninja", | ||
| "ninja install", | ||
| ], | ||
| static_libraries = ["libyaml-cpp.a"], | ||
| ) | ||
|
|
||
| cmake_external( | ||
| name = "zlib", | ||
| cache_entries = { | ||
| "CMAKE_BUILD_TYPE": "RelWithDebInfo", | ||
| }, | ||
| cmake_options = ["-GNinja"], | ||
| lib_source = "@com_github_madler_zlib//:all", | ||
| make_commands = [ | ||
| "ninja", | ||
| "ninja install", | ||
| ], | ||
| static_libraries = ["libz.a"], | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| diff --git a/lib/CMakeLists.txt b/lib/CMakeLists.txt | ||
| index 17e422b2..e58070f5 100644 | ||
| --- a/lib/CMakeLists.txt | ||
| +++ b/lib/CMakeLists.txt | ||
| @@ -52,6 +52,7 @@ | ||
| COMPILE_FLAGS "${WARNCFLAGS}" | ||
| VERSION ${LT_VERSION} SOVERSION ${LT_SOVERSION} | ||
| ARCHIVE_OUTPUT_NAME nghttp2 | ||
| + ARCHIVE_OUTPUT_DIRECTORY static | ||
|
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. This diff isn't going to work -- it builds the static nghttp2.lib in a different directory from the dynamic nghttp2.lib, but when To get this to work we updated our patch: https://github.com/greenhouse-org/envoy/blob/66776969519e993ea022bb385947897208704abe/bazel/foreign_cc/nghttp2.patch and associated PR: nghttp2/nghttp2#1198
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. Ack. I'm going to go ahead with the PR as-is and we can update this when the nghttp2 patch merges. |
||
| ) | ||
| target_compile_definitions(nghttp2_static PUBLIC "-DNGHTTP2_STATICLIB") | ||
| if(ENABLE_STATIC_LIB) | ||
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.
Add
"CMAKE_BUILD_TYPE": "RelWithDebInfo"(possibly"Debug"for Windows builds?)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, but I'm going to skip the Windows parts and let @sesmith177 take this.