Minimal Bazel infrastructure (#415). #577

htuch · 2017-03-16T19:08:44Z

This patch introduces the minimal Bazel infrastructure required to build and execute utility_test,
as a step towards conversion of the cmake build system to Bazel (#415). The main contributions are:

Machinery for declaring Envoy C++ library and test targets (envoy_cc_library, envoy_cc_test).
External dependency management that works both inside the Docker CI environment (using prebuilts)
and with developer-local dependency fetch/build (based on Add Bazel support #416).
Handling of the implicit dependencies that are today sourced via prebuilts.
Simple example of building and running a unit test with only its dependencies built. With the
cmake system, we would need to build all of Envoy and all tests to run utility_test. E.g.

blaze test //test/common/common:utility_test

This is not intended to be used by anyone other than those working with the Bazel conversion at this
point. The plan is to add "ci/do_ci.sh bazel.debug" as a Travis target to ensure we don't bit rot.

This patch introduces the minimal Bazel infrastructure required to build and execute utility_test, as a step towards conversion of the cmake build system to Bazel (envoyproxy#415). The main contributions are: * Machinery for declaring Envoy C++ library and test targets (envoy_cc_library, envoy_cc_test). * External dependency management that works both inside the Docker CI environment (using prebuilts) and with developer-local dependency fetch/build (based on envoyproxy#416). * Handling of the implicit dependencies that are today sourced via prebuilts. * Simple example of building and running a unit test with only its dependencies built. With the cmake system, we would need to build all of Envoy and all tests to run utility_test. E.g. blaze test //test/common/common:utility_test This is not intended to be used by anyone other than those working with the Bazel conversion at this point. The plan is to add "ci/do_ci.sh bazel.debug" as a Travis target to ensure we don't bit rot.

htuch · 2017-03-16T19:09:30Z

@mattklein123 @PiotrSikora @lizan @jamessynge @kyessenov

lizan · 2017-03-16T19:20:08Z

bazel/envoy_build_system.bzl

+    return "//external:%s" % dep
+
+ENVOY_COPTS = [
+    "-ggdb3", "-fno-omit-frame-pointer", "-Wall", "-Wextra", "-Werror",


-ggdb3 shouldn't be enforced here, you can include it in .bazelrc (but only for specific config). bazel has -c flag to control fastbuild vs debug (dbg) vs release (opt)

I'm wondering where this would go in the repository. Let's say we do want our own standard set of additions for fastbuild/opt/debug, such as -ggdb3. Would we put this in $(ENVOY_SRC)/.bazelrc? If we put it here, my reading of the docs is that ~/.blazerc is then ignored. This would be bad as we disallow user configurability.

Oh, I see, we have tools/bazel.rc to work with, great.

lizan · 2017-03-16T19:28:34Z

bazel/envoy_build_system.bzl

+    "-ggdb3", "-fno-omit-frame-pointer", "-Wall", "-Wextra", "-Werror",
+    "-Wnon-virtual-dtor", "-Woverloaded-virtual", "-Wold-style-cast",
+    "-std=c++0x", "-includesource/precompiled/precompiled.h",
+    "-isystem", "include", "-isystem", "source"


iquote? I don't see where we use <> to include from those directories.

lizan · 2017-03-16T19:29:12Z

bazel/envoy_build_system.bzl

+    "-isystem", "include", "-isystem", "source"
+]
+
+ENVOY_LINKOPTS = ["-static-libstdc++", "-static-libgcc"]


Yeah, I originally was thinking about envoy_cc_binary, but we're aways from that.

lizan · 2017-03-16T19:32:33Z

ci/do_ci.sh

+  echo "Building..."
+  export CC=gcc-4.9
+  export CXX=g++-4.9
+  TEST_TMPDIR=/source/build USER=bazel bazel build --strategy=CppCompile=standalone \


use --batch to run bazel inside docker (no starting/shutting down server). Maybe worth to try to push those common bazel options in ci to a bazelrc file inside the docker image, so they don't repeatedly appears in the script.

I've switched to --batch by default and cleaned up the duplication (bazelrc is probably a bit overkill so just refactored).

mattklein123 · 2017-03-16T20:10:53Z

@htuch FYI @lyft/network-team has no capability to review this right now. Please just get a +1 from @lizan and @PiotrSikora and I will merge.

mattklein123 · 2017-03-16T20:30:25Z

test/precompiled/precompiled_test.h

@@ -2,7 +2,5 @@

 #include "precompiled/precompiled.h"

-#include "test/test_common/printers.h"


This is required to have pretty printing work correctly. It will compile but won't actually pretty print without it. Any reason to remove?

Things that are in precompiled are depended upon by the envoy_cc_library rule, so it isn't possible to have them built using this rule. printers.cc actually depends on files such as common/buffer/buffer_impl.h, which is basically arbitrary implementation files. It's possible I could split the dependency at the header/source file level, with two distinct targets to resolve this, but I couldn't find any references to the symbols in the tree and thought it was stale.

Who uses the pretty printers? Is this GTest?

I see, it's part of https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md#teaching-google-test-how-to-print-your-values, will fix.

PiotrSikora · 2017-03-16T21:48:03Z

bazel/envoy_build_system.bzl

+ENVOY_COPTS = [
+    "-fno-omit-frame-pointer", "-Wall", "-Wextra", "-Werror", "-Wnon-virtual-dtor",
+    "-Woverloaded-virtual", "-Wold-style-cast", "-std=c++0x",
+    "-includesource/precompiled/precompiled.h", "-iquote", "include", "-iquote", "source"


I'm not a big fan of mixing compiler flags and includes, maybe just move them to envoy_cc_library?

You mean in the includes attribute in envoy_cc_library? I tried that first time around, but since the target is relative to the package, it can't reference the build root. E.g. if the target being defined is in source/common/common, it can only specify includes in the cc_library that are a subdirectory of source/common/common.

Yes, that what I've meant... But it indeed wouldn't work with BUILD files in subdirectories.

PiotrSikora · 2017-03-16T21:52:18Z

bazel/repositories.bzl

+#
+
+cc_library(
+    name = "googletest",


This by no means should block this PR, but I've seen this enough times by now, that maybe we should upstream this BUILD file to google/googletest?

Agreed. Was this from Istio originally? Does @lizan want to do this? If nobody owns it I can add it to my backlog.

It came from NGINX-based ESP. I believe Martin wrote this, so it's (c) Google and can be released under different license, if that's why you're asking. I don't believe anybody "owns" it right now, so please add it to your backlog. Thanks!

PiotrSikora · 2017-03-16T21:59:18Z

ci/WORKSPACE

@@ -0,0 +1,9 @@
+bind(
+    name = "googletest_main",
+    actual = "//ci/prebuilt:googletest_main"


Buildifier: missing comma at the end.

PiotrSikora · 2017-03-16T21:59:24Z

ci/WORKSPACE

+
+bind(
+    name = "spdlog",
+    actual = "//ci/prebuilt:spdlog"


Buildifier: missing comma at the end.

PiotrSikora · 2017-03-16T22:00:07Z

ci/prebuilt/BUILD

+
+cc_library(
+    name = "googletest_main",
+    hdrs = glob([


Buildifier: hdrs should be listed after srcs.

PiotrSikora · 2017-03-16T22:08:29Z

bazel/repositories.bzl

+
+def spdlog_repositories():
+    BUILD = """
+package(default_visibility=["//visibility:public"])


Buildifier: missing surrounding spaces around =.

PiotrSikora · 2017-03-16T22:08:46Z

bazel/repositories.bzl

+        "include/**/*.h",
+        "include/**/*.cc",
+    ]),
+    strip_include_prefix = "include"


Buildifier: missing comma at the end.

PiotrSikora · 2017-03-16T22:11:16Z

bazel/repositories.bzl

+        remote = "https://github.com/gabime/spdlog.git",
+        # v0.11.0 release
+        commit = "1f1f6a5f3b424203a429e9cb78e6548037adefa8",
+        build_file_content = BUILD,)


Buildifier: build_file_content should be listed before commit and remote.

PiotrSikora · 2017-03-16T22:11:37Z

bazel/repositories.bzl

+
+    native.new_git_repository(
+        name = "spdlog_git",
+        remote = "https://github.com/gabime/spdlog.git",


Buildifier: remote should be listed after commit.

PiotrSikora · 2017-03-16T22:12:26Z

bazel/envoy_build_system.bzl

+def envoy_external_dep_path(dep):
+    return "//external:%s" % dep
+
+ENVOY_COPTS = [


Buildifier: one flag per line.

PiotrSikora

Also, maybe consider moving bazel to tools subdirectory? We'll most likely need to create tools/bazel.rc anyway, so this way it wouldn't be spread out too much.

PiotrSikora · 2017-03-16T22:19:15Z

WORKSPACE

@@ -0,0 +1,13 @@
+load("//bazel:repositories.bzl", "envoy_dependencies")
+


Add workspace(name = "envoy").

Actually, please add it at the top (before load()).

Turns out that if you move bazel to tools/bazel, Bazel tries to invoke the directory as a binary. I'm guessing that similar to the special nature of tools/bazel.rc, Bazel looks in tools/bazel for another bazel binary to reinvoke?!

Ouch! That's definitely unexpected... Nevermind, then!

PiotrSikora · 2017-03-17T01:29:37Z

bazel/envoy_build_system.bzl

+def envoy_external_dep_path(dep):
+    return "//external:%s" % dep
+
+ENVOY_COPTS = [


Nit: consider moving this at the top, otherwise it's intermixed with def ....

PiotrSikora · 2017-03-17T01:30:15Z

bazel/repositories.bzl

+# The build rules below for external dependencies build rules are maintained
+# on a best effort basis. The rules are provided for developer convenience. For production builds,
+# we recommend building the libraries according to their canonical build systems and expressing the
+# dependencies in a manner similar to ci/WORKSPACE.


Nit: this paragraph is unevenly wrapped (first line @ 80, rest @ 100?).

PiotrSikora · 2017-03-17T01:38:07Z

bazel/envoy_build_system.bzl

+    native.cc_test(
+        name = name,
+        srcs = srcs,
+        deps = deps + ["//source/precompiled:precompiled_includes",


Style: consider splitting brackets from multiline lists (as in other places), i.e.:

[ "//source/precompiled:precompiled_includes", "//test/precompiled:precompiled_includes", ]

and remember to add comma after last element.

Buildifier: deps go at the end.

PiotrSikora · 2017-03-17T01:39:37Z

bazel/envoy_build_system.bzl

+        name = name,
+        srcs = srcs,
+        hdrs = hdrs + public_hdrs,
+        deps = deps + [envoy_external_dep_path(dep) for dep in external_deps] +


Buildifier: deps go at the end.

PiotrSikora · 2017-03-17T01:40:27Z

bazel/envoy_build_system.bzl

+        deps = deps + ["//source/precompiled:precompiled_includes",
+                       "//test/precompiled:precompiled_includes"],
+        copts = ENVOY_COPTS + ["-includetest/precompiled/precompiled_test.h"],
+        linkopts = ["-pthread"]


Buildifier: missing comma at the end.

PiotrSikora · 2017-03-17T01:43:14Z

bazel/repositories.bzl

+        "googletest/src/*.cc",
+        "googletest/src/*.h",
+        "googlemock/src/*.cc",
+    ]),


Style: glob([]) should be sorted alphabetically, so everything from googlemock/ should go before googletest/.

PiotrSikora · 2017-03-17T01:44:13Z

bazel/repositories.bzl

+    hdrs = glob([
+        "include/**/*.h",
+        "include/**/*.cc",
+    ]),


Style: please sort this alphabetically.

PiotrSikora · 2017-03-17T01:44:34Z

ci/prebuilt/BUILD

+    hdrs = glob([
+        "thirdparty/spdlog-0.11.0/include/**/*.h",
+        "thirdparty/spdlog-0.11.0/include/**/*.cc",
+    ]),


Style: please sort this alphabetically.

PiotrSikora · 2017-03-17T01:45:14Z

source/common/common/BUILD

+    hdrs = ["utility.h"],
+    deps = [
+        "//include/envoy/common:time_includes",
+    ],


Style: consider consolidating single-element list into one line (like everywhere else).

lizan · 2017-03-17T01:55:09Z

bazel/envoy_build_system.bzl

+                     hdrs = [],
+                     external_deps = [],
+                     deps = [],
+                     copts = []):


you may want a **kwargs here to catch everything else that you don't touch and pass to cc_library (e.g. visibility, defines)

I'm trying to use the grpc trick of using envoy_cc_library as an indirect to facilitate later generation of cmake (or other build system) if needed. The idea I think is to constrain as much as possible what we do with the rule, so it wouldn't work as well to allow the target to make arbitrary use of cc_library features (e.g. strip_include_prefix) that might not work in other build systems.

Yes I understand that concept, but visibility is bazel specific concept that you will need unless you make all target public. Also I am pretty sure you will need alwayslink later :)

I've added visibility (which can be stripped by other build systems as they don't use this information) and alwayslink.

I'm not sure how we'll end up using visibility - it's useful for hiding things that are package local and in theory it could be used to enforce some good practices with the source/ impls (forcing folks to depend only on abstract interfaces), but its main use IMHO is for very large monolithic code bases where you don't want completely unrelated projects forming dependencies against each others implementations which aren't subject to versioning or stable interfaces.

lizan · 2017-03-17T01:58:28Z

bazel/envoy_build_system.bzl

+# Envoy C++ test targets should be specified with this function.
+def envoy_cc_test(name,
+                  srcs = [],
+                  deps = []):


lizan

LGTM

PiotrSikora · 2017-03-17T19:22:02Z

bazel/envoy_build_system.bzl

+        srcs = srcs,
+        hdrs = hdrs + public_hdrs,
+        copts = ENVOY_COPTS + copts,
+        alwayslink = alwayslink,


Buildifier: alwayslink should be at the end.

PiotrSikora · 2017-03-17T19:29:04Z

bazel/repositories.bzl

+
+def googletest_repositories():
+    BUILD = """
+# Copyright 2016 Google Inc. All Rights Reserved.


This is the only file ("generated" or not) that has the copyright. We should either drop it here (we can, since it's ours) or include in every BUILD file.

FWIW, OSPO usually requires copyright / license header in every single file, but it looks that's not the case in Envoy.

PiotrSikora

LGTM

Signed-off-by: Shikugawa <[email protected]>

Makes several updates to our podspec in order to fix CocoaPods integration with v0.2.0: - Link `resolv.9` and `c++` which is being done with Bazel today - Link `SystemConfiguration` which is being done with Bazel today - Specify Swift version and platform version - Specify `source_files` **with an empty `.swift` file** in order to ensure that Swift libraries are linked when building (i.e., `swiftFoundation`) **Important notes:** Releasing new versions of the pod requires the following manual changes: - Unzipping `envoy_ios_framework.zip` - Renaming the directory to `envoy_ios_cocoapods` - Creating an empty file at `envoy_ios_cocoapods/Envoy.framework/Swift/Empty.swift`. This forces CocoaPods to link Swift libraries as necessary. We worked around this in Bazel similarly in the past [here](envoyproxy/envoy-mobile@b8216e4#diff-6dc94efb18b54c46a32898ba3a5a0756R15) - Copying the repo's `LICENSE` file and placing it at `envoy_ios_cocoapods/LICENSE` - Re-zipping `envoy_ios_cocoapods` and uploading `envoy_ios_cocoapods.zip` In the future, we should see if this can be simplified or at the very least add a script that uploads this artifact with each commit to master just like we do with other artifacts. This is being tracked in envoyproxy/envoy-mobile#578. Note: There is a similar problem reported in CocoaPods that requires this empty Swift file workaround: CocoaPods/CocoaPods#8649. I tested this PR by using this podspec for building an app against Envoy Mobile with CocoaPods. This spec is currently published as v0.2.0. Signed-off-by: Michael Rebello <[email protected]> Signed-off-by: JP Simard <[email protected]>

lizan reviewed Mar 16, 2017

View reviewed changes

mattklein123 reviewed Mar 16, 2017

View reviewed changes

@lizan review feedback.

f4399be

PiotrSikora reviewed Mar 16, 2017

View reviewed changes

@PiotrSikora review feedback.

d6787b2

PiotrSikora reviewed Mar 17, 2017

View reviewed changes

lizan reviewed Mar 17, 2017

View reviewed changes

htuch added 3 commits March 17, 2017 09:32

Further @PiotrSikora review feedback.

f936a17

Restore printers.h include. This also fixes the debug build.

c924d54

Add alwayslink/visibility as suggested by @lizan.

294501e

lizan approved these changes Mar 17, 2017

View reviewed changes

PiotrSikora reviewed Mar 17, 2017

View reviewed changes

Further @PiotrSikora review feedback.

43b6948

PiotrSikora approved these changes Mar 17, 2017

View reviewed changes

mattklein123 approved these changes Mar 17, 2017

View reviewed changes

mattklein123 merged commit 72e88af into envoyproxy:master Mar 17, 2017

htuch deleted the bazel-minimal branch March 31, 2017 16:31

lambdai pushed a commit to lambdai/envoy-dai that referenced this pull request Jul 21, 2020

abseil: enable to build wasm32 (envoyproxy#577)

18ad1bb

Signed-off-by: Shikugawa <[email protected]>

		@@ -2,7 +2,5 @@

		#include "precompiled/precompiled.h"

		#include "test/test_common/printers.h"

		@@ -0,0 +1,13 @@
		load("//bazel:repositories.bzl", "envoy_dependencies")

Minimal Bazel infrastructure (#415). #577

Minimal Bazel infrastructure (#415). #577

Conversation

htuch commented Mar 16, 2017

htuch commented Mar 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented Mar 16, 2017

Choose a reason for hiding this comment

htuch Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PiotrSikora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lizan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PiotrSikora left a comment

Choose a reason for hiding this comment

htuch Mar 16, 2017 •

edited

Loading