Skip to content
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

Minimal Bazel infrastructure (#415). #577

Merged
merged 7 commits into from
Mar 17, 2017
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
/build
/docs/landing_source/.bundle
/generated
/bazel-*
/ci/bazel-*
/ci/prebuilt/thirdparty
/ci/prebuilt/thirdparty_build
cscope.*
BROWSE
15 changes: 15 additions & 0 deletions WORKSPACE
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
workspace(name = "envoy")

load("//bazel:repositories.bzl", "envoy_dependencies")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add workspace(name = "envoy").

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

envoy_dependencies()

bind(
name = "googletest_main",
actual = "@googletest//:googletest_main",
)

bind(
name = "spdlog",
actual = "@spdlog_git//:spdlog",
)
Empty file added bazel/BUILD
Empty file.
47 changes: 47 additions & 0 deletions bazel/envoy_build_system.bzl
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
# References to Envoy external dependencies should be wrapped with this function.
def envoy_external_dep_path(dep):
return "//external:%s" % dep
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since introduction of named Bazel workspaces, there is barely no reason for using bind and //external, IMHO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on what you mean by named Bazel workspace here? How would we do this without bind?

Copy link
Contributor

@PiotrSikora PiotrSikora Mar 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally, Bazel didn't have a concept of named workspaces, where workspace(name = ...) can enforce how the workspace is imported into other workspaces (this is of course limited to repositories that support Bazel build system).

Because of that, people imported the same dependency under different names, for example BoringSSL could be imported as git_new_repository(name = ssl) or git_new_repository(name = boringssl) or git_new_repository(name = boringssl_git) and //external was a way to cheat around that with aliasing @....//:ssl to //external:ssl.

But since BoringSSL can enforce that it's always imported with git_new_repository(name = boringssl) and available as @boringssl//, there is no real reason to provide //external:ssl alias for @boringssl//:ssl, since BoringSSL's :ssl target is always going to be available under that path.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this help in the use case here, where we need the same name for a given target, one in which we get it from a git_new_repository, and another that is a cc_library hack that pulls in the prebuilt .a files?

Copy link
Contributor

@PiotrSikora PiotrSikora Mar 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't really help. I'm just saying that adding this indirection and accessing targets via //external:googletest_main doesn't provide any benefits over accessing it directly as @googletest//:googletest_main, and the fact that you see //external in other places is mostly due to historic reasons.

Please note that it might be just a matter of taste, at this point.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'll keep this in mind going forward, good to know what's happening in the Bazel world, but will retain due to the multiple options we're supporting for these deps.


ENVOY_COPTS = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Buildifier: one flag per line.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

# Envoy C++ library targets should be specified with this function.
def envoy_cc_library(name,
srcs = [],
public_hdrs = [],
hdrs = [],
external_deps = [],
deps = [],
copts = []):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

native.cc_library(
name = name,
srcs = srcs,
hdrs = hdrs + public_hdrs,
deps = deps + [envoy_external_dep_path(dep) for dep in external_deps] +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Buildifier: deps go at the end.

["//source/precompiled:precompiled_includes"],
copts = ENVOY_COPTS + copts,
)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Buildifier: deps go at the end.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Buildifier: missing comma at the end.

)
86 changes: 86 additions & 0 deletions bazel/repositories.bzl
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
# 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#
################################################################################
#

cc_library(
name = "googletest",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

srcs = [
"googlemock/src/gmock-all.cc",
"googletest/src/gtest-all.cc",
],
hdrs = glob([
"googletest/include/**/*.h",
"googlemock/include/**/*.h",
"googletest/src/*.cc",
"googletest/src/*.h",
"googlemock/src/*.cc",
]),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

includes = [
"googlemock",
"googlemock/include",
"googletest",
"googletest/include",
],
visibility = ["//visibility:public"],
)

cc_library(
name = "googletest_main",
srcs = ["googlemock/src/gmock_main.cc"],
visibility = ["//visibility:public"],
deps = [":googletest"],
)
"""
native.new_git_repository(
name = "googletest",
build_file_content = BUILD,
# v1.8.0 release
commit = "ec44c6c1675c25b9827aacd08c02433cccde7780",
remote = "https://github.com/google/googletest.git",
)

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

cc_library(
name = "spdlog",
hdrs = glob([
"include/**/*.h",
"include/**/*.cc",
]),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: please sort this alphabetically.

strip_include_prefix = "include",
)
"""

native.new_git_repository(
name = "spdlog_git",
build_file_content = BUILD,
# v0.11.0 release
commit = "1f1f6a5f3b424203a429e9cb78e6548037adefa8",
remote = "https://github.com/gabime/spdlog.git",
)

def envoy_dependencies():
googletest_repositories()
spdlog_repositories()
9 changes: 9 additions & 0 deletions ci/WORKSPACE
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
bind(
name = "googletest_main",
actual = "//ci/prebuilt:googletest_main",
)

bind(
name = "spdlog",
actual = "//ci/prebuilt:spdlog",
)
20 changes: 20 additions & 0 deletions ci/do_ci.sh
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,26 @@ set -e

echo "building using $NUM_CPUS CPUs"

if [[ "$1" == "bazel.debug" ]]; then
echo "debug bazel build with tests..."
cd ci
ln -sf /thirdparty prebuilt
ln -sf /thirdparty_build prebuilt
# Not sandboxing, since non-privileged Docker can't do nested namespaces.
echo "Building..."
export CC=gcc-4.9
export CXX=g++-4.9
export USER=bazel
export TEST_TMPDIR=/source
BAZEL_OPTIONS="--strategy=CppCompile=standalone --strategy=CppLink=standalone \
--strategy=TestRunner=standalone --verbose_failures --package_path %workspace%:.."
[[ "$BAZEL_INTERACTIVE" == "1" ]] && BAZEL_BATCH="" || BAZEL_BATCH="--batch"
bazel $BAZEL_BATCH build $BAZEL_OPTIONS //source/...
echo "Testing..."
bazel $BAZEL_BATCH test $BAZEL_OPTIONS --test_output=all //test/...
exit 0
fi

. "$(dirname "$0")"/build_setup.sh

if [[ "$1" == "fix_format" ]]; then
Expand Down
24 changes: 24 additions & 0 deletions ci/prebuilt/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package(default_visibility = ["//visibility:public"])

cc_library(
name = "googletest_main",
srcs = [
"thirdparty_build/lib/libgmock.a",
"thirdparty_build/lib/libgtest.a",
"thirdparty_build/lib/libgtest_main.a",
],
hdrs = glob([
"thirdparty_build/include/gmock/**/*.h",
"thirdparty_build/include/gtest/**/*.h",
]),
strip_include_prefix = "thirdparty_build/include",
)

cc_library(
name = "spdlog",
hdrs = glob([
"thirdparty/spdlog-0.11.0/include/**/*.h",
"thirdparty/spdlog-0.11.0/include/**/*.cc",
]),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: please sort this alphabetically.

strip_include_prefix = "thirdparty/spdlog-0.11.0/include",
)
22 changes: 22 additions & 0 deletions include/envoy/common/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package(default_visibility = ["//visibility:public"])

load("//bazel:envoy_build_system.bzl", "envoy_cc_library")

envoy_cc_library(
name = "base_includes",
hdrs = [
"exception.h",
"pure.h",
],
)

envoy_cc_library(
name = "time_includes",
hdrs = ["time.h"],
deps = [":base_includes"],
)

envoy_cc_library(
name = "optional_includes",
hdrs = ["optional.h"],
)
12 changes: 12 additions & 0 deletions source/common/common/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package(default_visibility = ["//visibility:public"])

load("//bazel:envoy_build_system.bzl", "envoy_cc_library")

envoy_cc_library(
name = "utility_lib",
srcs = ["utility.cc"],
hdrs = ["utility.h"],
deps = [
"//include/envoy/common:time_includes",
],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)
9 changes: 9 additions & 0 deletions source/precompiled/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package(default_visibility = ["//visibility:public"])

load("//bazel:envoy_build_system.bzl", "envoy_external_dep_path")

cc_library(
name = "precompiled_includes",
hdrs = ["precompiled.h"],
deps = [envoy_external_dep_path("spdlog")],
)
7 changes: 7 additions & 0 deletions test/common/common/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
load("//bazel:envoy_build_system.bzl", "envoy_cc_test")

envoy_cc_test(
name = "utility_test",
srcs = ["utility_test.cc"],
deps = ["//source/common/common:utility_lib"],
)
9 changes: 9 additions & 0 deletions test/precompiled/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package(default_visibility = ["//visibility:public"])

load("//bazel:envoy_build_system.bzl", "envoy_external_dep_path")

cc_library(
name = "precompiled_includes",
hdrs = ["precompiled_test.h"],
deps = [envoy_external_dep_path("googletest_main")],
)
2 changes: 0 additions & 2 deletions test/precompiled/precompiled_test.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,5 @@

#include "precompiled/precompiled.h"

#include "test/test_common/printers.h"

#include "gmock/gmock.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@htuch htuch Mar 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#include "gtest/gtest.h"