-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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: Add pre-compiled clang header poc #13788
Changes from all commits
d8018c7
c8a49bd
0225dc9
b575c29
8d1f918
0b5e2e1
4edefd1
702210b
bf2b2f5
6222c18
b8cb37a
93555c9
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 |
---|---|---|
@@ -0,0 +1,51 @@ | ||
load("@rules_cc//cc:defs.bzl", "cc_library") | ||
|
||
# DO NOT LOAD THIS FILE. Load envoy_build_system.bzl instead. | ||
# Envoy library targets | ||
load( | ||
":envoy_internal.bzl", | ||
"envoy_copts", | ||
"envoy_external_dep_path", | ||
"envoy_linkstatic", | ||
) | ||
load(":pch.bzl", "pch") | ||
|
||
def envoy_pch_copts(repository, target): | ||
return select({ | ||
repository + "//bazel:clang_pch_build": [ | ||
"-include-pch", | ||
"$(location {}{})".format(repository, target), | ||
], | ||
"//conditions:default": [], | ||
}) | ||
|
||
def envoy_pch_library( | ||
name, | ||
includes, | ||
deps, | ||
external_deps, | ||
visibility, | ||
testonly = False, | ||
repository = ""): | ||
cc_library( | ||
name = name + "_libs", | ||
visibility = ["//visibility:private"], | ||
copts = envoy_copts(repository), | ||
deps = deps + [envoy_external_dep_path(dep) for dep in external_deps], | ||
alwayslink = 1, | ||
testonly = testonly, | ||
linkstatic = envoy_linkstatic(), | ||
) | ||
|
||
pch( | ||
name = name, | ||
deps = [name + "_libs"], | ||
includes = includes, | ||
visibility = visibility, | ||
testonly = testonly, | ||
tags = ["no-remote"], | ||
enabled = select({ | ||
repository + "//bazel:clang_pch_build": True, | ||
"//conditions:default": False, | ||
}), | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,114 @@ | ||
load( | ||
"@bazel_tools//tools/build_defs/cc:action_names.bzl", | ||
"CPP_COMPILE_ACTION_NAME", | ||
) | ||
|
||
def _pch(ctx): | ||
deps_cc_info = cc_common.merge_cc_infos( | ||
cc_infos = [dep[CcInfo] for dep in ctx.attr.deps], | ||
) | ||
|
||
if not ctx.attr.enabled: | ||
return [deps_cc_info] | ||
|
||
cc_toolchain = ctx.attr._cc_toolchain[cc_common.CcToolchainInfo] | ||
feature_configuration = cc_common.configure_features( | ||
ctx = ctx, | ||
cc_toolchain = cc_toolchain, | ||
requested_features = ctx.features, | ||
unsupported_features = ctx.disabled_features, | ||
) | ||
|
||
cc_compiler_path = cc_common.get_tool_for_action( | ||
feature_configuration = feature_configuration, | ||
action_name = CPP_COMPILE_ACTION_NAME, | ||
) | ||
|
||
if "clang" not in cc_compiler_path: | ||
fail("error: attempting to use clang PCH without clang: {}".format(cc_compiler_path)) | ||
|
||
generated_header_file = ctx.actions.declare_file(ctx.label.name + ".h") | ||
ctx.actions.write( | ||
generated_header_file, | ||
"\n".join(["#include \"{}\"".format(include) for include in ctx.attr.includes]) + "\n", | ||
) | ||
|
||
# TODO: -fno-pch-timestamp / invalidation in that case doesn't work | ||
pch_flags = ["-x", "c++-header"] | ||
keith marked this conversation as resolved.
Show resolved
Hide resolved
|
||
pch_file = ctx.actions.declare_file(ctx.label.name + ".pch") | ||
|
||
deps_ctx = deps_cc_info.compilation_context | ||
cc_compile_variables = cc_common.create_compile_variables( | ||
feature_configuration = feature_configuration, | ||
cc_toolchain = cc_toolchain, | ||
user_compile_flags = ctx.fragments.cpp.copts + ctx.fragments.cpp.cxxopts + pch_flags, | ||
source_file = generated_header_file.path, | ||
output_file = pch_file.path, | ||
preprocessor_defines = depset(deps_ctx.defines.to_list() + deps_ctx.local_defines.to_list()), | ||
include_directories = deps_ctx.includes, | ||
quote_include_directories = deps_ctx.quote_includes, | ||
system_include_directories = deps_ctx.system_includes, | ||
framework_include_directories = deps_ctx.framework_includes, | ||
) | ||
|
||
env = cc_common.get_environment_variables( | ||
feature_configuration = feature_configuration, | ||
action_name = CPP_COMPILE_ACTION_NAME, | ||
variables = cc_compile_variables, | ||
) | ||
|
||
command_line = cc_common.get_memory_inefficient_command_line( | ||
feature_configuration = feature_configuration, | ||
action_name = CPP_COMPILE_ACTION_NAME, | ||
variables = cc_compile_variables, | ||
) | ||
|
||
transitive_headers = [] | ||
for dep in ctx.attr.deps: | ||
transitive_headers.append(dep[CcInfo].compilation_context.headers) | ||
ctx.actions.run( | ||
executable = cc_compiler_path, | ||
arguments = command_line, | ||
env = env, | ||
inputs = depset( | ||
items = [generated_header_file], | ||
transitive = [cc_toolchain.all_files] + transitive_headers, | ||
), | ||
outputs = [pch_file], | ||
) | ||
|
||
return [ | ||
DefaultInfo(files = depset(items = [pch_file])), | ||
cc_common.merge_cc_infos( | ||
direct_cc_infos = [ | ||
CcInfo( | ||
compilation_context = cc_common.create_compilation_context( | ||
headers = depset([pch_file, generated_header_file]), | ||
), | ||
), | ||
], | ||
cc_infos = [deps_cc_info], | ||
), | ||
] | ||
|
||
pch = rule( | ||
attrs = dict( | ||
includes = attr.string_list( | ||
mandatory = True, | ||
allow_empty = False, | ||
), | ||
deps = attr.label_list( | ||
mandatory = True, | ||
allow_empty = False, | ||
providers = [CcInfo], | ||
), | ||
enabled = attr.bool( | ||
mandatory = True, | ||
), | ||
_cc_toolchain = attr.label(default = Label("@bazel_tools//tools/cpp:current_cc_toolchain")), | ||
), | ||
fragments = ["cpp"], | ||
provides = [CcInfo], | ||
toolchains = ["@bazel_tools//tools/cpp:toolchain_type"], | ||
implementation = _pch, | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ load( | |
"envoy_cc_posix_library", | ||
"envoy_cc_win32_library", | ||
"envoy_package", | ||
"envoy_pch_library", | ||
) | ||
|
||
licenses(["notice"]) # Apache 2 | ||
|
@@ -469,3 +470,30 @@ envoy_cc_library( | |
"@com_google_absl//absl/status:statusor", | ||
], | ||
) | ||
|
||
envoy_pch_library( | ||
name = "common_pch", | ||
external_deps = [ | ||
"spdlog", | ||
], | ||
includes = [ | ||
"envoy/config/bootstrap/v3/bootstrap.pb.h", | ||
"envoy/config/cluster/v3/cluster.pb.h", | ||
"envoy/config/core/v3/base.pb.h", | ||
"envoy/config/core/v3/config_source.pb.h", | ||
"envoy/config/route/v3/route_components.pb.h", | ||
"envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.pb.h", | ||
"envoy/service/discovery/v3/discovery.pb.h", | ||
"spdlog/sinks/android_sink.h", | ||
"spdlog/spdlog.h", | ||
], | ||
Comment on lines
+479
to
+489
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. Not fan of directly listing those headers here. Can they be extracted from 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. So the problem is that the |
||
visibility = ["//visibility:public"], | ||
deps = [ | ||
"@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto", | ||
"@envoy_api//envoy/config/cluster/v3:pkg_cc_proto", | ||
"@envoy_api//envoy/config/core/v3:pkg_cc_proto", | ||
"@envoy_api//envoy/config/route/v3:pkg_cc_proto", | ||
"@envoy_api//envoy/extensions/filters/network/http_connection_manager/v3:pkg_cc_proto", | ||
"@envoy_api//envoy/service/discovery/v3:pkg_cc_proto", | ||
], | ||
) |
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.
use a bool_flag instead of
define
?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 couldn't get this option to work, started a thread on it here https://envoyproxy.slack.com/archives/C7E6C71QB/p1618869455054400
Either way it should be an easy swap if we want because no one should ever use this directly, they should only ever use the
--config=clang-pch
flag instead. We might actually prefer this way because it only requires 1 target vs 2There 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.
We talked offline and you can use
--@//
for this omitting theenvoy
part. But what do you think about merging this way since it's not meant for direct consumption anyways and this leaves us 1 less option to maintain since for the flags we have to have both.