-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 4 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 |
|---|---|---|
|
|
@@ -17,6 +17,18 @@ build process. | |
| `external_deps` attribute. | ||
| 3. `bazel test //test/...` | ||
|
|
||
| # Adding external dependencies to Envoy (external cmake) | ||
|
|
||
| This is the preferred style of adding dependencies that use cmake for their build system. | ||
|
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. Nit: |
||
|
|
||
| 1. Define a the source Bazel repository in [`bazel/repositories.bzl`](repositories.bzl), in the | ||
| `envoy_dependencies()` function. | ||
| 2. Add a `cmake_external` rule to [`bazel/foreign_cc/BUILD`](bazel/foreign_cc/BUILD). This will | ||
| reference the source repository in step 1. | ||
| 3. Reference your new external dependency in some `envoy_cc_library` via Y in the | ||
|
htuch marked this conversation as resolved.
Outdated
|
||
| `external_deps` attribute. | ||
| 4. `bazel test //test/...` | ||
|
|
||
| # Adding external dependencies to Envoy (genrule repository) | ||
|
|
||
| This is the newer style of adding dependencies with no upstream Bazel configs. | ||
|
|
@@ -44,8 +56,9 @@ to binaries, libraries, headers, etc. | |
|
|
||
| # Adding external dependencies to Envoy (build recipe) | ||
|
|
||
| This is the older style of adding dependencies. It uses shell scripts to build | ||
| and install dependencies into a shared directory prefix. | ||
| This is the older style of adding dependencies. It uses shell scripts to build and install | ||
| dependencies into a shared directory prefix. This should no longer be used unless there are | ||
| extenuating circumstances. | ||
|
|
||
| 1. Add a build recipe X in [`ci/build_container/build_recipes`](../ci/build_container/build_recipes) | ||
| for developer-local and CI external dependency build flows. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| licenses(["notice"]) # Apache 2 | ||
|
|
||
| load("//bazel:envoy_build_system.bzl", "envoy_package") | ||
| load("@rules_foreign_cc//tools/build_defs:cmake.bzl", "cmake_external") | ||
| load("@foreign_cc_platform_utils//:tools.bzl", "NINJA_COMMAND", "NINJA_DEP") | ||
|
|
||
| envoy_package() | ||
|
|
||
| cmake_external( | ||
| name = "ares", | ||
| 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
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. Done, but I'm going to skip the Windows parts and let @sesmith177 take this. |
||
| "CARES_SHARED": "no", | ||
| "CARES_STATIC": "on", | ||
| }, | ||
| 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_COMMAND, | ||
| NINJA_COMMAND + " install", | ||
| ], | ||
|
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. |
||
| tools_deps = NINJA_DEP, | ||
| ) | ||
|
|
||
| cmake_external( | ||
| name = "event", | ||
| cache_entries = { | ||
| "EVENT__DISABLE_OPENSSL": "on", | ||
| "EVENT__DISABLE_REGRESS": "on", | ||
| }, | ||
| cmake_options = ["-GNinja"], | ||
| lib_source = "@com_github_libevent_libevent//:all", | ||
| make_commands = [ | ||
| NINJA_COMMAND, | ||
| NINJA_COMMAND + " install", | ||
| ], | ||
| static_libraries = ["libevent.a"], | ||
| tools_deps = NINJA_DEP, | ||
| ) | ||
|
|
||
| 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_options = ["-GNinja"], | ||
| lib_source = "@com_github_nghttp2_nghttp2//:all", | ||
| make_commands = [ | ||
| NINJA_COMMAND, | ||
| NINJA_COMMAND + " install", | ||
| ], | ||
| static_libraries = ["libnghttp2.a"], | ||
| tools_deps = NINJA_DEP, | ||
| ) | ||
|
|
||
| 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_options = ["-GNinja"], | ||
| lib_source = "@com_github_jbeder_yaml_cpp//:all", | ||
| make_commands = [ | ||
| NINJA_COMMAND, | ||
| NINJA_COMMAND + " install", | ||
| ], | ||
| static_libraries = ["libyaml-cpp.a"], | ||
| tools_deps = NINJA_DEP, | ||
| ) | ||
|
|
||
| cmake_external( | ||
| name = "zlib", | ||
| cmake_options = ["-GNinja"], | ||
| lib_source = "@com_github_madler_zlib//:all", | ||
| make_commands = [ | ||
| NINJA_COMMAND, | ||
| NINJA_COMMAND + " install", | ||
| ], | ||
| static_libraries = ["libz.a"], | ||
| tools_deps = NINJA_DEP, | ||
| ) | ||
| 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.
Nit:
s/cmake/CMake/