[PATCH] D119547: [libc][bazel] Add tests to the bazel build

[Geoffrey Martin-Noble via Phabricator via llvm-commits]

GMNGeoffrey added inline comments.


================
Comment at: utils/bazel/.bazelrc:51
 # Use `-Wall` and `-Werror` for Clang.
-build:generic_clang --copt=-Wall --copt=-Werror --host_copt=-Wall --host_copt=-Werror
+build:generic_clang --copt=-Wall --copt=-Werror --host_copt=-Wall
 
----------------
Based on your other comment, I think this could be removed now? But also there's https://reviews.llvm.org/D123481 to remove -Werror from the default config. Note, though, that the CI still builds with -Werror


================
Comment at: utils/bazel/llvm-project-overlay/libc/test/libc_test_rules.bzl:1
+"""LLVM libc starlark rules for tests."""
+
----------------
Please add a license header and further explanation of why tests are constructed this way.


================
Comment at: utils/bazel/llvm-project-overlay/libc/test/libc_test_rules.bzl:13
+      additional_deps: List of additional targets used by this test.
+      **kw: Attributes relevant for a cc_test. For example, name, srcs, deps.
+    """
----------------
Can you use `**kwargs` for consistency with the rest of the starlark macros?


================
Comment at: utils/bazel/llvm-project-overlay/libc/test/libc_test_rules.bzl:16
+    if "deps" in kw:
+        fail("A libc_test target should list 'libc_function_deps' and " +
+             "'additional_deps' instead of 'deps'.")
----------------
It seems like calling the "additional_deps" argument "deps" would also be fine? Generally it's preferred to use standard names (https://bazel.build/rules/bzl-style). I do see the argument for different naming, thouhg. Your call


================
Comment at: utils/bazel/llvm-project-overlay/libc/test/src/math/BUILD.bazel:217
+
+# TODO(sivachandra): sqrtl test currently fails on Aarch64 because of
+# insufficient precision in MPFR leading to https://hal.archives-ouvertes.fr/hal-01091186/document
----------------
Couldn't we just tag it to be excluded from Aarch64 tests?


================
Comment at: utils/bazel/llvm-project-overlay/libc/test/src/math/libc_math_test_rules.bzl:36
+        additional_deps = additional_deps,
+        size = "enormous",  # Some tests are emulated and take very long.
+    )
----------------
Oh no 😨 How much does this extend CI runtime? Also, how many tests are like this? Instead of hardcoding "enormous" for every test, could you just pass that argument for the few that are in fact enormous? Separately, I think it is a better fit to use the `timeout` argument here `timeout = "eternal"`. Test size is ill-defined, but I think by any reasonable definition this test isn't an enormous one. Those usually involve spinning up whole servers and such. I think it's a small test that just happens to take a very long time.


================
Comment at: utils/bazel/third_party_build/mpfr.BUILD:11
+    toolchain = "@rules_foreign_cc//toolchains:preinstalled_autoconf_toolchain",
+    copts = ["-Wno-error"],  # ./configure crashes with `-Werror`
+    lib_name = "libmpfr",
----------------
Probably want to turn off all warnings entirely. Getting warnings from this is not going to be useful for anyone. I think `-w` is the flag for both clang and gcc.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119547/new/

https://reviews.llvm.org/D119547