build: external_cmake for cmake deps. #5218

htuch · 2018-12-04T22:48:12Z

This revives #4516, replacing all external cmake deps with the pending bazel foreign_cc support.

To make use of this, with the current release Bazel version, it's necessary to set
--experimental_cc_skylark_api_enabled_packages=@rules_foreign_cc//tools/build_defs,tools/build_defs
on the CLI or in .bazelrc.

Risk Level: Low
Testing: bazel test //test/...

Signed-off-by: Harvey Tuch htuch@google.com

This revives envoyproxy#4516, replacing all external cmake deps with the pending bazel foreign_cc support. To make use of this, with the current release Bazel version, it's necessary to set --experimental_cc_skylark_api_enabled_packages=@rules_foreign_cc//tools/build_defs,tools/build_defs on the CLI or in .bazelrc. Risk Level: Low Testing: bazel test //test/... Signed-off-by: Harvey Tuch <htuch@google.com>

htuch · 2018-12-04T22:50:55Z

This PR requires changes to .bazelrc still for the experimental flags. Question; should we provide a fallback to the deprecated build recipes if missing? The downside of this is it might involve Bazel hackery and would have two sources of truth for the build in the interim.

CC @dprotaso @moderation @irengrig

sesmith177

For the Windows builds, I don't think we need to provide a fallback to the build scripts, given that getting those to work with both release builds + debug builds was both tricky to get right and never made it upstream. And since you can't build Envoy on Windows without a forked version of Bazel (due to bazelbuild/bazel#5163), I don't think we have to worry about breaking people in this case

sesmith177 · 2018-12-05T13:20:48Z

tools/bazel.rc

@@ -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


I think we might need to add "@foreign_cc_impl" to this list; see https://github.com/bazelbuild/rules_foreign_cc/blob/master/README.md#rules_foreign_cc

sesmith177 · 2018-12-05T13:28:34Z

bazel/foreign_cc/nghttp2.patch

+     COMPILE_FLAGS "${WARNCFLAGS}"
+     VERSION ${LT_VERSION} SOVERSION ${LT_SOVERSION}
+     ARCHIVE_OUTPUT_NAME nghttp2
+    ARCHIVE_OUTPUT_DIRECTORY static


This diff isn't going to work -- it builds the static nghttp2.lib in a different directory from the dynamic nghttp2.lib, but when ninja install is run, it installs both on top of each other.

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

Ack. I'm going to go ahead with the PR as-is and we can update this when the nghttp2 patch merges.

sesmith177 · 2018-12-05T13:30:14Z

bazel/repositories.bzl

+    http_archive(
+        name = "rules_foreign_cc",
+        strip_prefix = "rules_foreign_cc-master",
+        # TODO(htuch): Pin to SHA or release.


I assume we want to do this before we merge?

sesmith177 · 2018-12-05T13:45:19Z

bazel/foreign_cc/BUILD

+    make_commands = [
+        NINJA_COMMAND,
+        NINJA_COMMAND + " install",
+    ],


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

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.

Not a problem -- we can submit a followup PR that cleans up the Windows bits

sesmith177 · 2018-12-05T13:46:05Z

bazel/foreign_cc/BUILD

+        NINJA_COMMAND,
+        NINJA_COMMAND + " install",
+    ],
+    static_libraries = ["libcares.a"],


The output static library files are going to be different on Windows and Linux

sesmith177 · 2018-12-05T13:47:28Z

bazel/foreign_cc/BUILD

+        "CARES_SHARED": "no",
+        "CARES_STATIC": "on",
+    },
+    cmake_options = ["-GNinja"],


We need to set generate_crosstool_file = true on Windows

stale · 2018-12-12T14:28:12Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch · 2018-12-18T07:16:50Z

@sesmith177 @mattklein123 PTAL. I think we should ship with the required additional CLI args. This will break anyone not using the repo's bazel.rc, but we can do a mail to envoy-dev warning folks. The advantages are we can move towards Windows support, unblock the mobile client work for next year and also effectively retire most uses of build recipes.

mattklein123

Nice, let's do it. Agreed on just sending out an email to envoy-dev about the CLI required args.

As an aside, do you know the status of having native Bazel in gperftools? Seems like Google should provide that at some point?

htuch · 2018-12-18T21:18:19Z

@mattklein123 gperftools is being tracked at gperftools/gperftools#800. Doesn't look like much recent progress. I will ping the thread.

I just synced with Bazel team, who say that we will be able to use external_cmake without CLI flags in 0.22, which will happen mid-late Jan. @sesmith177 @mattklein123 thoughts on whether we just wait until then rather than going back-and-forth with the dev community on this?

sesmith177 · 2018-12-18T21:47:34Z

Since Envoy having "real" support for Windows is blocked on bazelbuild/bazel#5163, we're OK with waiting until January. This specific PR isn't going to block us, since we have these changes on our fork anyways

mattklein123 · 2018-12-18T22:12:15Z

Sure, waiting till January is fine with me.

PiotrSikora · 2018-12-19T00:47:49Z

bazel/EXTERNAL_DEPS.md

@@ -17,6 +17,18 @@ build process.
   `external_deps` attribute.
 3. `bazel test //test/...`

+# Adding external dependencies to Envoy (external cmake)


Nit: s/cmake/CMake/

PiotrSikora · 2018-12-19T00:47:56Z

bazel/EXTERNAL_DEPS.md

@@ -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.


Nit: s/cmake/CMake/

PiotrSikora · 2018-12-19T00:50:53Z

bazel/foreign_cc/BUILD

+
+cmake_external(
+    name = "ares",
+    cache_entries = {


Add "CMAKE_BUILD_TYPE": "RelWithDebInfo" (possibly "Debug" for Windows builds?)

Done, but I'm going to skip the Windows parts and let @sesmith177 take this.

PiotrSikora · 2018-12-19T00:51:40Z

bazel/foreign_cc/BUILD

+
+cmake_external(
+    name = "nghttp2",
+    cache_entries = {


Add "CMAKE_BUILD_TYPE": "RelWithDebInfo" (possibly "Debug" for Windows builds?)

PiotrSikora · 2018-12-19T00:52:31Z

bazel/foreign_cc/BUILD

+cmake_external(
+    name = "yaml",
+    cache_entries = {
+        "YAML_CPP_BUILD_TESTS": "off",


Add "CMAKE_BUILD_TYPE": "RelWithDebInfo" (possibly "Debug" for Windows builds?)

PiotrSikora · 2018-12-20T03:15:59Z

This needs to be rebased to pull BoringSSL FIPS (which, even though is built using CMake, should continue to be build that way using the provided script and not cmake_external).

htuch · 2018-12-21T07:32:32Z

@PiotrSikora for BoringSSL FIPS, I think if we're going to keep it script based in the long term, it should be moved to https://github.com/envoyproxy/envoy/blob/master/bazel/EXTERNAL_DEPS.md#adding-external-dependencies-to-envoy-genrule-repository style of external repo. We want to completely retire build recipes and the repository rules.

Will you do the migration or should we file an issue?

stale · 2018-12-29T11:11:19Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch · 2019-01-30T16:20:45Z

All tests are passing; @mattklein123 @PiotrSikora @sesmith177 are we good to go with this?

sesmith177 · 2019-01-30T17:12:45Z

Looks good on our end -- we'll submit a follow up PR to fix up the Windows bits

moderation · 2019-01-30T19:51:44Z

Looks great. Will make dependency maintenance easier.

mattklein123

Nice!

I think this was the intended code, I noticed a failure in compile_time_options for envoyproxy#5218. Risk Level: Low Testing: bazel build --config=libc++ //source/exe:envoy-static Signed-off-by: Harvey Tuch <htuch@google.com> Signed-off-by: htuch <htuch@users.noreply.github.com>

This revives envoyproxy#4516, replacing all external cmake deps with the pending bazel foreign_cc support. Risk Level: Low Testing: bazel test //test/... Signed-off-by: Harvey Tuch <htuch@google.com> Signed-off-by: htuch <htuch@users.noreply.github.com>

*Description*: YAML tools build with bazel cmake from #5218 (it pulls our cc_configure and I think that is the reason) requires newer version of GLIBCXX that CentOS doesn't have. It also speed up build so lets just turn off building them. *Risk Level*: Low *Testing*: CI *Docs Changes*: *Release Notes*: Signed-off-by: Lizan Zhou <lizan@tetrate.io>

Now that #5218 has been merged, we can enable release builds on Windows. This PR updates `ci/do_ci.ps1` to build release + fastbuild versions in addition to debug versions. It also removes support for build scripts in `ci/build_container/build_recipes/` on Windows, as there is no clean way to pass information from Bazel regarding the type of build to those scripts Risk Level: Low Testing: `bazel build //source/... && bazel test //test/...` Signed-off-by: Sam Smith <sesmith177@gmail.com>

I think this was the intended code, I noticed a failure in compile_time_options for envoyproxy#5218. Risk Level: Low Testing: bazel build --config=libc++ //source/exe:envoy-static Signed-off-by: Harvey Tuch <htuch@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>

This revives envoyproxy#4516, replacing all external cmake deps with the pending bazel foreign_cc support. Risk Level: Low Testing: bazel test //test/... Signed-off-by: Harvey Tuch <htuch@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>

*Description*: YAML tools build with bazel cmake from envoyproxy#5218 (it pulls our cc_configure and I think that is the reason) requires newer version of GLIBCXX that CentOS doesn't have. It also speed up build so lets just turn off building them. *Risk Level*: Low *Testing*: CI *Docs Changes*: *Release Notes*: Signed-off-by: Lizan Zhou <lizan@tetrate.io> Signed-off-by: Fred Douglas <fredlas@google.com>

Now that envoyproxy#5218 has been merged, we can enable release builds on Windows. This PR updates `ci/do_ci.ps1` to build release + fastbuild versions in addition to debug versions. It also removes support for build scripts in `ci/build_container/build_recipes/` on Windows, as there is no clean way to pass information from Bazel regarding the type of build to those scripts Risk Level: Low Testing: `bazel build //source/... && bazel test //test/...` Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Fred Douglas <fredlas@google.com>

htuch requested review from sesmith177 and mattklein123 December 4, 2018 22:48

htuch assigned mattklein123 and sesmith177 Dec 4, 2018

sesmith177 suggested changes Dec 5, 2018

View reviewed changes

dprotaso mentioned this pull request Dec 7, 2018

Create a bazel rule to print Envoy's dependencies #5049

Closed

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 12, 2018

htuch removed the stale stalebot believes this issue/PR has not been touched recently label Dec 18, 2018

htuch added 3 commits December 18, 2018 00:57

Merge remote-tracking branch 'upstream/master' into external-cmake-now

2fb2961

Add @foreign_cc_impl and pin to SHA.

1c88f27

Signed-off-by: Harvey Tuch <htuch@google.com>

fix_format

3554527

Signed-off-by: Harvey Tuch <htuch@google.com>

mattklein123 previously approved these changes Dec 18, 2018

View reviewed changes

PiotrSikora suggested changes Dec 19, 2018

View reviewed changes

PiotrSikora mentioned this pull request Dec 22, 2018

build: migrate BoringSSL FIPS to genrule_repository. #5404

Merged

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 29, 2018

mattklein123 added the no stalebot Disables stalebot from closing an issue label Jan 3, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jan 3, 2019

htuch mentioned this pull request Jan 7, 2019

debug: Link in the debug version of tcmalloc for debug builds. #5424

Merged

mattklein123 added the waiting label Jan 11, 2019

Merge remote-tracking branch 'upstream/master' into external-cmake-now

509d8cd

Signed-off-by: Harvey Tuch <htuch@google.com>

Merge remote-tracking branch 'upstream/master' into external-cmake-now

4355c7a

htuch removed the no stalebot Disables stalebot from closing an issue label Jan 29, 2019

Merge remote-tracking branch 'upstream/master' into external-cmake-now

8b4c009

mattklein123 approved these changes Jan 30, 2019

View reviewed changes

htuch merged commit 7fa5513 into envoyproxy:master Jan 30, 2019

htuch deleted the external-cmake-now branch January 30, 2019 22:12

htuch mentioned this pull request Jan 30, 2019

For QUICHE integration, add --experimental_remap_main_repo to bazelrc. #5767

Closed

moderation mentioned this pull request Jan 31, 2019

Support for arm64 / aarch64 bazel-contrib/rules_foreign_cc#212

Closed

lizan mentioned this pull request Jan 31, 2019

build: fix external cmake path #5787

Merged

This was referenced Jan 31, 2019

move ssl implementation dependent tests to extensions #5778

Merged

build: not build yaml-cpp tools #5799

Merged

sesmith177 mentioned this pull request Feb 1, 2019

build: enable release builds on Windows #5812

Merged

f3nry mentioned this pull request Feb 4, 2019

build: add foreign_cc rules envoyproxy/envoy-filter-example#73

Merged

taion809 mentioned this pull request Feb 4, 2019

external filter bazel builds using envoy as a submodule fail to fetch external deps #5835

Closed

build: external_cmake for cmake deps. #5218

build: external_cmake for cmake deps. #5218

Uh oh!

Conversation

htuch commented Dec 4, 2018

Uh oh!

htuch commented Dec 4, 2018

Uh oh!

sesmith177 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale bot commented Dec 12, 2018

Uh oh!

htuch commented Dec 18, 2018

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

htuch commented Dec 18, 2018

Uh oh!

sesmith177 commented Dec 18, 2018

Uh oh!

mattklein123 commented Dec 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PiotrSikora commented Dec 20, 2018

Uh oh!

htuch commented Dec 21, 2018

Uh oh!

stale bot commented Dec 29, 2018

Uh oh!

htuch commented Jan 30, 2019

Uh oh!

sesmith177 commented Jan 30, 2019

Uh oh!

moderation commented Jan 30, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!