-
Notifications
You must be signed in to change notification settings - Fork 5.5k
[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 1 commit
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.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| # Envoy specific Bazel build/test options. | ||
|
|
||
| build --workspace_status_command=bazel/get_workspace_status | ||
| build --experimental_cc_skylark_api_enabled_packages=@rules_foreign_cc//tools/build_defs,tools/build_defs | ||
|
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. This is the main blocker to use merging this PR; we don't want folks to have to be setting this when importing Envoy to their own projects. CC @irengrig.
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. @irengrig this aged out of our review process, because we haven't figured out these last few steps and weren't able to verify Windows support (@sesmith177). Do you think there is any way to resolve these?
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. @htuch we have used a version of bazel that has the fix for bazelbuild/bazel#6292 and verified that we can build Envoy with it. We have not had a chance yet to take another look at this PR 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 @htuch, I asked the people from Bazel C++ team, they estimate the work for getting rid of the --experimental_cc_skylark_api_enabled_packages flag as at least two weeks. Nice to hear about Windows build.
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. Thanks @irengrig. Please ping me when this is done and we will try and switch this on in master (hopefully @sesmith177 and MSFT will have time to evaluate by then).
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. it gets farther on windows with the newer bazel, but still fails to build: we haven't had too much time to debug this |
||
|
|
||
| # Basic ASAN/UBSAN that works for gcc | ||
| build:asan --define ENVOY_CONFIG_ASAN=1 | ||
|
|
||
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.