-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[WIP] bazel: use experimental cmake_external for cares/nghttp2. #4516
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
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 |
|---|---|---|
|
|
@@ -9,6 +9,18 @@ build process. | |
| `external_deps` attribute. | ||
| 3. `bazel test //test/...` | ||
|
|
||
| # Adding external dependencies to Envoy (extern cmake) | ||
|
|
||
| This is the preferred style of adding dependencies that use cmake for their build system. | ||
|
|
||
| 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 | ||
|
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. Will improve the writeup before we merge here. This is now like the 5th way of handling external deps (obligatory https://xkcd.com/927/). |
||
| `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. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| 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 = { | ||
| "CARES_SHARED": "no", | ||
| "CARES_STATIC": "on", | ||
| }, | ||
| cmake_options = ["-GNinja"], | ||
| lib_source = "@com_github_c_ares_c_ares//:all", | ||
| make_commands = [ | ||
| NINJA_COMMAND, | ||
| NINJA_COMMAND + " install", | ||
| ], | ||
| static_libraries = ["libcares.a"], | ||
| tools_deps = NINJA_DEP, | ||
| ) | ||
|
|
||
| cmake_external( | ||
|
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. @sesmith177 any chance you can try out this PR and let me know if it does the right thing on Windows?
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 can take a look at it when we get a chance. One thing we noticed is that the names of the static libraries are different on Windows and Linux ( 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. Hi, here's my modified cares build for Windows (I just completed it, will update the examples): cmake_external(
Hope it helps. Will be happy to answer any questions. |
||
| name = "nghttp2", | ||
| cache_entries = { | ||
| "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, | ||
| ) | ||
|
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 should add some more of the cmake deps as well once we have agreement on when we will be merging this. |
||
| 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 | ||
| ) | ||
| target_compile_definitions(nghttp2_static PUBLIC "-DNGHTTP2_STATICLIB") | ||
| if(ENABLE_STATIC_LIB) |
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,11 @@ REPOSITORY_LOCATIONS = dict( | |
| strip_prefix = "backward-cpp-1.4", | ||
| urls = ["https://github.com/bombela/backward-cpp/archive/v1.4.tar.gz"], | ||
| ), | ||
| com_github_c_ares_c_ares = dict( | ||
| sha256 = "62dd12f0557918f89ad6f5b759f0bf4727174ae9979499f5452c02be38d9d3e8", | ||
| strip_prefix = "c-ares-cares-1_14_0", | ||
| urls = ["https://github.com/c-ares/c-ares/archive/cares-1_14_0.tar.gz"], | ||
| ), | ||
| com_github_circonus_labs_libcircllhist = dict( | ||
| commit = "050da53a44dede7bda136b93a9aeef47bd91fa12", # 2018-07-02 | ||
| remote = "https://github.com/circonus-labs/libcircllhist", | ||
|
|
@@ -62,6 +67,11 @@ REPOSITORY_LOCATIONS = dict( | |
| strip_prefix = "nanopb-f8ac463766281625ad710900479130c7fcb4d63b", | ||
| urls = ["https://github.com/nanopb/nanopb/archive/f8ac463766281625ad710900479130c7fcb4d63b.tar.gz"], | ||
| ), | ||
| com_github_nghttp2_nghttp2 = dict( | ||
| sha256 = "42fff7f290100c02234ac3b0095852e4392e6bfd95ebed900ca8bd630850d88c", | ||
| strip_prefix = "nghttp2-1.33.0", | ||
|
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. @htuch nghttp2 version 1.35.0 is out if you would like to bump to that version. Master is currently set at 1.34.0
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. Yep, thanks, already bumped the versions. |
||
| urls = ["https://github.com/nghttp2/nghttp2/releases/download/v1.33.0/nghttp2-1.33.0.tar.gz"], | ||
| ), | ||
| io_opentracing_cpp = dict( | ||
| sha256 = "4455ca507936bc4b658ded10a90d8ebbbd61c58f06207be565a4ffdc885687b5", | ||
| strip_prefix = "opentracing-cpp-1.5.0", | ||
|
|
||
This file was deleted.
This file was deleted.
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 would like to figure out how to roll this into
envoy_dependencies(), but it depends on some of the setup there.loadcan't be used inside a macro, only at file level or inWORKSPACE. Might be possibly with a multi-phaseenvoy_dependencies()setup.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.
afaik, unfortunately you can only call the initialization function directly in WORKSPACE file. Should be resolved when recursive workspace paring be implemented.