[bazel] Change how copts are added to cc_library et al
I had previously thought that copts flow from one target to
its dependents, but they *do not*. This means copts behave
differently than linkopts and defines, so we must treat
them differently.
Background: The way our GN setup deals with flags is roughly
1) BUILDCONFIG.gn defines default_configs [1]. This is a list
of GN config targets. The list of configs can be augmented
using if statements and gn arguments [2].
2) A GN config target is effectively a collection of lists like
cflags, defines, ldflags [3] which are also augmented to
using if statements and gn arguments. `gn help config`
gives a few more details.
3) BUILDCONFIG.gn makes the default_configs the, uh, default
configs for targets like source_set, static_library [4].
4) The GN toolchain knows how to take the lists from configs,
deduplicate them, and turn them into compiler flags.
In addition, the toolchain adds some flags necessary for
compilation, such as what include directories to search [5].
5) If we need a target to set special flags, in BUILD.gn,
we add a config that encapsulates those flags [6] or
add them to cflags etc directly [7].
The organization structure for Bazel copts has some similarities
to the GN setup. Additionally, I tried to make it so that in the
common case of adding a new flag or making an existing flag
conditional, a developer only needs to go to one file to understand
what changes to make and to implement those changes.
//bazel/copts.bzl is that file. DEFAULT_COPTS, defined there, has
all the C++ flags needed to compile Skia (e.g. //:skia_public),
and things in //modules, but *not* third party code (e.g.
//bazel/externals/libpng). These copts are set to be the defaults
on any skia_cc_library, skia_cc_binary, or skia_objc_library
target exported from macros.bzl.
There is no way (like GN) to set these defaults globally
(which I think is good from an understandability POV).
The toolchain configurations, e.g. linux_amd64_toolchain_config.bzl,
still do set some flags (e.g. -std=c++17, -isystem...). Any flags
set in the toolchain will be used by any thing compiled with that
toolchain (this would be all Skia code, modules, and third_party,
but not if Skia is compiled by some other project).
Which flags go where? Nearly all flags should go in //bazel/copts.bzl
because it is very difficult to conditionally set flags in the
toolchain. We do set some target platform-specific ones, e.g.
--target=arm64-apple-macos11, but options controlled by our own
custom flags are not viewable. The current roadmap for Bazel
emphasizes Clang support over other compilers, but should we need to
target different compilers, we should be able to make those changes
in the select statements in //bazel/copts.bzl.
The linkopts (flags for the linker) continue to flow up the directory
structure, e.g. //tools/sk_app/unix/BUILD.bazel has the logic to
decide if we should link against X11-xcb or not and that logic
will come into play iff and only iff we are building a target
that uses that code.
In addition to linkopts flowing up, I added //bazel/linkopts.bzl
as a place to put global linker flags (e.g. related to dropping
dead code or link-time optimizations). These default linkopts are
added by default to skia_cc_binary targets defined from macros.bzl.
The copts and linkopts in this CL correspond to the default
cflags and cflags_cc from the GN rules. This introduced some
valid warnings, which are fixed here as well.
What about C-specific flags for compiling just C code? The Bazel
build of Skia has no C code (we ignore //src/c). Third party C
libraries will have their own copts, which can be C-specific.
What if we need to coordinate flags between Skia and its
third_party libraries? Such coordinated flags should probably
go in the toolchain configuration. If that's not viable, we can
have the flags live in a sub-workspace that both Skia and
the third_party libraries include (similar to how we deal with
freetype configuration files).
Why don't the warning flags we set trigger on third_party headers
that are included as deps or transitive deps? The use of `includes`
in the third_party cc_library rule makes them added to the compiler
flag with -isystem [8]. Clang takes this as a cue to suppress the
diagnostic warnings.
What about "warnings_for_public_headers" that we had for GN to
enforce certain warnings for our public API? We will need to
construct a new target that sets the additional warnings and
includes all the Skia header files we want to enforce this.
See also http://cl/463893799
[1] https://github.com/google/skia/blob/bd8826a8dfe814dcbd9ed4569232dea2a420cece/gn/BUILDCONFIG.gn#L177
[2] https://github.com/google/skia/blob/bd8826a8dfe814dcbd9ed4569232dea2a420cece/gn/BUILDCONFIG.gn#L182-L187
[3] https://github.com/google/skia/blob/bd8826a8dfe814dcbd9ed4569232dea2a420cece/gn/skia/BUILD.gn#L50-L344
[4] https://github.com/google/skia/blob/bd8826a8dfe814dcbd9ed4569232dea2a420cece/gn/BUILDCONFIG.gn#L197-L203
[5] https://github.com/google/skia/blob/bd8826a8dfe814dcbd9ed4569232dea2a420cece/gn/toolchain/BUILD.gn#L260
[6] https://github.com/google/skia/blob/bd8826a8dfe814dcbd9ed4569232dea2a420cece/BUILD.gn#L1205
[7] https://github.com/google/skia/blob/bd8826a8dfe814dcbd9ed4569232dea2a420cece/BUILD.gn#L258
[8] https://skia-review.googlesource.com/c/skia/+/562311
Bug: skia:12541
Change-Id: Ib522f158f1205790e9695ab4d13cafdbcb6d6df5
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/562336
Reviewed-by: Ben Wagner <bungeman@google.com>
44 files changed