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

bazel/ci: unify developer-local and CI build generation. #716

Closed
wants to merge 4 commits into from

Conversation

htuch
Copy link
Member

@htuch htuch commented Apr 7, 2017

This patch dedupes the distinct approaches that existed previously, where we assembled hand curated
BUILD files for external dependencies to be used in developer-local builds and used prebuilt
artifacts compiled under the external dependency's native build system for CI.

In the new approach, the CI flow continues to prebuild artifacts with the build recipes and
recursive make in ci/build_container/{Makefile,build_recipes}, ahead of time and prior to any
invocation of Bazel.

Developer-local builds will not prebuild, but instead invoke the same build recipes and recursive
make under a Bazel genrule. The trick here is that we can no longer use globs to refer to the build
artifacts in BUILD rules, since Bazel doesn't know about the outputs of the genrule ahead of time,
so we need to use a generated BUILD, produced by the bazel/gen_prebuilt.sh script, which does an
offline build first, analyzes the outputs and then produces a BUILD file stating what the genrule
outputs. This is only done once by a developer making an external dependency change, prior to
checking in the change.

There's a change to how external dependencies are specified as a result. Instead of adding new
external dependencies to ci/build_container/BUILD, they are added to the definitions at the start
of bazel/gen_prebuilt.py. The developer who is adding the external dependency then runs:

ENVOY_SRC_DIR=$PWD ./bazel/gen_prebuilt.sh

and checks in the resulting generated bazel/prebuilt.bzl file.

Existing users of the CI flow (Lyft) will need to make a slight tweak to how the prebuilt artifacts
are generated - the thirdparty/ and thirdparty_build/ directories will need to be generated with
the BUILD_DISTINCT=1 Makefile setting in #714.

A significant improvement that is also provided by this patch is the automatic workspace population
via prebuilt.bzl. See the changes to WORKSPACE and ci/WORKSPACE. Projects that consume Envoy, e.g.
to link in additional filters, no longer need to track Envoy's dependencies and maintain their own
bind rules, this is now automagic.

This patch dedupes the distinct approaches that existed previously, where we assembled hand curated
BUILD files for external dependencies to be used in developer-local builds and used prebuilt
artifacts compiled under the external dependency's native build system for CI.

In the new approach, the CI flow continues to prebuild artifacts with the build recipes and
recursive make in ci/build_container/{Makefile,build_recipes}, ahead of time and prior to any
invocation of Bazel.

Developer-local builds will not prebuild, but instead invoke the same build recipes and recursive
make under a Bazel genrule. The trick here is that we can no longer use globs to refer to the build
artifacts in BUILD rules, since Bazel doesn't know about the outputs of the genrule ahead of time,
so we need to use a generated BUILD, produced by the bazel/gen_prebuilt.sh script, which does an
offline build first, analyzes the outputs and then produces a BUILD file stating what the genrule
outputs. This is only done once by a developer making an external dependency change, prior to
checking in the change.

There's a change to how external dependencies are specified as a result. Instead of adding new
external dependencies to ci/build_container/BUILD, they are added to the definitions at the start
of bazel/gen_prebuilt.py. The developer who is adding the external dependency then runs:

ENVOY_SRC_DIR=$PWD ./bazel/gen_prebuilt.sh

and checks in the resulting generated bazel/prebuilt.bzl file.

Existing users of the CI flow (Lyft) will need to make a slight tweak to how the prebuilt artifacts
are generated - the thirdparty/ and thirdparty_build/ directories will need to be generated with
the BUILD_DISTINCT=1 Makefile setting in envoyproxy#714.

A significant improvement that is also provided by this patch is the automatic workspace population
via prebuilt.bzl. See the changes to WORKSPACE and ci/WORKSPACE. Projects that consume Envoy, e.g.
to link in additional filters, no longer need to track Envoy's dependencies and maintain their own
bind rules, this is now automagic.
@htuch
Copy link
Member Author

htuch commented Apr 7, 2017

@mattklein123 for review and Lyft integration discussion. @lizan for Bazel review. @dnoe and @rlazarus for dog fooding with the new dependencies they plan on adding to gflags and backwards/elfutils.

Allows Bazel to treat it with -isystem and avoid compiler warnings for headers from deps.
htuch added a commit to htuch/envoy that referenced this pull request Apr 7, 2017
…voyproxy#415).

Bazel has 3 compilation modes out-of-the-box {fastbuild, opt, dbg}, see
https://bazel.build/versions/master/docs/bazel-user-manual.html#flag--compilation_mode.

The cmake build flags for normal/debug/server_only/asan/coverage are at
envoyproxy#415 (comment).

Below are the relevant flags (ignoring -include etc.) from the various builds. Lines that begin with
* are those that include additional flags we're injecting.

fastbuild:

 -U_FORTIFY_SOURCE -fstack-protector -Wall
 -Wunused-but-set-parameter -Wno-free-nonheap-object -fno-omit-frame-pointer '-std=c++0x'
 -frandom-seed=...
 -fPIC
*-DBAZEL_BRINGUP -Wall -Wextra -Werror -Wnon-virtual-dtor -Woverloaded-virtual
*-Wold-style-cast '-std=c++0x' -includeprecompiled/precompiled.h -DDEBUG
 -fno-canonical-system-headers
 -Wno-builtin-macro-redefined '-D__DATE__="redacted"' '-D__TIMESTAMP__="redacted"'
 '-D__TIME__="redacted"'

dbg:

 -U_FORTIFY_SOURCE -fstack-protector -Wall
 -Wunused-but-set-parameter -Wno-free-nonheap-object -fno-omit-frame-pointer -g '-std=c++0x'
 -frandom-seed=...
 -fPIC
*-DBAZEL_BRINGUP -Wall -Wextra -Werror -Wnon-virtual-dtor
*-Woverloaded-virtual -Wold-style-cast '-std=c++0x' -includeprecompiled/precompiled.h -ggdb3 -DDEBUG
 -fno-canonical-system-headers
 -Wno-builtin-macro-redefined '-D__DATE__="redacted"' '-D__TIMESTAMP__="redacted"'
 '-D__TIME__="redacted"'

opt:

 -U_FORTIFY_SOURCE -fstack-protector -Wall
 -Wunused-but-set-parameter -Wno-free-nonheap-object -fno-omit-frame-pointer -g0 -O2
 '-D_FORTIFY_SOURCE=1' -DNDEBUG -ffunction-sections -fdata-sections '-std=c++0x'
 -frandom-seed=...
*-DBAZEL_BRINGUP -Wall -Wextra -Werror -Wnon-virtual-dtor
*-Woverloaded-virtual -Wold-style-cast '-std=c++0x' -includeprecompiled/precompiled.h
 -fno-canonical-system-headers
 -Wno-builtin-macro-redefined '-D__DATE__="redacted"' '-D__TIMESTAMP__="redacted"'
 '-D__TIME__="redacted"'

This PR requires envoyproxy#716 to merge, since envoyproxy#716 provides some important changes to how the headers are
exported from the 3rd party deps to allow -Wextra and -Wold-style-cast to work.

I've verified that we now have a 100% static envoy-static build, which ldd tells me is not a dynamic
binary.

% bazel build -c opt //source/exe:envoy-static.stripped
Target //source/exe:envoy-static.stripped up-to-date:
  bazel-bin/source/exe/envoy-static.stripped

% ldd bazel-bin/source/exe/envoy-static.stripped
        not a dynamic executable

% ls -lh bazel-bin/source/exe/envoy-static.stripped
 -r-xr-xr-x 1 htuch eng 8.1M Apr  7 15:46 bazel-bin/source/exe/envoy-static.stripped
@mattklein123
Copy link
Member

@htuch does this need to be split somehow so it can pass CI? The bazel build is failing.

@htuch
Copy link
Member Author

htuch commented Apr 7, 2017

Yes, let me split out the base image part, sorry.

htuch added a commit to htuch/envoy that referenced this pull request Apr 7, 2017
This is needed for envoyproxy#716, it's going to temporarily increase the CI image size, but this will go away
when we remove the need for cmake.
)

# Used only for protobuf.bzl
native.git_repository(
Copy link
Member

Choose a reason for hiding this comment

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

we had similar hack in istio before, if you only intended use protobuf.bzl from this protobuf repo, use new_git_repository with build_file_content = "", it will disable the BUILD file in protobuf root, so the anything else (cc_library, etc) won't be loaded.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, wil fix.

mattklein123 pushed a commit that referenced this pull request Apr 8, 2017
This is needed for #716, it's going to temporarily increase the CI image size, but this will go away
when we remove the need for cmake.
@htuch
Copy link
Member Author

htuch commented Apr 9, 2017

There may be a cleaner way to do this actually, that doesn't involve offine .bzl generation, by defining a repository_rule and doing this entirely in Skylark. I'll see if it's easy to do, because if it is then this review can be dropped and we can get the same outcome with fewer moving parts.

@htuch htuch closed this Apr 10, 2017
nezdolik pushed a commit to nezdolik/envoy that referenced this pull request May 4, 2024
At first I try to add some functions as what Chrome does at their
https://chromium.googlesource.com/chromium/src/+/master/base/allocator/allocator_shim_override_ucrt_symbols_win.h,
but it still fails. So I decide to remove all heap-related objects
from libucrt.lib to see what happens. At the end I find that a lot of
functions in the CRT directly invoke _malloc_base instead of
malloc (and the others alike), hence we need to override them as well.

This should close issue envoyproxy#716.

[alkondratenko@gmail.com: added reference to ticket]
Signed-off-by: Aliaksey Kandratsenka <alkondratenko@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants