Skip to content

Commit

Permalink
[bazel] Get CPU tests to pass on RBE.
Browse files Browse the repository at this point in the history
bazel test //tests/... --config=cpu_only_debug --config=linux_rbe \
    --remote_download_minimal --test_output=errors

All but one of the tests passed initially. The failing test was one
that required a default font to have Bold, Italic, and Bold+Italic
variants. [1]

As it turns out, our RBE image only had a few fonts from the
DejaVu family, but no italic variants.

Thus, this CL updates the RBE image to include not only fonts
to make that test pass, but also fonts needed for GMs (when
we need to run those). However, we needed to go to an older
version of Debian because the latest Debian 12 (Bookworm)
version has a version of GLIBC that causes an issue with the
version of Docker used by RBE (20.10.8) [2].

Using an older RBE image required that the version of Clang
we use could be run with an older version of GLIBC. The Clang
15 binary available from GitHub [3] requires GLIBC 2.33, but
Debian 11 (Bullseye) only has 2.31.

Thus, in a previous CL [4], we changed Bazel to use the Clang
which we compile ourselves in a Debian 10 (buster) image for
use with the GN build.

With the GLIBC, Clang, and Docker situation sorted out, we
could add the fonts to the RBE image, following the steps
in //bazel/rbe/README.md.

The test still did not pass because none of the default font
family names that src/ports/SkFontMgr_custom.cpp tries to use
were installed (many of which are proprietary). So, we updated
that list to find the DejaVu Serif font, which is on many Linux
distros by default, and has Italic, Bold, and BoldItalic variants.

For good measure, we update the version of Bazel to a stable version
and not a release candidate.

A follow-on CL will add a CI job to run the tests.

Suggested Review Order:
 - src/ports/SkFontMgr_custom.cpp
 - bazel/rbe/gce_linux_container/Dockerfile to see the changes to
   the image.
 - bazel/platform/BUILD.bazel to see where the image hash needs
   to be updated such that it is used.
 - bazel/rbe/README.md
 - IGNORE everything in bazel/rbe/gce_linux/... as that is all
   generated
 - Everything else.

[1] https://github.com/google/skia/blob/0c437ea3502af8407c9f13fa99d4a2d84830a765/tests/TypefaceTest.cpp#L599-L611
[2] https://medium.com/nttlabs/ubuntu-21-10-and-fedora-35-do-not-work-on-docker-20-10-9-1cd439d9921
[3] https://github.com/llvm/llvm-project/releases/download/llvmorg-15.0.2/clang+llvm-15.0.2-x86_64-unknown-linux-gnu-rhel86.tar.xz
[4] https://skia-review.googlesource.com/c/skia/+/594807

Change-Id: I1b52928fe2922745387a4b2e31c98cb4862a4d41
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/593557
Reviewed-by: Ben Wagner <[email protected]>
  • Loading branch information
kjlubick committed Oct 26, 2022
1 parent e8a8a3b commit c0248ae
Show file tree
Hide file tree
Showing 14 changed files with 3,783 additions and 4,226 deletions.
2 changes: 1 addition & 1 deletion .bazelversion
Original file line number Diff line number Diff line change
@@ -1 +1 @@
5.3.0rc1
5.3.2
4 changes: 4 additions & 0 deletions bazel/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,10 @@ rbe_known_good_builds:
bazelisk build //modules/canvaskit:canvaskit --config=linux_rbe \
--config=ck_full_webgl2_debug --remote_download_minimal

rbe_known_good_tests:
bazel test //tests/... --config=cpu_only_debug --config=linux_rbe \
--remote_download_minimal --test_output=errors

iwyu_rbe:
bazelisk build //:skia_public --config=for_linux_x64_with_rbe \
--config=enforce_iwyu --remote_download_minimal --nobuild_runfile_manifests
Expand Down
1 change: 1 addition & 0 deletions bazel/buildrc
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ build:enforce_iwyu --features=skia_enforce_iwyu --cc_output_directory_tag=iwyu \


build:cpu_only --cc_output_directory_tag=cpu_tests
build:cpu_only_debug --config=cpu --config=debug

build:gl_ganesh --enable_gpu_test_utils --gpu_backend=gl_backend \
--cc_output_directory_tag=gl_ganesh
Expand Down
2 changes: 1 addition & 1 deletion bazel/platform/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ platform(
# See https://github.com/bazelbuild/bazel/blob/f28209df2b0ebeff1de0b8b7f6b9e215d890e753/src/main/java/com/google/devtools/build/lib/actions/ActionKeyCacher.java#L67-L73
# for how the exec_properties and execution platform impact the action cache.
exec_properties = create_rbe_exec_properties_dict(
container_image = "docker://gcr.io/skia-public/rbe_linux@sha256:4f7ea556fbf46f65f0c6a2d65144bbcb1139acc78ef19be4bd4b04dcfa623f18",
container_image = "docker://gcr.io/skia-public/rbe_linux@sha256:654139e5cecb163f80a7d18e2b2da6c758ebe6325d2d9c41d9facf58e1b3f799",
os_family = "Linux",
pool = "gce_linux",
),
Expand Down
4 changes: 2 additions & 2 deletions bazel/rbe/Makefile
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
LINUX_VERSION=v1
LINUX_VERSION=v2

build_linux_container:
docker build -t gcr.io/skia-public/rbe_linux:${LINUX_VERSION} ./gce_linux_container/
Expand All @@ -11,7 +11,7 @@ generate_linux_config:
# https://github.com/bazelbuild/bazel-toolchains/releases/tag/v5.1.2
rbe_configs_gen \
--bazel_version=5.0.0 \
--toolchain_container=gcr.io/skia-public/rbe_linux@sha256:4f7ea556fbf46f65f0c6a2d65144bbcb1139acc78ef19be4bd4b04dcfa623f18 \
--toolchain_container=gcr.io/skia-public/rbe_linux@sha256:654139e5cecb163f80a7d18e2b2da6c758ebe6325d2d9c41d9facf58e1b3f799 \
--output_src_root=../.. \
--output_config_path=bazel/rbe/gce_linux \
--exec_os=linux \
Expand Down
5 changes: 4 additions & 1 deletion bazel/rbe/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,14 @@ This process is:
3) Run `make build_linux_container` to build the image locally. One may verify it works by running
something like `docker run -it gcr.io/skia-public/rbe_linux:v2 /bin/bash`.
4) Note the versions and base image hash that were used. Modify the Dockerfile to use these.
1) `docker pull debian:bookworm-slim` is the easiest way to see the sha256 and get the latest.
2) Versions can be found looking for logs like:
`Get:89 http://deb.debian.org/debian bookworm/main amd64 clang amd64 1:14.0-55.2+b1 [9976 B]`
5) Run `make push_linux_container` to rebuild the container and push it to GCS where it can
be used by our RBE workers. Note the sha256 hash of this created container
6) Modify the appropriate generate step in `Makefile` (e.g. `generate_linux_config`) to refer
to the correct toolchain_container. Then, run that step.
7) Modify the RBE platform in `./BUILD.bazel` to refer to the new `container_image`.
7) Modify the RBE platform in `//platform/BUILD.bazel` to refer to the new `container_image`.

We chose not to use Bazel rules for this container step, as that could be difficult to bootstrap
without Bazel already setup. Additionally, Make is a simple and sufficient way to script the steps
Expand Down
123 changes: 52 additions & 71 deletions bazel/rbe/gce_linux/cc/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,7 @@ filegroup(

filegroup(
name = "compiler_deps",
srcs = glob(
["extra_tools/**"],
allow_empty = True,
) + [":builtin_include_directory_paths"],
srcs = glob(["extra_tools/**"], allow_empty = True) + [":builtin_include_directory_paths"],
)

# This is the entry point for --crosstool_top. Toolchains are found
Expand All @@ -59,100 +56,86 @@ cc_toolchain_suite(

cc_toolchain(
name = "cc-compiler-k8",
toolchain_identifier = "linux_gnu_x86",
toolchain_config = ":linux_gnu_x86",
all_files = ":compiler_deps",
ar_files = ":compiler_deps",
as_files = ":compiler_deps",
compiler_files = ":compiler_deps",
dwp_files = ":empty",
linker_files = ":compiler_deps",
module_map = ":module.modulemap",
objcopy_files = ":empty",
strip_files = ":empty",
supports_param_files = 1,
toolchain_config = ":linux_gnu_x86",
toolchain_identifier = "linux_gnu_x86",
module_map = ":module.modulemap",
)

cc_toolchain_config(
name = "linux_gnu_x86",
abi_libc_version = "glibc_2.19",
abi_version = "clang",
compile_flags = [
"-U_FORTIFY_SOURCE",
"-fstack-protector",
"-Wall",
"-Wthread-safety",
"-Wself-assign",
"-Wunused-but-set-parameter",
"-Wno-free-nonheap-object",
"-fcolor-diagnostics",
"-fno-omit-frame-pointer",
],
compiler = "clang",
coverage_compile_flags = ["--coverage"],
coverage_link_flags = ["--coverage"],
cpu = "k8",
cxx_builtin_include_directories = [
"/usr/lib/llvm-13/lib/clang/13.0.1/include",
"/usr/local/include",
"/usr/include/x86_64-linux-gnu",
"/usr/include",
"/usr/lib/llvm-13/lib/clang/13.0.1/share",
"/usr/include/c++/11",
"/usr/include/x86_64-linux-gnu/c++/11",
"/usr/include/c++/11/backward",
],
cxx_flags = ["-std=c++0x"],
dbg_compile_flags = ["-g"],
compiler = "clang",
toolchain_identifier = "linux_gnu_x86",
host_system_name = "i686-unknown-linux-gnu",
link_flags = [
"-fuse-ld=/usr/bin/ld.gold",
"-Wl,-no-as-needed",
"-Wl,-z,relro,-z,now",
"-B/usr/lib/llvm-13/bin",
],
link_libs = [
"-lstdc++",
"-lm",
],
opt_compile_flags = [
"-g0",
"-O2",
"-D_FORTIFY_SOURCE=1",
"-DNDEBUG",
"-ffunction-sections",
"-fdata-sections",
],
opt_link_flags = ["-Wl,--gc-sections"],
supports_start_end_lib = True,
target_libc = "glibc_2.19",
target_system_name = "x86_64-unknown-linux-gnu",
tool_paths = {
"ar": "/usr/bin/ar",
target_libc = "glibc_2.19",
abi_version = "clang",
abi_libc_version = "glibc_2.19",
cxx_builtin_include_directories = ["/usr/local/include",
"/usr/lib/llvm-11/lib/clang/11.0.1/include",
"/usr/include/x86_64-linux-gnu",
"/usr/include",
"/usr/lib/llvm-11/lib/clang/11.0.1/share",
"/usr/include/c++/10",
"/usr/include/x86_64-linux-gnu/c++/10",
"/usr/include/c++/10/backward"],
tool_paths = {"ar": "/usr/bin/ar",
"ld": "/usr/bin/ld",
"llvm-cov": "None",
"cpp": "/usr/bin/cpp",
"gcc": "/usr/lib/llvm-13/bin/clang",
"gcc": "/usr/lib/llvm-11/bin/clang",
"dwp": "/usr/bin/dwp",
"gcov": "None",
"nm": "/usr/bin/nm",
"objcopy": "/usr/bin/objcopy",
"objdump": "/usr/bin/objdump",
"strip": "/usr/bin/strip",
},
toolchain_identifier = "linux_gnu_x86",
unfiltered_compile_flags = [
"-no-canonical-prefixes",
"-Wno-builtin-macro-redefined",
"-D__DATE__=\"redacted\"",
"-D__TIMESTAMP__=\"redacted\"",
"-D__TIME__=\"redacted\"",
],
"strip": "/usr/bin/strip"},
compile_flags = ["-U_FORTIFY_SOURCE",
"-fstack-protector",
"-Wall",
"-Wthread-safety",
"-Wself-assign",
"-fcolor-diagnostics",
"-fno-omit-frame-pointer"],
opt_compile_flags = ["-g0",
"-O2",
"-D_FORTIFY_SOURCE=1",
"-DNDEBUG",
"-ffunction-sections",
"-fdata-sections"],
dbg_compile_flags = ["-g"],
cxx_flags = ["-std=c++0x"],
link_flags = ["-fuse-ld=/usr/bin/ld.gold",
"-Wl,-no-as-needed",
"-Wl,-z,relro,-z,now",
"-B/usr/lib/llvm-11/bin"],
link_libs = ["-lstdc++",
"-lm"],
opt_link_flags = ["-Wl,--gc-sections"],
unfiltered_compile_flags = ["-no-canonical-prefixes",
"-Wno-builtin-macro-redefined",
"-D__DATE__=\"redacted\"",
"-D__TIMESTAMP__=\"redacted\"",
"-D__TIME__=\"redacted\""],
coverage_compile_flags = ["--coverage"],
coverage_link_flags = ["--coverage"],
supports_start_end_lib = True,
)

# Android tooling requires a default toolchain for the armeabi-v7a cpu.
cc_toolchain(
name = "cc-compiler-armeabi-v7a",
toolchain_identifier = "stub_armeabi-v7a",
toolchain_config = ":stub_armeabi-v7a",
all_files = ":empty",
ar_files = ":empty",
as_files = ":empty",
Expand All @@ -162,8 +145,6 @@ cc_toolchain(
objcopy_files = ":empty",
strip_files = ":empty",
supports_param_files = 1,
toolchain_config = ":stub_armeabi-v7a",
toolchain_identifier = "stub_armeabi-v7a",
)

armeabi_cc_toolchain_config(name = "stub_armeabi-v7a")
12 changes: 6 additions & 6 deletions bazel/rbe/gce_linux/cc/builtin_include_directory_paths
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
This file is generated by cc_configure and contains builtin include directories
that /usr/lib/llvm-13/bin/clang reported. This file is a dependency of every compilation action and
that /usr/lib/llvm-11/bin/clang reported. This file is a dependency of every compilation action and
changes to it will be reflected in the action cache key. When some of these
paths change, Bazel will make sure to rerun the action, even though none of
declared action inputs or the action commandline changes.

/usr/lib/llvm-13/lib/clang/13.0.1/include
/usr/local/include
/usr/lib/llvm-11/lib/clang/11.0.1/include
/usr/include/x86_64-linux-gnu
/usr/include
/usr/lib/llvm-13/lib/clang/13.0.1/share
/usr/include/c++/11
/usr/include/x86_64-linux-gnu/c++/11
/usr/include/c++/11/backward
/usr/lib/llvm-11/lib/clang/11.0.1/share
/usr/include/c++/10
/usr/include/x86_64-linux-gnu/c++/10
/usr/include/c++/10/backward
2 changes: 1 addition & 1 deletion bazel/rbe/gce_linux/cc/cc_wrapper.sh
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,4 @@ set -eu


# Call the C++ compiler
/usr/lib/llvm-13/bin/clang "$@"
/usr/lib/llvm-11/bin/clang "$@"
Loading

0 comments on commit c0248ae

Please sign in to comment.